Implement dependency resolution for services

.phpstorm.meta.php

@@ -12,11 +12,12 @@
 			'admin.onboarding_menu'              => \AmpProject\AmpWP\Admin\OnboardingWizardSubmenu::class,
 			'admin.onboarding_wizard'            => \AmpProject\AmpWP\Admin\OnboardingWizardSubmenuPage::class,
 			'admin.options_menu'                 => \AmpProject\AmpWP\Admin\OptionsMenu::class,
-			'admin.polyfills'                    => \AmpProject\AmpWP\Admin\Polyfills::class,
 			'admin.paired_browsing'              => \AmpProject\AmpWP\Admin\PairedBrowsing::class,
-			'admin.validation_counts'            => \AmpProject\AmpWP\Admin\ValidationCounts::class,
 			'admin.plugin_row_meta'              => \AmpProject\AmpWP\Admin\PluginRowMeta::class,
+			'admin.polyfills'                    => \AmpProject\AmpWP\Admin\Polyfills::class,
+			'admin.validation_counts'            => \AmpProject\AmpWP\Admin\ValidationCounts::class,
 			'amp_slug_customization_watcher'     => \AmpProject\AmpWP\AmpSlugCustomizationWatcher::class,
+			'background_task_deactivator'        => \AmpProject\AmpWP\BackgroundTask\BackgroundTaskDeactivator::class,
 			'cli.command_namespace'              => \AmpProject\AmpWP\CliCli\CommandNamespaceRegistration::class,
 			'cli.optimizer_command'              => \AmpProject\AmpWP\CliCli\OptimizerCommand::class,
 			'cli.transformer_command'            => \AmpProject\AmpWP\CliCli\TransformerCommand::class,
@@ -33,26 +34,26 @@
 			'editor.editor_support'              => \AmpProject\AmpWP\Editor\EditorSupport::class,
 			'extra_theme_and_plugin_headers'     => \AmpProject\AmpWP\ExtraThemeAndPluginHeaders::class,
 			'injector'                           => \AmpProject\AmpWP\Infrastructure\Injector::class,
+			'loading_error'                      => \AmpProject\AmpWP\LoadingError::class,
 			'mobile_redirection'                 => \AmpProject\AmpWP\MobileRedirection::class,
 			'obsolete_block_attribute_remover'   => \AmpProject\AmpWP\ObsoleteBlockAttributeRemover::class,
 			'optimizer'                          => \AmpProject\AmpWP\Optimizer\OptimizerService::class,
 			'optimizer.hero_candidate_filtering' => \AmpProject\AmpWP\Optimizer\HeroCandidateFiltering::class,
+			'paired_routing'                     => \AmpProject\AmpWP\PairedRouting::class,
+			'paired_url'                         => \AmpProject\AmpWP\PairedUrl::class,
 			'plugin_activation_notice'           => \AmpProject\AmpWP\Admin\PluginActivationNotice::class,
 			'plugin_registry'                    => \AmpProject\AmpWP\PluginRegistry::class,
 			'plugin_suppression'                 => \AmpProject\AmpWP\PluginSuppression::class,
 			'reader_theme_loader'                => \AmpProject\AmpWP\ReaderThemeLoader::class,
 			'reader_theme_support_features'      => \AmpProject\AmpWP\ReaderThemeSupportFeatures::class,
 			'rest.options_controller'            => \AmpProject\AmpWP\OptionsRESTController::class,
 			'rest.validation_counts_controller'  => \AmpProject\AmpWP\Validation\ValidationCountsRestController::class,
+			'save_post_validation_event'         => \AmpProject\AmpWP\Validation\SavePostValidationEvent::class,
 			'server_timing'                      => \AmpProject\AmpWP\Instrumentation\ServerTiming::class,
 			'site_health_integration'            => \AmpProject\AmpWP\Admin\SiteHealth::class,
-			'validated_url_stylesheet_gc'        => \AmpProject\AmpWP\BackgroundTask\ValidatedUrlStylesheetDataGarbageCollection::class,
 			'url_validation_cron'                => \AmpProject\AmpWP\Validation\URLValidationCron::class,
-			'save_post_validation_event'         => \AmpProject\AmpWP\Validation\SavePostValidationEvent::class,
-			'background_task_deactivator'        => \AmpProject\AmpWP\BackgroundTask\BackgroundTaskDeactivator::class,
-			'paired_routing'                     => \AmpProject\AmpWP\PairedRouting::class,
-			'paired_url'                         => \AmpProject\AmpWP\PairedUrl::class,
-			'loading_error'                      => \AmpProject\AmpWP\LoadingError::class,
+			'url_validation_rest_controller'     => \AmpProject\AmpWP\Validation\URLValidationRESTController::class,
+			'validated_url_stylesheet_gc'        => \AmpProject\AmpWP\BackgroundTask\ValidatedUrlStylesheetDataGarbageCollection::class,
 		] )
 	);
 

