From 874b337bddacec121e9bfc24767b08da73f29f34 Mon Sep 17 00:00:00 2001 From: Pierre Gordon Date: Sat, 24 Jul 2021 20:23:35 -0400 Subject: [PATCH 01/16] Implement dependency resolution for services --- src/Admin/PairedBrowsing.php | 14 +++++++- src/Admin/Polyfills.php | 14 +++++++- src/Admin/ValidationCounts.php | 15 +++++++- src/AmpWpPlugin.php | 20 +++++------ src/Infrastructure/HasRequirements.php | 27 ++++++++++++++ src/Infrastructure/ServiceBasedPlugin.php | 41 ++++++++++++++++++++-- src/Validation/SavePostValidationEvent.php | 1 + 7 files changed, 116 insertions(+), 16 deletions(-) create mode 100644 src/Infrastructure/HasRequirements.php diff --git a/src/Admin/PairedBrowsing.php b/src/Admin/PairedBrowsing.php index ec0338581d0..b96cb2e18a4 100644 --- a/src/Admin/PairedBrowsing.php +++ b/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 array List of required services. + */ + public static function get_requirements() { + return [ + 'dependency_support', + ]; + } + /** * PairedBrowsing constructor. * diff --git a/src/Admin/Polyfills.php b/src/Admin/Polyfills.php index ce5dfe3b261..ed7d2186e4b 100644 --- a/src/Admin/Polyfills.php +++ b/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 array List of required services. + */ + public static function get_requirements() { + return [ + 'dependency_support', + ]; + } + /** * Runs on instantiation. */ diff --git a/src/Admin/ValidationCounts.php b/src/Admin/ValidationCounts.php index b155034b5df..3c878e412d9 100644 --- a/src/Admin/ValidationCounts.php +++ b/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 array List of required services. + */ + public static function get_requirements() { + return [ + 'dependency_support', + 'dev_tools.user_access', + ]; + } + /** * Check whether the conditional object is currently needed. * diff --git a/src/AmpWpPlugin.php b/src/AmpWpPlugin.php index e1997bba6ef..6477931e650 100644 --- a/src/AmpWpPlugin.php +++ b/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,10 +93,13 @@ 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, @@ -104,15 +108,11 @@ final class AmpWpPlugin extends ServiceBasedPlugin { 'rest.options_controller' => OptionsRESTController::class, 'rest.validation_counts_controller' => Validation\ValidationCountsRestController::class, 'server_timing' => Instrumentation\ServerTiming::class, + 'save_post_validation_event' => SavePostValidationEvent::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, ]; /** diff --git a/src/Infrastructure/HasRequirements.php b/src/Infrastructure/HasRequirements.php new file mode 100644 index 00000000000..22abd0b990b --- /dev/null +++ b/src/Infrastructure/HasRequirements.php @@ -0,0 +1,27 @@ +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 ) + ) { + // Move to the end of the array. + unset( $services[ key( $services ) ] ); + $services += [ key( $services ) => current( $services ) ]; + + continue; + } // Allow the services to delay their registration. if ( is_a( $class, Delayed::class, true ) ) { @@ -198,6 +211,7 @@ public function register_services() { if ( did_action( $registration_action ) ) { $this->register_service( $id, $class ); + next( $services ); continue; } @@ -208,11 +222,32 @@ function () use ( $id, $class ) { } ); + next( $services ); continue; } $this->register_service( $id, $class ); + + next( $services ); + } + } + + /** + * @param HasRequirements $class + * + * @return bool + */ + protected function requirements_are_met( $class ) { + $requirements = $class::get_requirements(); + + // TODO: bail if it requires itself. + foreach ( $requirements as $requirement ) { + if ( ! $this->get_container()->has( $requirement ) ) { + return false; + } } + + return true; } /** diff --git a/src/Validation/SavePostValidationEvent.php b/src/Validation/SavePostValidationEvent.php index 4041ddbc9aa..87389699d2d 100644 --- a/src/Validation/SavePostValidationEvent.php +++ b/src/Validation/SavePostValidationEvent.php @@ -12,6 +12,7 @@ use AmpProject\AmpWP\BackgroundTask\SingleScheduledBackgroundTask; use AmpProject\AmpWP\DevTools\UserAccess; use AmpProject\AmpWP\Infrastructure\Conditional; +use AmpProject\AmpWP\Services; /** * SavePostValidationEvent class. From 0ce84931b82a7fdff89d83a1e9a9242ad69e123f Mon Sep 17 00:00:00 2001 From: Pierre Gordon Date: Sat, 24 Jul 2021 20:40:08 -0400 Subject: [PATCH 02/16] Update .phpstorm.meta.php --- .phpstorm.meta.php | 17 +++++++++-------- src/AmpWpPlugin.php | 2 +- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/.phpstorm.meta.php b/.phpstorm.meta.php index 0e2fc394232..edaaa686499 100644 --- a/.phpstorm.meta.php +++ b/.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,10 +34,13 @@ '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, @@ -44,15 +48,12 @@ '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, ] ) ); diff --git a/src/AmpWpPlugin.php b/src/AmpWpPlugin.php index 6477931e650..ee13dd28b46 100644 --- a/src/AmpWpPlugin.php +++ b/src/AmpWpPlugin.php @@ -107,8 +107,8 @@ final class AmpWpPlugin extends ServiceBasedPlugin { 'reader_theme_support_features' => ReaderThemeSupportFeatures::class, 'rest.options_controller' => OptionsRESTController::class, 'rest.validation_counts_controller' => Validation\ValidationCountsRestController::class, - 'server_timing' => Instrumentation\ServerTiming::class, 'save_post_validation_event' => SavePostValidationEvent::class, + 'server_timing' => Instrumentation\ServerTiming::class, 'site_health_integration' => Admin\SiteHealth::class, 'url_validation_cron' => URLValidationCron::class, 'url_validation_rest_controller' => Validation\URLValidationRESTController::class, From e3c45dc85776eb189a9f2e27df8256ae22f4eb19 Mon Sep 17 00:00:00 2001 From: Pierre Gordon Date: Sun, 25 Jul 2021 17:33:48 -0400 Subject: [PATCH 03/16] Fix delayed service not being re-inserted into the array --- src/Infrastructure/ServiceBasedPlugin.php | 33 ++++++++++++----------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/src/Infrastructure/ServiceBasedPlugin.php b/src/Infrastructure/ServiceBasedPlugin.php index ab31a7aede1..9c03744f139 100644 --- a/src/Infrastructure/ServiceBasedPlugin.php +++ b/src/Infrastructure/ServiceBasedPlugin.php @@ -188,7 +188,7 @@ public function register_services() { } while ( null !== key( $services ) ) { - $id = $this->maybe_resolve( key($services) ); + $id = $this->maybe_resolve( key( $services ) ); $class = $this->maybe_resolve( current( $services ) ); // Delay registering the service until all requirements are met. @@ -197,9 +197,15 @@ public function register_services() { && ! $this->requirements_are_met( $class ) ) { - // Move to the end of the array. + /* + * 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( $services ) => current( $services ) ]; + $services[ key( $delayed_service ) ] = current( $delayed_service ); continue; } @@ -210,18 +216,15 @@ public function register_services() { if ( did_action( $registration_action ) ) { $this->register_service( $id, $class ); - - next( $services ); - 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; } @@ -233,9 +236,9 @@ function () use ( $id, $class ) { } /** - * @param HasRequirements $class + * @param HasRequirements $class Service with dependency requirements. * - * @return bool + * @return bool Whether the requirements for the service has been met. */ protected function requirements_are_met( $class ) { $requirements = $class::get_requirements(); From c80decdff31fbf2efecdb9e3600c337ddc41ed53 Mon Sep 17 00:00:00 2001 From: Pierre Gordon Date: Tue, 27 Jul 2021 12:57:45 -0400 Subject: [PATCH 04/16] Test registering a service with requirements --- src/Admin/PairedBrowsing.php | 2 +- src/Admin/Polyfills.php | 2 +- src/Admin/ValidationCounts.php | 2 +- src/Infrastructure/HasRequirements.php | 2 +- src/Infrastructure/ServiceBasedPlugin.php | 18 +++++-- src/Validation/SavePostValidationEvent.php | 1 - tests/php/src/Admin/PairedBrowsingTest.php | 7 +++ tests/php/src/Admin/PolyfillsTest.php | 7 +++ tests/php/src/Admin/ValidationCountsTest.php | 10 ++++ .../Fixture/DummyServiceWithRequirements.php | 19 +++++++ .../Infrastructure/ServiceBasedPluginTest.php | 49 +++++++++++++++++++ 11 files changed, 110 insertions(+), 9 deletions(-) create mode 100644 tests/php/src/Fixture/DummyServiceWithRequirements.php diff --git a/src/Admin/PairedBrowsing.php b/src/Admin/PairedBrowsing.php index b96cb2e18a4..b38c1234e45 100644 --- a/src/Admin/PairedBrowsing.php +++ b/src/Admin/PairedBrowsing.php @@ -77,7 +77,7 @@ public static function is_needed() { /** * Get the list of service IDs required for this service to be registered. * - * @return array List of required services. + * @return array List of required services. */ public static function get_requirements() { return [ diff --git a/src/Admin/Polyfills.php b/src/Admin/Polyfills.php index ed7d2186e4b..84e01340812 100644 --- a/src/Admin/Polyfills.php +++ b/src/Admin/Polyfills.php @@ -47,7 +47,7 @@ public static function is_needed() { /** * Get the list of service IDs required for this service to be registered. * - * @return array List of required services. + * @return array List of required services. */ public static function get_requirements() { return [ diff --git a/src/Admin/ValidationCounts.php b/src/Admin/ValidationCounts.php index 3c878e412d9..68b4408e79e 100644 --- a/src/Admin/ValidationCounts.php +++ b/src/Admin/ValidationCounts.php @@ -42,7 +42,7 @@ public static function get_registration_action() { /** * Get the list of service IDs required for this service to be registered. * - * @return array List of required services. + * @return array List of required services. */ public static function get_requirements() { return [ diff --git a/src/Infrastructure/HasRequirements.php b/src/Infrastructure/HasRequirements.php index 22abd0b990b..94a11f98e45 100644 --- a/src/Infrastructure/HasRequirements.php +++ b/src/Infrastructure/HasRequirements.php @@ -21,7 +21,7 @@ interface HasRequirements { /** * Get the list of service IDs required for this service to be registered. * - * @return array List of required services. + * @return array List of required services. */ public static function get_requirements(); } diff --git a/src/Infrastructure/ServiceBasedPlugin.php b/src/Infrastructure/ServiceBasedPlugin.php index 9c03744f139..2043c48e32f 100644 --- a/src/Infrastructure/ServiceBasedPlugin.php +++ b/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; @@ -195,7 +196,7 @@ public function register_services() { if ( is_a( $class, HasRequirements::class, true ) && - ! $this->requirements_are_met( $class ) + ! $this->requirements_are_met( $class, array_keys( $services ) ) ) { /* * Move the service to the end of the array. @@ -236,15 +237,24 @@ function () use ( $id, $class ) { } /** - * @param HasRequirements $class Service with dependency requirements. + * Determine if the requirements for a service to be registered are met. + * + * @param HasRequirements $class Service with requirements. + * @param array $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 ) { + protected function requirements_are_met( $class, $service_ids ) { $requirements = $class::get_requirements(); - // TODO: bail if it requires itself. 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; } diff --git a/src/Validation/SavePostValidationEvent.php b/src/Validation/SavePostValidationEvent.php index 87389699d2d..4041ddbc9aa 100644 --- a/src/Validation/SavePostValidationEvent.php +++ b/src/Validation/SavePostValidationEvent.php @@ -12,7 +12,6 @@ use AmpProject\AmpWP\BackgroundTask\SingleScheduledBackgroundTask; use AmpProject\AmpWP\DevTools\UserAccess; use AmpProject\AmpWP\Infrastructure\Conditional; -use AmpProject\AmpWP\Services; /** * SavePostValidationEvent class. diff --git a/tests/php/src/Admin/PairedBrowsingTest.php b/tests/php/src/Admin/PairedBrowsingTest.php index 652f691053b..71475e4cacd 100644 --- a/tests/php/src/Admin/PairedBrowsingTest.php +++ b/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() */ diff --git a/tests/php/src/Admin/PolyfillsTest.php b/tests/php/src/Admin/PolyfillsTest.php index 91ad01f35fa..4706efc2f02 100644 --- a/tests/php/src/Admin/PolyfillsTest.php +++ b/tests/php/src/Admin/PolyfillsTest.php @@ -10,6 +10,7 @@ use AmpProject\AmpWP\Admin\Polyfills; 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 WP_Scripts; @@ -49,6 +50,12 @@ public function test__construct() { $this->assertInstanceOf( Delayed::class, $this->instance ); $this->assertInstanceOf( Conditional::class, $this->instance ); $this->assertInstanceOf( Registerable::class, $this->instance ); + $this->assertInstanceOf( HasRequirements::class, $this->instance ); + } + + /** @covers ::get_requirements() */ + public function test_get_requirements() { + $this->assertSame( [ 'dependency_support' ], Polyfills::get_requirements() ); } /** diff --git a/tests/php/src/Admin/ValidationCountsTest.php b/tests/php/src/Admin/ValidationCountsTest.php index 3eedaaa33c8..ea7f78dc1d9 100644 --- a/tests/php/src/Admin/ValidationCountsTest.php +++ b/tests/php/src/Admin/ValidationCountsTest.php @@ -14,6 +14,7 @@ use AmpProject\AmpWP\DevTools\UserAccess; 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\Option; @@ -51,6 +52,7 @@ public function test__construct() { $this->assertInstanceOf( Conditional::class, $this->instance ); $this->assertInstanceOf( Service::class, $this->instance ); $this->assertInstanceOf( Registerable::class, $this->instance ); + $this->assertInstanceOf( HasRequirements::class, $this->instance ); } /** @@ -62,6 +64,14 @@ public function test_get_registration_action() { self::assertEquals( 'admin_enqueue_scripts', ValidationCounts::get_registration_action() ); } + /** @covers ::get_requirements() */ + public function test_get_requirements() { + $this->assertSame( + [ 'dependency_support', 'dev_tools.user_access' ], + ValidationCounts::get_requirements() + ); + } + /** * Test ::register(). * diff --git a/tests/php/src/Fixture/DummyServiceWithRequirements.php b/tests/php/src/Fixture/DummyServiceWithRequirements.php new file mode 100644 index 00000000000..91b47e951d5 --- /dev/null +++ b/tests/php/src/Fixture/DummyServiceWithRequirements.php @@ -0,0 +1,19 @@ + List of required services. + */ + public static function get_requirements() + { + return [ 'service_a' ]; + } +} diff --git a/tests/php/src/Infrastructure/ServiceBasedPluginTest.php b/tests/php/src/Infrastructure/ServiceBasedPluginTest.php index 7c1c38467a9..3bf1b08f8d3 100644 --- a/tests/php/src/Infrastructure/ServiceBasedPluginTest.php +++ b/tests/php/src/Infrastructure/ServiceBasedPluginTest.php @@ -8,6 +8,7 @@ use AmpProject\AmpWP\Infrastructure\ServiceContainer\SimpleServiceContainer; use AmpProject\AmpWP\Tests\Fixture\DummyService; use AmpProject\AmpWP\Tests\Fixture\DummyServiceBasedPlugin; +use AmpProject\AmpWP\Tests\Fixture\DummyServiceWithRequirements; use WP_UnitTestCase; final class ServiceBasedPluginTest extends WP_UnitTestCase { @@ -151,6 +152,54 @@ static function ( $services ) { $this->assertInstanceof( DummyService::class, $container->get( 'filtered_service' ) ); } + public function test_it_registers_service_with_requirements() { + $container = new SimpleServiceContainer(); + $plugin = $this->getMockBuilder( DummyServiceBasedPlugin::class ) + ->enableOriginalConstructor() + ->setConstructorArgs( [ true, null, $container ] ) + ->setMethodsExcept( + [ + 'register', + 'register_services', + 'get_service_classes', + ] + ) + ->getMock(); + + // Throws an exception if it requires a service that has not been recognized. + $service_callback = static function () { + return [ 'filtered_service' => DummyServiceWithRequirements::class ]; + }; + + add_filter( 'services', $service_callback ); + + $this->expectExceptionMessage( 'The service ID "service_a" is not recognized and cannot be retrieved.' ); + $plugin->register(); + + remove_filter( 'services', $service_callback ); + + // Successfully registers a service that has requirements. + $service_callback = static function ( $services ) { + array_unshift( + $services, + [ 'filtered_service' => DummyServiceWithRequirements::class ] + ); + return $services; + }; + + add_filter( 'services', $service_callback ); + + $plugin->register(); + + $this->assertEquals( 4, count( $container ) ); + $this->assertTrue( $container->has( 'service_a' ) ); + $this->assertInstanceof( DummyService::class, $container->get( 'service_a' ) ); + $this->assertTrue( $container->has( 'service_b' ) ); + $this->assertInstanceof( DummyService::class, $container->get( 'service_b' ) ); + $this->assertTrue( $container->has( 'filtered_service' ) ); + $this->assertInstanceof( DummyServiceWithRequirements::class, $container->get( 'filtered_service' ) ); + } + public function test_it_generates_identifiers_as_needed() { $container = new SimpleServiceContainer(); $plugin = $this->getMockBuilder( ServiceBasedPlugin::class ) From dcc3c7164513e68d370c2025b425073a312005da Mon Sep 17 00:00:00 2001 From: Pierre Gordon Date: Tue, 27 Jul 2021 15:08:03 -0400 Subject: [PATCH 05/16] Fix lint errors --- tests/php/src/Fixture/DummyServiceWithRequirements.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/php/src/Fixture/DummyServiceWithRequirements.php b/tests/php/src/Fixture/DummyServiceWithRequirements.php index 91b47e951d5..c0953b86228 100644 --- a/tests/php/src/Fixture/DummyServiceWithRequirements.php +++ b/tests/php/src/Fixture/DummyServiceWithRequirements.php @@ -12,8 +12,7 @@ class DummyServiceWithRequirements implements Service, HasRequirements { * * @return array List of required services. */ - public static function get_requirements() - { + public static function get_requirements() { return [ 'service_a' ]; } } From 357ae8de2171c537fc40e256907f3ffca7f641d4 Mon Sep 17 00:00:00 2001 From: Pierre Gordon Date: Tue, 27 Jul 2021 16:57:03 -0400 Subject: [PATCH 06/16] No longer wait on all coverage reports --- codecov.yml | 5 ----- 1 file changed, 5 deletions(-) diff --git a/codecov.yml b/codecov.yml index a82746ffacc..f705980f67a 100644 --- a/codecov.yml +++ b/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) From c06b300712830d7410c6fe8c1d3575f856a835a8 Mon Sep 17 00:00:00 2001 From: Pierre Gordon Date: Tue, 27 Jul 2021 20:40:01 -0400 Subject: [PATCH 07/16] Use `string[]` param type Co-authored-by: Weston Ruter --- src/Admin/PairedBrowsing.php | 2 +- src/Admin/Polyfills.php | 2 +- src/Admin/ValidationCounts.php | 2 +- src/Infrastructure/HasRequirements.php | 2 +- src/Infrastructure/ServiceBasedPlugin.php | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Admin/PairedBrowsing.php b/src/Admin/PairedBrowsing.php index b38c1234e45..156ab603a4a 100644 --- a/src/Admin/PairedBrowsing.php +++ b/src/Admin/PairedBrowsing.php @@ -77,7 +77,7 @@ public static function is_needed() { /** * Get the list of service IDs required for this service to be registered. * - * @return array List of required services. + * @return string[] List of required services. */ public static function get_requirements() { return [ diff --git a/src/Admin/Polyfills.php b/src/Admin/Polyfills.php index 84e01340812..78c9e34cdd5 100644 --- a/src/Admin/Polyfills.php +++ b/src/Admin/Polyfills.php @@ -47,7 +47,7 @@ public static function is_needed() { /** * Get the list of service IDs required for this service to be registered. * - * @return array List of required services. + * @return string[] List of required services. */ public static function get_requirements() { return [ diff --git a/src/Admin/ValidationCounts.php b/src/Admin/ValidationCounts.php index 68b4408e79e..78585a88f28 100644 --- a/src/Admin/ValidationCounts.php +++ b/src/Admin/ValidationCounts.php @@ -42,7 +42,7 @@ public static function get_registration_action() { /** * Get the list of service IDs required for this service to be registered. * - * @return array List of required services. + * @return string[] List of required services. */ public static function get_requirements() { return [ diff --git a/src/Infrastructure/HasRequirements.php b/src/Infrastructure/HasRequirements.php index 94a11f98e45..e85f2f7ab3e 100644 --- a/src/Infrastructure/HasRequirements.php +++ b/src/Infrastructure/HasRequirements.php @@ -21,7 +21,7 @@ interface HasRequirements { /** * Get the list of service IDs required for this service to be registered. * - * @return array List of required services. + * @return string[] List of required services. */ public static function get_requirements(); } diff --git a/src/Infrastructure/ServiceBasedPlugin.php b/src/Infrastructure/ServiceBasedPlugin.php index 2043c48e32f..16aeade08ba 100644 --- a/src/Infrastructure/ServiceBasedPlugin.php +++ b/src/Infrastructure/ServiceBasedPlugin.php @@ -240,7 +240,7 @@ function () use ( $id, $class ) { * Determine if the requirements for a service to be registered are met. * * @param HasRequirements $class Service with requirements. - * @param array $service_ids List of service IDs to be registered. + * @param string[] $service_ids List of service IDs to be registered. * * @throws InvalidService If the required service is not recognized. * From 1f1feee13b288c2b68eca7060714ea4899c4a76f Mon Sep 17 00:00:00 2001 From: Pierre Gordon Date: Thu, 29 Jul 2021 16:37:07 -0400 Subject: [PATCH 08/16] Remove obsolete comment --- src/AmpWpPlugin.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/AmpWpPlugin.php b/src/AmpWpPlugin.php index ee13dd28b46..c2dd276f58e 100644 --- a/src/AmpWpPlugin.php +++ b/src/AmpWpPlugin.php @@ -84,7 +84,7 @@ final class AmpWpPlugin extends ServiceBasedPlugin { '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. + 'dependency_support' => DependencySupport::class, 'dev_tools.block_sources' => DevTools\BlockSources::class, 'dev_tools.callback_reflection' => DevTools\CallbackReflection::class, 'dev_tools.error_page' => DevTools\ErrorPage::class, From 400f8d60acfc5697c8aeace6e5d61bdf4f516172 Mon Sep 17 00:00:00 2001 From: Alain Schlesser Date: Mon, 2 Aug 2021 14:45:54 +0200 Subject: [PATCH 09/16] Add more robust mechanism for postponing service registration with missing requirements --- src/Infrastructure/ServiceBasedPlugin.php | 104 +++++++++++++++++----- 1 file changed, 84 insertions(+), 20 deletions(-) diff --git a/src/Infrastructure/ServiceBasedPlugin.php b/src/Infrastructure/ServiceBasedPlugin.php index 16aeade08ba..df4b8477bad 100644 --- a/src/Infrastructure/ServiceBasedPlugin.php +++ b/src/Infrastructure/ServiceBasedPlugin.php @@ -196,18 +196,9 @@ public function register_services() { if ( is_a( $class, HasRequirements::class, true ) && - ! $this->requirements_are_met( $class, array_keys( $services ) ) + ! $this->requirements_are_met( $id, $class, $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 ); - + next( $services ); continue; } @@ -215,11 +206,11 @@ public function register_services() { if ( is_a( $class, Delayed::class, true ) ) { $registration_action = $class::get_registration_action(); - if ( did_action( $registration_action ) ) { + if ( \did_action( $registration_action ) ) { $this->register_service( $id, $class ); } else { \add_action( - $class::get_registration_action(), + $registration_action, function () use ( $id, $class ) { $this->register_service( $id, $class ); } @@ -239,28 +230,101 @@ function () use ( $id, $class ) { /** * 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. + * This also hooks up another check in the future to the registration action(s) of its requirements. + * + * @param string $id Service ID of the service with requirements. + * @param string $class Service FQCN of the service with requirements. + * @param string[] &$services List of services 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 ) { + protected function requirements_are_met( $id, $class, &$services ) { + $missing_requirements = $this->collect_missing_requirements( $class, $services ); + + if ( empty( $missing_requirements ) ) { + return true; + } + + foreach ( $missing_requirements as $missing_requirement ) { + if ( is_a( $missing_requirement, Delayed::class, true ) ) { + $action = $missing_requirement::get_registration_action(); + + if ( \did_action( $action ) ) { + continue; + } + + /* + * The current service depends on another service that is Delayed and hasn't been registered yet + * and for which the registration action has not yet passed. + * + * Therefore, we postpone the registration of the current service up until the requirement's + * action has passed. + * + * We don't register the service right away, though, we will first check whether at that point, + * the requirements have been met. + * + * Note that badly configured requirements can lead to services that will never register at all. + */ + + \add_action( + $action, + function () use ( $id, $class, $missing_requirements, $services ) { + if ( ! $this->requirements_are_met( $id, $class, $services ) ) { + return; + } + + $this->register_service( $id, $class ); + }, + PHP_INT_MAX + ); + + return false; + } + } + + /* + * The registration actions from all of the requirements were already processed. This means that the missing + * requirement(s) are about to be registered, they just weren't encountered yet while traversing the services + * map. Therefore, we skip registration for now and move this particular service to the end of the service map. + */ + unset( $services[ $id ] ); + $services[ $id ] = $class; + + return false; + } + + /** + * Collect the list of missing requirements for a service which has requirements. + * + * @param string $class Service FQCN of the service with requirements. + * @param string[] $services List of services to register. + * + * @throws InvalidService If the required service is not recognized. + * + * @return string[] List of missing requirements as a $service_id => $service_class mapping. + */ + protected function collect_missing_requirements( $class, $services ) + { $requirements = $class::get_requirements(); + $missing = []; + foreach ( $requirements as $requirement ) { // Bail if it requires a service that is not recognized. - if ( ! in_array( $requirement, $service_ids, true ) ) { + if ( ! array_key_exists( $requirement, $services ) ) { throw InvalidService::from_service_id( $requirement ); } - if ( ! $this->get_container()->has( $requirement ) ) { - return false; + if ( $this->get_container()->has( $requirement ) ) { + continue; } + + $missing[ $requirement ] = $services[ $requirement ]; } - return true; + return $missing; } /** From 7e1ee1642046b63b831cacf1585af6301d702da4 Mon Sep 17 00:00:00 2001 From: Alain Schlesser Date: Mon, 2 Aug 2021 15:29:32 +0200 Subject: [PATCH 10/16] Add delayed requirements test --- .../php/src/Fixture/DummyServiceWithDelay.php | 19 +++++ .../Infrastructure/ServiceBasedPluginTest.php | 76 ++++++++++++++++--- 2 files changed, 86 insertions(+), 9 deletions(-) create mode 100644 tests/php/src/Fixture/DummyServiceWithDelay.php diff --git a/tests/php/src/Fixture/DummyServiceWithDelay.php b/tests/php/src/Fixture/DummyServiceWithDelay.php new file mode 100644 index 00000000000..9fe8628d5d2 --- /dev/null +++ b/tests/php/src/Fixture/DummyServiceWithDelay.php @@ -0,0 +1,19 @@ +getMock(); - // Throws an exception if it requires a service that has not been recognized. - $service_callback = static function () { - return [ 'filtered_service' => DummyServiceWithRequirements::class ]; + $service_callback = static function ( $services ) { + array_unshift( + $services, + [ 'service_with_requirements' => DummyServiceWithRequirements::class ] + ); + return $services; }; add_filter( 'services', $service_callback ); - $this->expectExceptionMessage( 'The service ID "service_a" is not recognized and cannot be retrieved.' ); $plugin->register(); - remove_filter( 'services', $service_callback ); + $this->assertEquals( 4, count( $container ) ); + $this->assertTrue( $container->has( 'service_a' ) ); + $this->assertInstanceof( DummyService::class, $container->get( 'service_a' ) ); + $this->assertTrue( $container->has( 'service_b' ) ); + $this->assertInstanceof( DummyService::class, $container->get( 'service_b' ) ); + $this->assertTrue( $container->has( 'filtered_service' ) ); + $this->assertInstanceof( DummyServiceWithRequirements::class, $container->get( 'service_with_requirements' ) ); + } + + public function test_it_handles_delays_for_requirements() { + $container = new SimpleServiceContainer(); + $plugin = $this->getMockBuilder( DummyServiceBasedPlugin::class ) + ->enableOriginalConstructor() + ->setConstructorArgs( [ true, null, $container ] ) + ->setMethodsExcept( + [ + 'register', + 'register_services', + 'get_service_classes', + ] + ) + ->getMock(); - // Successfully registers a service that has requirements. $service_callback = static function ( $services ) { array_unshift( $services, - [ 'filtered_service' => DummyServiceWithRequirements::class ] + [ + 'service_a' => DummyServiceWithDelay::class, + 'service_with_requirements' => DummyServiceWithRequirements::class, + ] ); return $services; }; @@ -191,13 +217,45 @@ public function test_it_registers_service_with_requirements() { $plugin->register(); + $this->assertEquals( 1, count( $container ) ); + $this->assertFalse( $container->has( 'service_a' ) ); + $this->assertTrue( $container->has( 'service_b' ) ); + $this->assertFalse( $container->has( 'service_with_requirements' ) ); + $this->assertInstanceof( DummyService::class, $container->get( 'service_b' ) ); + + do_action( 'some_action' ); + $this->assertEquals( 4, count( $container ) ); $this->assertTrue( $container->has( 'service_a' ) ); $this->assertInstanceof( DummyService::class, $container->get( 'service_a' ) ); $this->assertTrue( $container->has( 'service_b' ) ); $this->assertInstanceof( DummyService::class, $container->get( 'service_b' ) ); - $this->assertTrue( $container->has( 'filtered_service' ) ); - $this->assertInstanceof( DummyServiceWithRequirements::class, $container->get( 'filtered_service' ) ); + $this->assertTrue( $container->has( 'service_with_requirements' ) ); + $this->assertInstanceof( DummyServiceWithRequirements::class, $container->get( 'service_with_requirements' ) ); + } + + public function test_it_throws_an_exception_if_unrecognized_service_is_required() { + $container = new SimpleServiceContainer(); + $plugin = $this->getMockBuilder( DummyServiceBasedPlugin::class ) + ->enableOriginalConstructor() + ->setConstructorArgs( [ true, null, $container ] ) + ->setMethodsExcept( + [ + 'register', + 'register_services', + 'get_service_classes', + ] + ) + ->getMock(); + + $service_callback = static function () { + return [ 'service_with_requirements' => DummyServiceWithRequirements::class ]; + }; + + add_filter( 'services', $service_callback ); + + $this->expectExceptionMessage( 'The service ID "service_a" is not recognized and cannot be retrieved.' ); + $plugin->register(); } public function test_it_generates_identifiers_as_needed() { From bca199fec0a0f24e1c75866ad4403d4505c87884 Mon Sep 17 00:00:00 2001 From: Alain Schlesser Date: Mon, 2 Aug 2021 16:12:57 +0200 Subject: [PATCH 11/16] Fix PHPCS issues --- src/Infrastructure/ServiceBasedPlugin.php | 9 ++--- .../php/src/Fixture/DummyServiceWithDelay.php | 3 +- .../Infrastructure/ServiceBasedPluginTest.php | 40 +++++++++---------- 3 files changed, 25 insertions(+), 27 deletions(-) diff --git a/src/Infrastructure/ServiceBasedPlugin.php b/src/Infrastructure/ServiceBasedPlugin.php index df4b8477bad..b801319197a 100644 --- a/src/Infrastructure/ServiceBasedPlugin.php +++ b/src/Infrastructure/ServiceBasedPlugin.php @@ -232,9 +232,9 @@ function () use ( $id, $class ) { * * This also hooks up another check in the future to the registration action(s) of its requirements. * - * @param string $id Service ID of the service with requirements. - * @param string $class Service FQCN of the service with requirements. - * @param string[] &$services List of services to be registered. + * @param string $id Service ID of the service with requirements. + * @param string $class Service FQCN of the service with requirements. + * @param string[] $services List of services to be registered. * * @throws InvalidService If the required service is not recognized. * @@ -305,8 +305,7 @@ function () use ( $id, $class, $missing_requirements, $services ) { * * @return string[] List of missing requirements as a $service_id => $service_class mapping. */ - protected function collect_missing_requirements( $class, $services ) - { + protected function collect_missing_requirements( $class, $services ) { $requirements = $class::get_requirements(); $missing = []; diff --git a/tests/php/src/Fixture/DummyServiceWithDelay.php b/tests/php/src/Fixture/DummyServiceWithDelay.php index 9fe8628d5d2..ea3e3bc5598 100644 --- a/tests/php/src/Fixture/DummyServiceWithDelay.php +++ b/tests/php/src/Fixture/DummyServiceWithDelay.php @@ -12,8 +12,7 @@ class DummyServiceWithDelay implements Service, Delayed { * * @return string Registration action to use. */ - public static function get_registration_action() - { + public static function get_registration_action() { return 'some_action'; } } diff --git a/tests/php/src/Infrastructure/ServiceBasedPluginTest.php b/tests/php/src/Infrastructure/ServiceBasedPluginTest.php index fe63e6e6b91..6aa7b9782ca 100644 --- a/tests/php/src/Infrastructure/ServiceBasedPluginTest.php +++ b/tests/php/src/Infrastructure/ServiceBasedPluginTest.php @@ -191,16 +191,16 @@ public function test_it_registers_service_with_requirements() { public function test_it_handles_delays_for_requirements() { $container = new SimpleServiceContainer(); $plugin = $this->getMockBuilder( DummyServiceBasedPlugin::class ) - ->enableOriginalConstructor() - ->setConstructorArgs( [ true, null, $container ] ) - ->setMethodsExcept( - [ - 'register', - 'register_services', - 'get_service_classes', - ] - ) - ->getMock(); + ->enableOriginalConstructor() + ->setConstructorArgs( [ true, null, $container ] ) + ->setMethodsExcept( + [ + 'register', + 'register_services', + 'get_service_classes', + ] + ) + ->getMock(); $service_callback = static function ( $services ) { array_unshift( @@ -237,16 +237,16 @@ public function test_it_handles_delays_for_requirements() { public function test_it_throws_an_exception_if_unrecognized_service_is_required() { $container = new SimpleServiceContainer(); $plugin = $this->getMockBuilder( DummyServiceBasedPlugin::class ) - ->enableOriginalConstructor() - ->setConstructorArgs( [ true, null, $container ] ) - ->setMethodsExcept( - [ - 'register', - 'register_services', - 'get_service_classes', - ] - ) - ->getMock(); + ->enableOriginalConstructor() + ->setConstructorArgs( [ true, null, $container ] ) + ->setMethodsExcept( + [ + 'register', + 'register_services', + 'get_service_classes', + ] + ) + ->getMock(); $service_callback = static function () { return [ 'service_with_requirements' => DummyServiceWithRequirements::class ]; From 614f35b40290c77b34eb091c851648fff41b3eed Mon Sep 17 00:00:00 2001 From: Alain Schlesser Date: Mon, 2 Aug 2021 16:17:56 +0200 Subject: [PATCH 12/16] Fix broken tests --- tests/php/src/Infrastructure/ServiceBasedPluginTest.php | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tests/php/src/Infrastructure/ServiceBasedPluginTest.php b/tests/php/src/Infrastructure/ServiceBasedPluginTest.php index 6aa7b9782ca..e3ed1c26934 100644 --- a/tests/php/src/Infrastructure/ServiceBasedPluginTest.php +++ b/tests/php/src/Infrastructure/ServiceBasedPluginTest.php @@ -168,11 +168,10 @@ public function test_it_registers_service_with_requirements() { ->getMock(); $service_callback = static function ( $services ) { - array_unshift( + return array_merge( $services, [ 'service_with_requirements' => DummyServiceWithRequirements::class ] ); - return $services; }; add_filter( 'services', $service_callback ); @@ -184,7 +183,7 @@ public function test_it_registers_service_with_requirements() { $this->assertInstanceof( DummyService::class, $container->get( 'service_a' ) ); $this->assertTrue( $container->has( 'service_b' ) ); $this->assertInstanceof( DummyService::class, $container->get( 'service_b' ) ); - $this->assertTrue( $container->has( 'filtered_service' ) ); + $this->assertTrue( $container->has( 'service_with_requirements' ) ); $this->assertInstanceof( DummyServiceWithRequirements::class, $container->get( 'service_with_requirements' ) ); } @@ -203,14 +202,13 @@ public function test_it_handles_delays_for_requirements() { ->getMock(); $service_callback = static function ( $services ) { - array_unshift( + return array_merge( $services, [ 'service_a' => DummyServiceWithDelay::class, 'service_with_requirements' => DummyServiceWithRequirements::class, ] ); - return $services; }; add_filter( 'services', $service_callback ); From ab3452599e9f27460afd20f39a942db3da4de9a4 Mon Sep 17 00:00:00 2001 From: Alain Schlesser Date: Mon, 2 Aug 2021 16:22:36 +0200 Subject: [PATCH 13/16] Remove unused argument from callback --- src/Infrastructure/ServiceBasedPlugin.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Infrastructure/ServiceBasedPlugin.php b/src/Infrastructure/ServiceBasedPlugin.php index b801319197a..e0eb63ce742 100644 --- a/src/Infrastructure/ServiceBasedPlugin.php +++ b/src/Infrastructure/ServiceBasedPlugin.php @@ -270,7 +270,7 @@ protected function requirements_are_met( $id, $class, &$services ) { \add_action( $action, - function () use ( $id, $class, $missing_requirements, $services ) { + function () use ( $id, $class, $services ) { if ( ! $this->requirements_are_met( $id, $class, $services ) ) { return; } From 14af92b0fe56ab08435b1d57a18d73a0ca5e2947 Mon Sep 17 00:00:00 2001 From: Alain Schlesser Date: Mon, 2 Aug 2021 16:44:42 +0200 Subject: [PATCH 14/16] Mock less of the plugin for requirements tests --- tests/php/src/Infrastructure/ServiceBasedPluginTest.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/php/src/Infrastructure/ServiceBasedPluginTest.php b/tests/php/src/Infrastructure/ServiceBasedPluginTest.php index e3ed1c26934..24e4e601b1f 100644 --- a/tests/php/src/Infrastructure/ServiceBasedPluginTest.php +++ b/tests/php/src/Infrastructure/ServiceBasedPluginTest.php @@ -160,8 +160,11 @@ public function test_it_registers_service_with_requirements() { ->setConstructorArgs( [ true, null, $container ] ) ->setMethodsExcept( [ + 'collect_missing_requirements', 'register', 'register_services', + 'requirements_are_met', + 'get_container', 'get_service_classes', ] ) @@ -194,8 +197,11 @@ public function test_it_handles_delays_for_requirements() { ->setConstructorArgs( [ true, null, $container ] ) ->setMethodsExcept( [ + 'collect_missing_requirements', 'register', 'register_services', + 'requirements_are_met', + 'get_container', 'get_service_classes', ] ) From 9ef514e51cb8524f4b509c066c96fd79f268d01b Mon Sep 17 00:00:00 2001 From: Alain Schlesser Date: Mon, 2 Aug 2021 18:06:42 +0200 Subject: [PATCH 15/16] Fix test --- tests/php/src/Infrastructure/ServiceBasedPluginTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/php/src/Infrastructure/ServiceBasedPluginTest.php b/tests/php/src/Infrastructure/ServiceBasedPluginTest.php index 24e4e601b1f..34a4183a91a 100644 --- a/tests/php/src/Infrastructure/ServiceBasedPluginTest.php +++ b/tests/php/src/Infrastructure/ServiceBasedPluginTest.php @@ -221,7 +221,7 @@ public function test_it_handles_delays_for_requirements() { $plugin->register(); - $this->assertEquals( 1, count( $container ) ); + $this->assertEquals( 2, count( $container ) ); $this->assertFalse( $container->has( 'service_a' ) ); $this->assertTrue( $container->has( 'service_b' ) ); $this->assertFalse( $container->has( 'service_with_requirements' ) ); @@ -231,7 +231,7 @@ public function test_it_handles_delays_for_requirements() { $this->assertEquals( 4, count( $container ) ); $this->assertTrue( $container->has( 'service_a' ) ); - $this->assertInstanceof( DummyService::class, $container->get( 'service_a' ) ); + $this->assertInstanceof( DummyServiceWithDelay::class, $container->get( 'service_a' ) ); $this->assertTrue( $container->has( 'service_b' ) ); $this->assertInstanceof( DummyService::class, $container->get( 'service_b' ) ); $this->assertTrue( $container->has( 'service_with_requirements' ) ); From 953a039fc480720cfd8f83b415536ba702cc179e Mon Sep 17 00:00:00 2001 From: Weston Ruter Date: Wed, 4 Aug 2021 16:45:01 -0700 Subject: [PATCH 16/16] Remove invalid import --- src/Infrastructure/ServiceBasedPlugin.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Infrastructure/ServiceBasedPlugin.php b/src/Infrastructure/ServiceBasedPlugin.php index e0eb63ce742..d844d6d287f 100644 --- a/src/Infrastructure/ServiceBasedPlugin.php +++ b/src/Infrastructure/ServiceBasedPlugin.php @@ -7,7 +7,6 @@ namespace AmpProject\AmpWP\Infrastructure; -use AmpProject\AmpWP\Exception\InvalidRequirement; use AmpProject\AmpWP\Exception\InvalidService; use AmpProject\AmpWP\Infrastructure\ServiceContainer\LazilyInstantiatedService; use WP_CLI;