codecov.yml

@@ -1,10 +1,5 @@
 # See documentation at https://docs.codecov.io/docs/codecovyml-reference
 
-codecov:
-  notify:
-    # Wait for Jest & PHPUnit $ Behat reports before notifying
-    after_n_builds: 3
-
 coverage:
   status:
     # Patch-level coverage (how well is the PR tested)

src/Admin/PairedBrowsing.php

@@ -12,6 +12,7 @@
 use AMP_Validation_Manager;
 use AMP_Validated_URL_Post_Type;
 use AmpProject\AmpWP\Infrastructure\Conditional;
+use AmpProject\AmpWP\Infrastructure\HasRequirements;
 use AmpProject\AmpWP\Infrastructure\Registerable;
 use AmpProject\AmpWP\Infrastructure\Service;
 use AmpProject\AmpWP\Option;
@@ -29,7 +30,7 @@
  * @since 2.1
  * @internal
  */
-final class PairedBrowsing implements Service, Registerable, Conditional {
+final class PairedBrowsing implements Service, Registerable, Conditional, HasRequirements {
 
 	/**
 	 * Query var for requests to open the app.
@@ -73,6 +74,17 @@ public static function is_needed() {
 		);
 	}
 
+	/**
+	 * Get the list of service IDs required for this service to be registered.
+	 *
+	 * @return string[] List of required services.
+	 */
+	public static function get_requirements() {
+		return [
+			'dependency_support',
+		];
+	}
+
 	/**
 	 * PairedBrowsing constructor.
 	 *

src/Admin/Polyfills.php

@@ -11,6 +11,7 @@
 
 use AmpProject\AmpWP\Infrastructure\Conditional;
 use AmpProject\AmpWP\Infrastructure\Delayed;
+use AmpProject\AmpWP\Infrastructure\HasRequirements;
 use AmpProject\AmpWP\Infrastructure\Registerable;
 use AmpProject\AmpWP\Infrastructure\Service;
 use AmpProject\AmpWP\Services;
@@ -23,7 +24,7 @@
  * @since 2.0
  * @internal
  */
-final class Polyfills implements Conditional, Delayed, Service, Registerable {
+final class Polyfills implements Conditional, Delayed, Service, Registerable, HasRequirements {
 
 	/**
 	 * Get the action to use for registering the service.
@@ -43,6 +44,17 @@ public static function is_needed() {
 		return ! Services::get( 'dependency_support' )->has_support();
 	}
 
+	/**
+	 * Get the list of service IDs required for this service to be registered.
+	 *
+	 * @return string[] List of required services.
+	 */
+	public static function get_requirements() {
+		return [
+			'dependency_support',
+		];
+	}
+
 	/**
 	 * Runs on instantiation.
 	 */

src/Admin/ValidationCounts.php

@@ -9,6 +9,7 @@
 
 use AmpProject\AmpWP\Infrastructure\Conditional;
 use AmpProject\AmpWP\Infrastructure\Delayed;
+use AmpProject\AmpWP\Infrastructure\HasRequirements;
 use AmpProject\AmpWP\Infrastructure\Registerable;
 use AmpProject\AmpWP\Infrastructure\Service;
 use AmpProject\AmpWP\Services;
@@ -20,7 +21,7 @@
  * @since 2.1
  * @internal
  */
-final class ValidationCounts implements Service, Registerable, Conditional, Delayed {
+final class ValidationCounts implements Service, Registerable, Conditional, Delayed, HasRequirements {
 
 	/**
 	 * Assets handle.
@@ -38,6 +39,18 @@ public static function get_registration_action() {
 		return 'admin_enqueue_scripts';
 	}
 
+	/**
+	 * Get the list of service IDs required for this service to be registered.
+	 *
+	 * @return string[] List of required services.
+	 */
+	public static function get_requirements() {
+		return [
+			'dependency_support',
+			'dev_tools.user_access',
+		];
+	}
+
 	/**
 	 * Check whether the conditional object is currently needed.
 	 *

src/AmpWpPlugin.php

@@ -67,23 +67,24 @@ final class AmpWpPlugin extends ServiceBasedPlugin {
 	 * @var string[]
 	 */
 	const SERVICES = [
-		'dependency_support'                 => DependencySupport::class, // Needs to be registered first as other services depend on it.
 		'admin.analytics_menu'               => Admin\AnalyticsOptionsSubmenu::class,
 		'admin.google_fonts'                 => Admin\GoogleFonts::class,
 		'admin.onboarding_menu'              => Admin\OnboardingWizardSubmenu::class,
 		'admin.onboarding_wizard'            => Admin\OnboardingWizardSubmenuPage::class,
 		'admin.options_menu'                 => Admin\OptionsMenu::class,
-		'admin.polyfills'                    => Admin\Polyfills::class,
 		'admin.paired_browsing'              => Admin\PairedBrowsing::class,
-		'admin.validation_counts'            => Admin\ValidationCounts::class,
 		'admin.plugin_row_meta'              => Admin\PluginRowMeta::class,
+		'admin.polyfills'                    => Admin\Polyfills::class,
+		'admin.validation_counts'            => Admin\ValidationCounts::class,
 		'amp_slug_customization_watcher'     => AmpSlugCustomizationWatcher::class,
+		'background_task_deactivator'        => BackgroundTaskDeactivator::class,
 		'cli.command_namespace'              => Cli\CommandNamespaceRegistration::class,
 		'cli.optimizer_command'              => Cli\OptimizerCommand::class,
 		'cli.transformer_command'            => Cli\TransformerCommand::class,
 		'cli.validation_command'             => Cli\ValidationCommand::class,
 		'css_transient_cache.ajax_handler'   => Admin\ReenableCssTransientCachingAjaxAction::class,
 		'css_transient_cache.monitor'        => BackgroundTask\MonitorCssTransientCaching::class,
+		'dependency_support'                 => DependencySupport::class, // Needs to be registered first as other services depend on it.
 		'dev_tools.block_sources'            => DevTools\BlockSources::class,
 		'dev_tools.callback_reflection'      => DevTools\CallbackReflection::class,
 		'dev_tools.error_page'               => DevTools\ErrorPage::class,
@@ -92,27 +93,26 @@ final class AmpWpPlugin extends ServiceBasedPlugin {
 		'dev_tools.user_access'              => DevTools\UserAccess::class,
 		'editor.editor_support'              => Editor\EditorSupport::class,
 		'extra_theme_and_plugin_headers'     => ExtraThemeAndPluginHeaders::class,
+		'loading_error'                      => LoadingError::class,
 		'mobile_redirection'                 => MobileRedirection::class,
 		'obsolete_block_attribute_remover'   => ObsoleteBlockAttributeRemover::class,
 		'optimizer'                          => OptimizerService::class,
 		'optimizer.hero_candidate_filtering' => HeroCandidateFiltering::class,
+		'paired_routing'                     => PairedRouting::class,
+		'paired_url'                         => PairedUrl::class,
 		'plugin_activation_notice'           => Admin\PluginActivationNotice::class,
 		'plugin_registry'                    => PluginRegistry::class,
 		'plugin_suppression'                 => PluginSuppression::class,
 		'reader_theme_loader'                => ReaderThemeLoader::class,
 		'reader_theme_support_features'      => ReaderThemeSupportFeatures::class,
 		'rest.options_controller'            => OptionsRESTController::class,
 		'rest.validation_counts_controller'  => Validation\ValidationCountsRestController::class,
+		'save_post_validation_event'         => SavePostValidationEvent::class,
 		'server_timing'                      => Instrumentation\ServerTiming::class,
 		'site_health_integration'            => Admin\SiteHealth::class,
-		'validated_url_stylesheet_gc'        => BackgroundTask\ValidatedUrlStylesheetDataGarbageCollection::class,
-		'url_validation_rest_controller'     => Validation\URLValidationRESTController::class,
 		'url_validation_cron'                => URLValidationCron::class,
-		'save_post_validation_event'         => SavePostValidationEvent::class,
-		'background_task_deactivator'        => BackgroundTaskDeactivator::class,
-		'paired_routing'                     => PairedRouting::class,
-		'paired_url'                         => PairedUrl::class,
-		'loading_error'                      => LoadingError::class,
+		'url_validation_rest_controller'     => Validation\URLValidationRESTController::class,
+		'validated_url_stylesheet_gc'        => BackgroundTask\ValidatedUrlStylesheetDataGarbageCollection::class,
 	];
 
 	/**

src/Infrastructure/HasRequirements.php

@@ -0,0 +1,27 @@
+<?php
+/**
+ * Interface HasRequirements.
+ *
+ * @package AmpProject\AmpWP
+ */
+
+namespace AmpProject\AmpWP\Infrastructure;
+
+/**
+ * Something that requires other services to be registered before it can be registered.
+ *
+ * A class marked as having requirements can return the list of services it requires
+ * to be available before it can be registered.
+ *
+ * @since 2.2
+ * @internal
+ */
+interface HasRequirements {
+
+	/**
+	 * Get the list of service IDs required for this service to be registered.
+	 *
+	 * @return string[] List of required services.
+	 */
+	public static function get_requirements();
+}

src/Infrastructure/ServiceBasedPlugin.php

@@ -7,6 +7,7 @@
 
 namespace AmpProject\AmpWP\Infrastructure;
 
+use AmpProject\AmpWP\Exception\InvalidRequirement;
 use AmpProject\AmpWP\Exception\InvalidService;
 use AmpProject\AmpWP\Infrastructure\ServiceContainer\LazilyInstantiatedService;
 use WP_CLI;
@@ -187,32 +188,79 @@ public function register_services() {
 			$services = $this->validate_services( $filtered_services, $services );
 		}
 
-		foreach ( $services as $id => $class ) {
-			$id    = $this->maybe_resolve( $id );
-			$class = $this->maybe_resolve( $class );
+		while ( null !== key( $services ) ) {
+			$id    = $this->maybe_resolve( key( $services ) );
+			$class = $this->maybe_resolve( current( $services ) );
+
+			// Delay registering the service until all requirements are met.
+			if (
+				is_a( $class, HasRequirements::class, true )
+				&&
+				! $this->requirements_are_met( $class, array_keys( $services ) )
+			) {
+				/*
+				 * Move the service to the end of the array.
+				 *
+				 * Note: Unsetting the key advances the internal array pointer to the next array item, so
+				 * the current array item will have to be temporarily stored so that it can be re-added later.
+				 */
+				$delayed_service = [ key( $services ) => current( $services ) ];
+				unset( $services[ key( $services ) ] );
+				$services[ key( $delayed_service ) ] = current( $delayed_service );
+
+				continue;
+			}
 
 			// Allow the services to delay their registration.
 			if ( is_a( $class, Delayed::class, true ) ) {
 				$registration_action = $class::get_registration_action();
 
 				if ( did_action( $registration_action ) ) {
 					$this->register_service( $id, $class );
-
-					continue;
+				} else {
+					\add_action(
+						$class::get_registration_action(),
+						function () use ( $id, $class ) {
+							$this->register_service( $id, $class );
+						}
+					);
 				}
 
-				\add_action(
-					$class::get_registration_action(),
-					function () use ( $id, $class ) {
-						$this->register_service( $id, $class );
-					}
-				);
-
+				next( $services );
 				continue;
 			}
 
 			$this->register_service( $id, $class );
+
+			next( $services );
+		}
+	}
+
+	/**
+	 * Determine if the requirements for a service to be registered are met.
+	 *
+	 * @param HasRequirements $class       Service with requirements.
+	 * @param string[]        $service_ids List of service IDs to be registered.
+	 *
+	 * @throws InvalidService If the required service is not recognized.
+	 *
+	 * @return bool Whether the requirements for the service has been met.
+	 */
+	protected function requirements_are_met( $class, $service_ids ) {
+		$requirements = $class::get_requirements();
+
+		foreach ( $requirements as $requirement ) {
+			// Bail if it requires a service that is not recognized.
+			if ( ! in_array( $requirement, $service_ids, true ) ) {
+				throw InvalidService::from_service_id( $requirement );
+			}
+
+			if ( ! $this->get_container()->has( $requirement ) ) {
+				return false;
+			}
 		}
+
+		return true;
 	}
 
 	/**

tests/php/src/Admin/PairedBrowsingTest.php

@@ -11,6 +11,7 @@
 use AMP_Theme_Support;
 use AmpProject\AmpWP\Admin\PairedBrowsing;
 use AmpProject\AmpWP\Infrastructure\Conditional;
+use AmpProject\AmpWP\Infrastructure\HasRequirements;
 use AmpProject\AmpWP\Infrastructure\Registerable;
 use AmpProject\AmpWP\Infrastructure\Service;
 use AmpProject\AmpWP\Option;
@@ -52,12 +53,18 @@ public function test_is_needed() {
 		$this->assertSame( Services::get( 'dependency_support' )->has_support(), PairedBrowsing::is_needed() );
 	}
 
+	/** @covers ::get_requirements() */
+	public function test_get_requirements() {
+		$this->assertSame( [ 'dependency_support' ], PairedBrowsing::get_requirements() );
+	}
+
 	/** @covers ::__construct() */
 	public function test__construct() {
 		$this->assertInstanceOf( PairedBrowsing::class, $this->instance );
 		$this->assertInstanceOf( Service::class, $this->instance );
 		$this->assertInstanceOf( Registerable::class, $this->instance );
 		$this->assertInstanceOf( Conditional::class, $this->instance );
+		$this->assertInstanceOf( HasRequirements::class, $this->instance );
 	}
 
 	/** @covers ::register() */