From 7b4db96a8b1e4c1d54efc48ba9035c79420c5a81 Mon Sep 17 00:00:00 2001 From: Pascal Birchler Date: Thu, 29 Jul 2021 21:00:05 +0200 Subject: [PATCH 01/41] Implement dependency resolution for services --- includes/Infrastructure/HasRequirements.php | 43 +++++++++ .../Infrastructure/ServiceBasedPlugin.php | 93 +++++++++++++++---- .../Fixture/DummyServiceWithRequirements.php | 33 +++++++ .../Infrastructure/ServiceBasedPluginTest.php | 69 ++++++++++++-- 4 files changed, 208 insertions(+), 30 deletions(-) create mode 100644 includes/Infrastructure/HasRequirements.php create mode 100644 tests/phpunit/includes/Fixture/DummyServiceWithRequirements.php diff --git a/includes/Infrastructure/HasRequirements.php b/includes/Infrastructure/HasRequirements.php new file mode 100644 index 000000000000..1c31bd536eef --- /dev/null +++ b/includes/Infrastructure/HasRequirements.php @@ -0,0 +1,43 @@ +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 ); - }, - $class::get_registration_action_priority() - ); - + next( $services ); continue; } $this->register_service( $id, $class ); + + next( $services ); + } + } + + /** + * Determine if the requirements for a service to be registered are met. + * + * @since 1.10.0 + * + * @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 ): bool { + $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; } /** @@ -396,7 +449,7 @@ function () use ( $class ) { * @param Injector $injector Injector instance to configure. * @return Injector Configured injector instance. */ - protected function configure_injector( Injector $injector ) { + protected function configure_injector( Injector $injector ): Injector { $bindings = $this->get_bindings(); $shared_instances = $this->get_shared_instances(); $arguments = $this->get_arguments(); @@ -412,7 +465,7 @@ protected function configure_injector( Injector $injector ) { * implementation bindings. Both * should be FQCNs. */ - $bindings = (array) \apply_filters( + $bindings = (array) apply_filters( static::HOOK_PREFIX . static::BINDINGS_FILTER, $bindings ); @@ -428,7 +481,7 @@ protected function configure_injector( Injector $injector ) { * array maps argument names to * values. */ - $arguments = (array) \apply_filters( + $arguments = (array) apply_filters( static::HOOK_PREFIX . static::ARGUMENTS_FILTER, $arguments ); @@ -442,7 +495,7 @@ protected function configure_injector( Injector $injector ) { * @param array $shared_instances Array of FQCNs to turn * into shared objects. */ - $shared_instances = (array) \apply_filters( + $shared_instances = (array) apply_filters( static::HOOK_PREFIX . static::SHARED_INSTANCES_FILTER, $shared_instances ); @@ -456,7 +509,7 @@ protected function configure_injector( Injector $injector ) { * @param array $delegations Associative array of class => * callable mappings. */ - $delegations = (array) \apply_filters( + $delegations = (array) apply_filters( static::HOOK_PREFIX . static::DELEGATIONS_FILTER, $delegations ); diff --git a/tests/phpunit/includes/Fixture/DummyServiceWithRequirements.php b/tests/phpunit/includes/Fixture/DummyServiceWithRequirements.php new file mode 100644 index 000000000000..26f137424f58 --- /dev/null +++ b/tests/phpunit/includes/Fixture/DummyServiceWithRequirements.php @@ -0,0 +1,33 @@ +setMethodsExcept( [ 'register', 'register_services' ] ) ->getMock(); - $this->assertEquals( 0, count( $container ) ); + $this->assertCount( 0, $container ); $plugin->register(); - $this->assertEquals( 1, count( $container ) ); + $this->assertCount( 1, $container ); $this->assertTrue( $container->has( 'injector' ) ); $this->assertInstanceof( Injector::class, $container->get( 'injector' ) ); } @@ -71,11 +72,11 @@ public function test_it_registers_default_services() { ) ->getMock(); - $this->assertEquals( 0, count( $container ) ); + $this->assertCount( 0, $container ); $plugin->register(); - $this->assertEquals( 3, count( $container ) ); + $this->assertCount( 3, $container ); $this->assertTrue( $container->has( 'service_a' ) ); $this->assertInstanceof( DummyService::class, $container->get( 'service_a' ) ); $this->assertTrue( $container->has( 'service_b' ) ); @@ -107,7 +108,7 @@ static function () { $plugin->register(); - $this->assertEquals( 2, count( $container ) ); + $this->assertCount( 2, $container ); $this->assertTrue( $container->has( 'filtered_service' ) ); $this->assertInstanceof( DummyService::class, $container->get( 'filtered_service' ) ); $this->assertfalse( $container->has( 'service_a' ) ); @@ -142,7 +143,7 @@ static function ( $services ) { $plugin->register(); - $this->assertEquals( 4, count( $container ) ); + $this->assertCount( 4, $container ); $this->assertTrue( $container->has( 'service_a' ) ); $this->assertInstanceof( DummyService::class, $container->get( 'service_a' ) ); $this->assertTrue( $container->has( 'service_b' ) ); @@ -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->assertCount( 4, $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 ) @@ -168,7 +217,7 @@ static function () { $plugin->register(); - $this->assertEquals( 2, count( $container ) ); + $this->assertCount( 2, $container ); $this->assertTrue( $container->has( 'dummy_service' ) ); $this->assertInstanceof( DummyService::class, $container->get( 'dummy_service' ) ); } @@ -192,7 +241,7 @@ static function () { $plugin->register(); - $this->assertEquals( 1, count( $container ) ); + $this->assertCount( 1, $container ); $this->assertFalse( $container->has( 'dummy_service' ) ); } @@ -219,7 +268,7 @@ static function () { $plugin->register(); - $this->assertEquals( 3, count( $container ) ); + $this->assertCount( 3, $container ); $this->assertTrue( $container->has( 'service_a' ) ); $this->assertInstanceof( DummyService::class, $container->get( 'service_a' ) ); $this->assertTrue( $container->has( 'service_b' ) ); @@ -249,7 +298,7 @@ static function () { $plugin->register(); - $this->assertEquals( 3, count( $container ) ); + $this->assertCount( 3, $container ); $this->assertTrue( $container->has( 'service_a' ) ); $this->assertInstanceof( DummyService::class, $container->get( 'service_a' ) ); $this->assertTrue( $container->has( 'service_b' ) ); From bc5472bcd2d320b6db6d179fca67f416904930c9 Mon Sep 17 00:00:00 2001 From: Pascal Birchler Date: Thu, 29 Jul 2021 21:15:52 +0200 Subject: [PATCH 02/41] Fix incorrect test --- includes/Experiments.php | 2 +- tests/phpunit/tests/Experiments.php | 7 +++++-- tests/phpunit/tests/Settings.php | 1 + 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/includes/Experiments.php b/includes/Experiments.php index 936221b40ffa..e1974f8aa532 100644 --- a/includes/Experiments.php +++ b/includes/Experiments.php @@ -493,7 +493,7 @@ public function is_experiment_enabled( string $name ): bool { * * @return array List of all enabled experiments. */ - public function get_enabled_experiments() { + public function get_enabled_experiments(): array { $experiments = array_filter( wp_list_pluck( $this->get_experiments(), 'name' ), [ $this, 'is_experiment_enabled' ] diff --git a/tests/phpunit/tests/Experiments.php b/tests/phpunit/tests/Experiments.php index f0b0fcb5a02c..65dd0af271f4 100644 --- a/tests/phpunit/tests/Experiments.php +++ b/tests/phpunit/tests/Experiments.php @@ -84,11 +84,14 @@ public function test_add_menu_page() { * @covers ::initialize_settings */ public function test_initialize_settings() { + global $wp_settings_fields, $wp_settings_sections; + $experiments = new \Google\Web_Stories\Experiments(); $experiments->initialize_settings(); - $options = get_registered_settings(); - $this->assertArrayHasKey( \Google\Web_Stories\Settings::SETTING_NAME_EXPERIMENTS, $options ); + $this->assertArrayHasKey( \Google\Web_Stories\Experiments::PAGE_NAME, $wp_settings_fields ); + $this->assertArrayHasKey( \Google\Web_Stories\Experiments::PAGE_NAME, $wp_settings_sections ); + $this->assertArrayHasKey( 'web_stories_experiments_section', $wp_settings_sections[ \Google\Web_Stories\Experiments::PAGE_NAME ] ); } /** diff --git a/tests/phpunit/tests/Settings.php b/tests/phpunit/tests/Settings.php index 276b55fb5a0b..ed1a7c0e6be2 100644 --- a/tests/phpunit/tests/Settings.php +++ b/tests/phpunit/tests/Settings.php @@ -44,5 +44,6 @@ public function test_register_settings() { $this->assertArrayHasKey( $settings::SETTING_NAME_AD_MANAGER_SLOT_ID, $options ); $this->assertArrayHasKey( $settings::SETTING_NAME_ADSENSE_PUBLISHER_ID, $options ); $this->assertArrayHasKey( $settings::SETTING_NAME_ADSENSE_SLOT_ID, $options ); + $this->assertArrayHasKey( $settings::SETTING_NAME_EXPERIMENTS, $options ); } } From b6421cb41e15df63d216558776f23c7a0caf9cff Mon Sep 17 00:00:00 2001 From: Pascal Birchler Date: Fri, 30 Jul 2021 10:34:53 +0200 Subject: [PATCH 03/41] LInt fix --- includes/Infrastructure/ServiceBasedPlugin.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/includes/Infrastructure/ServiceBasedPlugin.php b/includes/Infrastructure/ServiceBasedPlugin.php index a9f0caff0382..b5974226f3ac 100644 --- a/includes/Infrastructure/ServiceBasedPlugin.php +++ b/includes/Infrastructure/ServiceBasedPlugin.php @@ -268,8 +268,8 @@ function () use ( $id, $class ) { * * @since 1.10.0 * - * @param HasRequirements $class Service with requirements. - * @param string[] $service_ids List of service IDs to be registered. + * @param class-string|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. * From 45cacb2f1c2c73bd7ba69fe2dab4f1f438da6c06 Mon Sep 17 00:00:00 2001 From: Pascal Birchler Date: Fri, 30 Jul 2021 17:16:22 +0200 Subject: [PATCH 04/41] Make experiment toggling more robust Borrowed from #7748 --- bin/local-env/install-wordpress.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/bin/local-env/install-wordpress.sh b/bin/local-env/install-wordpress.sh index 128385616480..73e50733bc15 100755 --- a/bin/local-env/install-wordpress.sh +++ b/bin/local-env/install-wordpress.sh @@ -209,6 +209,9 @@ wp media import /var/www/html/wp-content/e2e-assets/example-1.jpg --quiet wp media import /var/www/html/wp-content/e2e-assets/example-2.jpg --quiet wp media import /var/www/html/wp-content/e2e-assets/example-3.png --quiet +# Ensures that the patch command below always works. +wp option update web_stories_experiments '{}' --format=json + wp option patch insert web_stories_experiments enableSVG 1 wp media import /var/www/html/wp-content/e2e-assets/video-play.svg wp option patch insert web_stories_experiments enableSVG 0 From f8a3a1b1708b6f47d722f8cdd575a1ccbe31530a Mon Sep 17 00:00:00 2001 From: Pascal Birchler Date: Mon, 2 Aug 2021 13:04:37 +0200 Subject: [PATCH 05/41] Fix `Settings::register()` --- includes/Settings.php | 35 +++++++++++--------------------- tests/phpunit/tests/Settings.php | 10 --------- 2 files changed, 12 insertions(+), 33 deletions(-) diff --git a/includes/Settings.php b/includes/Settings.php index d617b3c818d2..43ce62e01310 100644 --- a/includes/Settings.php +++ b/includes/Settings.php @@ -109,28 +109,6 @@ class Settings extends Service_Base { */ const SETTING_NAME_VIDEO_CACHE = 'web_stories_video_cache'; - /** - * Initializes the Settings logic. - * - * @since 1.0.0 - * - * @return void - */ - public function register() { - add_action( 'init', [ $this, 'register_settings' ] ); - } - - /** - * Get the action priority to use for registering the service. - * - * @since 1.6.0 - * - * @return int Registration action priority to use. - */ - public static function get_registration_action_priority(): int { - return 5; - } - /** * Register settings. * @@ -140,7 +118,7 @@ public static function get_registration_action_priority(): int { * * @return void */ - public function register_settings() { + public function register() { register_setting( self::SETTING_GROUP, self::SETTING_NAME_TRACKING_ID, @@ -251,4 +229,15 @@ public function register_settings() { ] ); } + + /** + * Get the action priority to use for registering the service. + * + * @since 1.6.0 + * + * @return int Registration action priority to use. + */ + public static function get_registration_action_priority(): int { + return 5; + } } diff --git a/tests/phpunit/tests/Settings.php b/tests/phpunit/tests/Settings.php index ed1a7c0e6be2..ef62ac2bb8b8 100644 --- a/tests/phpunit/tests/Settings.php +++ b/tests/phpunit/tests/Settings.php @@ -28,16 +28,6 @@ public function test_register() { $settings = new \Google\Web_Stories\Settings(); $settings->register(); - $this->assertSame( 10, has_action( 'init', [ $settings, 'register_settings' ] ) ); - } - - /** - * @covers ::register_settings - */ - public function test_register_settings() { - $settings = new \Google\Web_Stories\Settings(); - $settings->register_settings(); - $options = get_registered_settings(); $this->assertArrayHasKey( $settings::SETTING_NAME_TRACKING_ID, $options ); $this->assertArrayHasKey( $settings::SETTING_NAME_AD_NETWORK, $options ); From 2127db66ef21d9f18ffc8be89e223acc0f9403e0 Mon Sep 17 00:00:00 2001 From: Pascal Birchler Date: Thu, 5 Aug 2021 00:42:13 +0200 Subject: [PATCH 06/41] Add some types --- .../Injector/InjectionChain.php | 12 ++--- .../Injector/SimpleInjector.php | 48 +++++++++---------- includes/Infrastructure/Plugin.php | 2 +- .../Infrastructure/ServiceBasedPlugin.php | 2 +- includes/Infrastructure/ServiceContainer.php | 4 +- .../LazilyInstantiatedService.php | 2 +- .../SimpleServiceContainer.php | 4 +- 7 files changed, 37 insertions(+), 37 deletions(-) diff --git a/includes/Infrastructure/Injector/InjectionChain.php b/includes/Infrastructure/Injector/InjectionChain.php index 96cc2424eb75..4060d0edfe97 100644 --- a/includes/Infrastructure/Injector/InjectionChain.php +++ b/includes/Infrastructure/Injector/InjectionChain.php @@ -53,7 +53,7 @@ final class InjectionChain { * @param string $class Class to add to injection chain. * @return self Modified injection chain. */ - public function add_to_chain( $class ) { + public function add_to_chain( $class ): self { $new_chain = clone $this; $new_chain->chain[] = $class; @@ -68,7 +68,7 @@ public function add_to_chain( $class ) { * @param string $resolution Resolution to add. * @return self Modified injection chain. */ - public function add_resolution( $resolution ) { + public function add_resolution( $resolution ): self { $new_chain = clone $this; $new_chain->resolutions[ $resolution ] = true; @@ -83,7 +83,7 @@ public function add_resolution( $resolution ) { * @return string Last class pushed to the injection chain. * @throws LogicException If the injection chain is accessed too early. */ - public function get_class() { + public function get_class(): string { if ( empty( $this->chain ) ) { throw new LogicException( 'Access to injection chain before any resolution was made.' @@ -100,7 +100,7 @@ public function get_class() { * * @return array Chain of injections. */ - public function get_chain() { + public function get_chain(): array { return \array_reverse( $this->chain ); } @@ -112,7 +112,7 @@ public function get_chain() { * @param string $resolution Resolution to check for. * @return bool Whether the resolution was found. */ - public function has_resolution( $resolution ) { + public function has_resolution( $resolution ): bool { return \array_key_exists( $resolution, $this->resolutions ); } @@ -124,7 +124,7 @@ public function has_resolution( $resolution ) { * @param string $class Class to check. * @return bool Whether the given class is already part of the chain. */ - public function is_in_chain( $class ) { + public function is_in_chain( $class ): bool { return in_array( $class, $this->chain, true ); } } diff --git a/includes/Infrastructure/Injector/SimpleInjector.php b/includes/Infrastructure/Injector/SimpleInjector.php index 9b1a437bd282..49ff15ce238c 100644 --- a/includes/Infrastructure/Injector/SimpleInjector.php +++ b/includes/Infrastructure/Injector/SimpleInjector.php @@ -24,6 +24,8 @@ use ReflectionClass; use ReflectionNamedType; use ReflectionParameter; +use function array_key_exists; +use function array_map; /** * A simplified implementation of a dependency injector. @@ -85,9 +87,7 @@ final class SimpleInjector implements Injector { * @param Instantiator|null $instantiator Optional. Instantiator to use. */ public function __construct( Instantiator $instantiator = null ) { - $this->instantiator = null !== $instantiator - ? $instantiator - : new FallbackInstantiator(); + $this->instantiator = $instantiator ?? new FallbackInstantiator(); } /** @@ -130,7 +130,7 @@ public function make( $interface_or_class, $arguments = [] ) { $object = $this->instantiator->instantiate( $class, $dependencies ); } - if ( \array_key_exists( $class, $this->shared_instances ) ) { + if ( array_key_exists( $class, $this->shared_instances ) ) { $this->shared_instances[ $class ] = $object; } @@ -149,7 +149,7 @@ public function make( $interface_or_class, $arguments = [] ) { * @param string $to Interface or class that provides the implementation. * @return Injector */ - public function bind( $from, $to ) { + public function bind( $from, $to ): Injector { $this->mappings[ $from ] = $to; return $this; @@ -171,7 +171,7 @@ public function bind_argument( $interface_or_class, $argument_name, $value - ) { + ): Injector { $this->argument_mappings[ $interface_or_class ][ $argument_name ] = $value; return $this; @@ -186,7 +186,7 @@ public function bind_argument( * @param string $interface_or_class Interface or class to reuse. * @return Injector */ - public function share( $interface_or_class ) { + public function share( $interface_or_class ): Injector { $this->shared_instances[ $interface_or_class ] = null; return $this; @@ -202,7 +202,7 @@ public function share( $interface_or_class ) { * @param callable $callable Callable to use for instantiation. * @return Injector */ - public function delegate( $interface_or_class, callable $callable ) { + public function delegate( $interface_or_class, callable $callable ): Injector { $this->delegates[ $interface_or_class ] = $callable; return $this; @@ -249,7 +249,7 @@ private function make_dependency( $object = $this->instantiator->instantiate( $class, $dependencies ); - if ( \array_key_exists( $class, $this->shared_instances ) ) { + if ( array_key_exists( $class, $this->shared_instances ) ) { $this->shared_instances[ $class ] = $object; } @@ -270,7 +270,7 @@ private function make_dependency( private function resolve( InjectionChain $injection_chain, $interface_or_class - ) { + ): InjectionChain { if ( $injection_chain->is_in_chain( $interface_or_class ) ) { // Circular reference detected, aborting. throw FailedToMakeInstance::for_circular_reference( @@ -281,7 +281,7 @@ private function resolve( $injection_chain = $injection_chain->add_resolution( $interface_or_class ); - if ( \array_key_exists( $interface_or_class, $this->mappings ) ) { + if ( array_key_exists( $interface_or_class, $this->mappings ) ) { return $this->resolve( $injection_chain, $this->mappings[ $interface_or_class ] @@ -309,7 +309,7 @@ private function get_dependencies_for( InjectionChain $injection_chain, ReflectionClass $reflection, $arguments = [] - ) { + ): array { $constructor = $reflection->getConstructor(); $class = $reflection->getName(); @@ -317,7 +317,7 @@ private function get_dependencies_for( return []; } - return \array_map( + return array_map( function ( ReflectionParameter $parameter ) use ( $injection_chain, $class, $arguments ) { return $this->resolve_argument( $injection_chain, @@ -385,7 +385,7 @@ private function resolve_argument( ); } - $type = $type instanceof \ReflectionNamedType + $type = $type instanceof ReflectionNamedType ? $type->getName() : (string) $type; } else { @@ -394,7 +394,7 @@ private function resolve_argument( // support it. $reflection_class = $parameter->getClass(); - $type = $reflection_class ? $reflection_class->name : null; + $type = $reflection_class->name ?? null; if ( null === $type ) { return $this->resolve_argument_by_name( @@ -428,13 +428,13 @@ private function resolve_argument_by_name( $name = $parameter->getName(); // The argument was directly provided to the make() call. - if ( \array_key_exists( $name, $arguments ) ) { + if ( array_key_exists( $name, $arguments ) ) { return $arguments[ $name ]; } // Check if we have mapped this argument for the specific class. - if ( \array_key_exists( $class, $this->argument_mappings ) - && \array_key_exists( $name, $this->argument_mappings[ $class ] ) ) { + if ( array_key_exists( $class, $this->argument_mappings ) + && array_key_exists( $name, $this->argument_mappings[ $class ] ) ) { $value = $this->argument_mappings[ $class ][ $name ]; // Closures are immediately resolved, to provide lazy resolution. @@ -446,7 +446,7 @@ private function resolve_argument_by_name( } // No argument found for the class, check if we have a global value. - if ( \array_key_exists( $name, $this->argument_mappings[ self::GLOBAL_ARGUMENTS ] ) ) { + if ( array_key_exists( $name, $this->argument_mappings[ self::GLOBAL_ARGUMENTS ] ) ) { return $this->argument_mappings[ self::GLOBAL_ARGUMENTS ][ $name ]; } @@ -471,8 +471,8 @@ private function resolve_argument_by_name( * @param string $class Class to check for a shared instance. * @return bool Whether a shared instance exists. */ - private function has_shared_instance( $class ) { - return \array_key_exists( $class, $this->shared_instances ) + private function has_shared_instance( $class ): bool { + return array_key_exists( $class, $this->shared_instances ) && null !== $this->shared_instances[ $class ]; } @@ -502,8 +502,8 @@ private function get_shared_instance( $class ) { * @param string $class Class to check for a delegate. * @return bool Whether a delegate exists. */ - private function has_delegate( $class ) { - return \array_key_exists( $class, $this->delegates ); + private function has_delegate( $class ): bool { + return array_key_exists( $class, $this->delegates ); } /** @@ -515,7 +515,7 @@ private function has_delegate( $class ) { * @return callable Delegate. * @throws FailedToMakeInstance If an invalid delegate is requested. */ - private function get_delegate( $class ) { + private function get_delegate( $class ): callable { if ( ! $this->has_delegate( $class ) ) { throw FailedToMakeInstance::for_invalid_delegate( $class ); } diff --git a/includes/Infrastructure/Plugin.php b/includes/Infrastructure/Plugin.php index 6e5b55c970fc..766f3fdd22d0 100644 --- a/includes/Infrastructure/Plugin.php +++ b/includes/Infrastructure/Plugin.php @@ -44,5 +44,5 @@ interface Plugin extends Activateable, Deactivateable, Registerable { * * @return ServiceContainer Service container of the plugin. */ - public function get_container(); + public function get_container(): ServiceContainer; } diff --git a/includes/Infrastructure/ServiceBasedPlugin.php b/includes/Infrastructure/ServiceBasedPlugin.php index b5974226f3ac..b4672bef2f2c 100644 --- a/includes/Infrastructure/ServiceBasedPlugin.php +++ b/includes/Infrastructure/ServiceBasedPlugin.php @@ -391,7 +391,7 @@ protected function register_service( $id, $class ) { * * @return ServiceContainer Service container of the plugin. */ - public function get_container() { + public function get_container(): ServiceContainer { return $this->service_container; } diff --git a/includes/Infrastructure/ServiceContainer.php b/includes/Infrastructure/ServiceContainer.php index fc0aacfca1db..3b9abca3b0b8 100644 --- a/includes/Infrastructure/ServiceContainer.php +++ b/includes/Infrastructure/ServiceContainer.php @@ -46,7 +46,7 @@ interface ServiceContainer extends Traversable, Countable, ArrayAccess { * * @return Service Service that was requested. */ - public function get( $id ); + public function get( $id ): Service; /** * Check whether the container can return a service for the given @@ -58,7 +58,7 @@ public function get( $id ); * * @return bool */ - public function has( $id ); + public function has( $id ): bool; /** * Put a service into the container for later retrieval. diff --git a/includes/Infrastructure/ServiceContainer/LazilyInstantiatedService.php b/includes/Infrastructure/ServiceContainer/LazilyInstantiatedService.php index 8ee2a317c33c..b6f20a571df2 100644 --- a/includes/Infrastructure/ServiceContainer/LazilyInstantiatedService.php +++ b/includes/Infrastructure/ServiceContainer/LazilyInstantiatedService.php @@ -54,7 +54,7 @@ public function __construct( callable $instantiation ) { * * @return Service Properly instantiated service. */ - public function instantiate() { + public function instantiate(): Service { $instantiation = $this->instantiation; // Because uniform variable syntax not supported in PHP 5.6. $service = $instantiation(); diff --git a/includes/Infrastructure/ServiceContainer/SimpleServiceContainer.php b/includes/Infrastructure/ServiceContainer/SimpleServiceContainer.php index 684532a0483b..e7cd126c8603 100644 --- a/includes/Infrastructure/ServiceContainer/SimpleServiceContainer.php +++ b/includes/Infrastructure/ServiceContainer/SimpleServiceContainer.php @@ -46,7 +46,7 @@ final class SimpleServiceContainer * * @return Service Service that was requested. */ - public function get( $id ) { + public function get( $id ): Service { if ( ! $this->has( $id ) ) { throw InvalidService::from_service_id( $id ); } @@ -72,7 +72,7 @@ public function get( $id ) { * * @return bool */ - public function has( $id ) { + public function has( $id ): bool { return $this->offsetExists( $id ); } From 1b9590c28ba565519b348f1718991f347278a12d Mon Sep 17 00:00:00 2001 From: Pascal Birchler Date: Thu, 5 Aug 2021 00:42:20 +0200 Subject: [PATCH 07/41] Pull upstream changes --- .../Infrastructure/ServiceBasedPlugin.php | 111 ++++++++++++++---- .../Fixture/DummyServiceWithDelay.php | 41 +++++++ .../Infrastructure/ServiceBasedPluginTest.php | 86 ++++++++++++-- 3 files changed, 204 insertions(+), 34 deletions(-) create mode 100644 tests/phpunit/includes/Fixture/DummyServiceWithDelay.php diff --git a/includes/Infrastructure/ServiceBasedPlugin.php b/includes/Infrastructure/ServiceBasedPlugin.php index b4672bef2f2c..4af3c8c8df2a 100644 --- a/includes/Infrastructure/ServiceBasedPlugin.php +++ b/includes/Infrastructure/ServiceBasedPlugin.php @@ -21,9 +21,9 @@ use Google\Web_Stories\Infrastructure\ServiceContainer\LazilyInstantiatedService; use function add_action; use function apply_filters; +use function did_action; use function Google\Web_Stories\rewrite_flush; - /** * This abstract base plugin provides all the boilerplate code for working with * the dependency injector and the service container. @@ -223,33 +223,26 @@ 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; } // Allow the services to delay their registration. if ( is_a( $class, Delayed::class, true ) ) { - $registration_action = $class::get_registration_action(); + $registration_action = $class::get_registration_action(); + $registration_action_priority = $class::get_registration_action_priority(); 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 ); - } + }, + $registration_action_priority ); } @@ -266,30 +259,104 @@ function () use ( $id, $class ) { /** * Determine if the requirements for a service to be registered are met. * + * This also hooks up another check in the future to the registration action(s) of its requirements. + * * @since 1.10.0 * - * @param class-string|HasRequirements $class Service with requirements. - * @param string[] $service_ids List of service IDs to be registered. + * @param string $id Service ID of the service with requirements. + * @param HasRequirements|class-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 ): bool { + protected function requirements_are_met( string $id, $class, array &$services ): bool { + $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, $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. + * + * @since 1.10.0 + * + * @param HasRequirements|class-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 ): array { $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; } /** diff --git a/tests/phpunit/includes/Fixture/DummyServiceWithDelay.php b/tests/phpunit/includes/Fixture/DummyServiceWithDelay.php new file mode 100644 index 000000000000..0be77c6960ad --- /dev/null +++ b/tests/phpunit/includes/Fixture/DummyServiceWithDelay.php @@ -0,0 +1,41 @@ +setConstructorArgs( [ true, null, $container ] ) ->setMethodsExcept( [ + 'collect_missing_requirements', 'register', 'register_services', + 'requirements_are_met', + 'get_container', '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 ]; + $service_callback = static function ( $services ) { + return array_merge( + $services, + [ '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(); - remove_filter( 'services', $service_callback ); + $this->assertCount( 4, $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( 'service_with_requirements' ) ); + $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( + [ + 'collect_missing_requirements', + 'register', + 'register_services', + 'requirements_are_met', + 'get_container', + 'get_service_classes', + ] + ) + ->getMock(); - // Successfully registers a service that has requirements. $service_callback = static function ( $services ) { - array_unshift( + return array_merge( $services, - [ 'filtered_service' => DummyServiceWithRequirements::class ] + [ + 'service_a' => DummyServiceWithDelay::class, + 'service_with_requirements' => DummyServiceWithRequirements::class, + ] ); - return $services; }; add_filter( 'services', $service_callback ); $plugin->register(); + $this->assertCount( 2, $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->assertCount( 4, $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( '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 c50f20c261db93b31c447c93d6b6dbbb731176df Mon Sep 17 00:00:00 2001 From: Pascal Birchler Date: Fri, 20 Aug 2021 13:45:26 +0200 Subject: [PATCH 08/41] Update docs --- includes/Infrastructure/HasRequirements.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/includes/Infrastructure/HasRequirements.php b/includes/Infrastructure/HasRequirements.php index 1c31bd536eef..5e04a8e44e21 100644 --- a/includes/Infrastructure/HasRequirements.php +++ b/includes/Infrastructure/HasRequirements.php @@ -37,6 +37,8 @@ interface HasRequirements { /** * Get the list of service IDs required for this service to be registered. * + * @since 1.11.0 + * * @return string[] List of required services. */ public static function get_requirements(): array; From 9686c90a7069ec266ac810736fc85cfbd641ddae Mon Sep 17 00:00:00 2001 From: Pascal Birchler Date: Fri, 20 Aug 2021 13:49:41 +0200 Subject: [PATCH 09/41] Ensure services that are delayed and have requirements are scheduled correctly See ampproject/amp-wp#6548 --- .../Infrastructure/ServiceBasedPlugin.php | 63 +++++++++++-------- 1 file changed, 38 insertions(+), 25 deletions(-) diff --git a/includes/Infrastructure/ServiceBasedPlugin.php b/includes/Infrastructure/ServiceBasedPlugin.php index 4af3c8c8df2a..e847bbf98425 100644 --- a/includes/Infrastructure/ServiceBasedPlugin.php +++ b/includes/Infrastructure/ServiceBasedPlugin.php @@ -229,28 +229,7 @@ public function register_services() { continue; } - // Allow the services to delay their registration. - if ( is_a( $class, Delayed::class, true ) ) { - $registration_action = $class::get_registration_action(); - $registration_action_priority = $class::get_registration_action_priority(); - - if ( did_action( $registration_action ) ) { - $this->register_service( $id, $class ); - } else { - add_action( - $registration_action, - function () use ( $id, $class ) { - $this->register_service( $id, $class ); - }, - $registration_action_priority - ); - } - - next( $services ); - continue; - } - - $this->register_service( $id, $class ); + $this->schedule_potential_service_registration( $id, $class ); next( $services ); } @@ -306,7 +285,7 @@ function () use ( $id, $class, $services ) { return; } - $this->register_service( $id, $class ); + $this->schedule_potential_service_registration( $id, $class ); }, PHP_INT_MAX ); @@ -425,7 +404,36 @@ protected function get_identifier_from_fqcn( $fqcn ): string { } /** - * Register a single service. + * Schedule the potential registration of a single service. + * + * This takes into account whether the service registration needs to be delayed or not. + * + * @since 1.11.0 + * + * @param string $id ID of the service to register. + * @param string $class Class of the service to register. + */ + protected function schedule_potential_service_registration( $id, $class ) { + if ( is_a( $class, Delayed::class, true ) ) { + $registration_action = $class::get_registration_action(); + + if ( \did_action( $registration_action ) ) { + $this->maybe_register_service( $id, $class ); + } else { + \add_action( + $registration_action, + function () use ( $id, $class ) { + $this->maybe_register_service( $id, $class ); + } + ); + } + } else { + $this->maybe_register_service( $id, $class ); + } + } + + /** + * Register a single service, provided its conditions are met. * * @since 1.6.0 * @@ -434,7 +442,12 @@ protected function get_identifier_from_fqcn( $fqcn ): string { * * @return void */ - protected function register_service( $id, $class ) { + protected function maybe_register_service( $id, $class ) { + // Ensure we don't register the same service more than once. + if ( $this->service_container->has( $id ) ) { + return; + } + // Only instantiate services that are actually needed. if ( is_a( $class, Conditional::class, true ) && ! $class::is_needed() ) { From e8db36d649a0e3ca14f834a5e80e929952f3065d Mon Sep 17 00:00:00 2001 From: Pascal Birchler Date: Fri, 20 Aug 2021 14:08:53 +0200 Subject: [PATCH 10/41] Fix PHPDoc --- includes/Infrastructure/ServiceBasedPlugin.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/includes/Infrastructure/ServiceBasedPlugin.php b/includes/Infrastructure/ServiceBasedPlugin.php index e847bbf98425..b7777db377aa 100644 --- a/includes/Infrastructure/ServiceBasedPlugin.php +++ b/includes/Infrastructure/ServiceBasedPlugin.php @@ -410,8 +410,8 @@ protected function get_identifier_from_fqcn( $fqcn ): string { * * @since 1.11.0 * - * @param string $id ID of the service to register. - * @param string $class Class of the service to register. + * @param string $id ID of the service to register. + * @param HasRequirements|class-string $class Class of the service to register. */ protected function schedule_potential_service_registration( $id, $class ) { if ( is_a( $class, Delayed::class, true ) ) { From fbbd35fcee166b82da5ea5326cc853cd523e7000 Mon Sep 17 00:00:00 2001 From: Pascal Birchler Date: Fri, 20 Aug 2021 14:08:56 +0200 Subject: [PATCH 11/41] Fix typo --- includes/Infrastructure/ServiceBasedPlugin.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/includes/Infrastructure/ServiceBasedPlugin.php b/includes/Infrastructure/ServiceBasedPlugin.php index b7777db377aa..3dfdd4a98c5d 100644 --- a/includes/Infrastructure/ServiceBasedPlugin.php +++ b/includes/Infrastructure/ServiceBasedPlugin.php @@ -71,7 +71,7 @@ abstract class ServiceBasedPlugin implements Plugin { protected $injector; /** - * ServiceC ontainer. + * ServiceContainer. * * @var ServiceContainer */ From 617873c02e76845ecd21a700bbc183b43cc9d92b Mon Sep 17 00:00:00 2001 From: Pascal Birchler Date: Fri, 20 Aug 2021 14:14:28 +0200 Subject: [PATCH 12/41] Add some more robust assertions --- includes/REST_API/Embed_Controller.php | 1 + tests/phpunit/tests/REST_API/Autosaves_Controller.php | 2 ++ tests/phpunit/tests/REST_API/Embed_Controller.php | 1 + tests/phpunit/tests/REST_API/Stories_Controller.php | 2 ++ .../tests/REST_API/Stories_Media_Controller.php | 2 ++ .../tests/REST_API/Stories_Users_Controller.php | 10 +++++----- 6 files changed, 13 insertions(+), 5 deletions(-) diff --git a/includes/REST_API/Embed_Controller.php b/includes/REST_API/Embed_Controller.php index bc849cf9c877..9a41271d4148 100644 --- a/includes/REST_API/Embed_Controller.php +++ b/includes/REST_API/Embed_Controller.php @@ -44,6 +44,7 @@ */ class Embed_Controller extends REST_Controller { use Document_Parser, Post_Type; + /** * Constructor. */ diff --git a/tests/phpunit/tests/REST_API/Autosaves_Controller.php b/tests/phpunit/tests/REST_API/Autosaves_Controller.php index cd2990a0fb01..013927394879 100644 --- a/tests/phpunit/tests/REST_API/Autosaves_Controller.php +++ b/tests/phpunit/tests/REST_API/Autosaves_Controller.php @@ -92,6 +92,8 @@ public function test_create_item_as_author_should_not_strip_markup() { $response = rest_get_server()->dispatch( $request ); $new_data = $response->get_data(); + $this->assertArrayHasKey( 'content', $new_data ); + $this->assertArrayHasKey( 'content', $new_data ); $this->assertEquals( $unsanitized_content, $new_data['content']['raw'] ); $this->assertEquals( $unsanitized_story_data, $new_data['story_data'] ); diff --git a/tests/phpunit/tests/REST_API/Embed_Controller.php b/tests/phpunit/tests/REST_API/Embed_Controller.php index 8633f7e396b1..2ec2dced3545 100644 --- a/tests/phpunit/tests/REST_API/Embed_Controller.php +++ b/tests/phpunit/tests/REST_API/Embed_Controller.php @@ -162,6 +162,7 @@ public function test_register_routes() { $this->assertArrayHasKey( '/web-stories/v1/embed', $routes ); $route = $routes['/web-stories/v1/embed']; + $this->assertCount( 1, $route ); $this->assertArrayHasKey( 'callback', $route[0] ); $this->assertArrayHasKey( 'permission_callback', $route[0] ); diff --git a/tests/phpunit/tests/REST_API/Stories_Controller.php b/tests/phpunit/tests/REST_API/Stories_Controller.php index 293f9fb77c6d..2cd30f26dbcf 100644 --- a/tests/phpunit/tests/REST_API/Stories_Controller.php +++ b/tests/phpunit/tests/REST_API/Stories_Controller.php @@ -405,6 +405,8 @@ public function test_get_item_schema() { $this->assertNotEmpty( $data ); + $this->assertArrayHasKey( 'schema', $data ); + $this->assertArrayHasKey( 'properties', $data['schema'] ); $properties = $data['schema']['properties']; $this->assertArrayHasKey( 'story_data', $properties ); $this->assertArrayHasKey( 'featured_media_url', $properties ); diff --git a/tests/phpunit/tests/REST_API/Stories_Media_Controller.php b/tests/phpunit/tests/REST_API/Stories_Media_Controller.php index 56bf3f2db22b..5cf5fca5b39f 100644 --- a/tests/phpunit/tests/REST_API/Stories_Media_Controller.php +++ b/tests/phpunit/tests/REST_API/Stories_Media_Controller.php @@ -286,6 +286,8 @@ public function test_get_item_schema() { $this->assertNotEmpty( $data ); + $this->assertArrayHasKey( 'schema', $data ); + $this->assertArrayHasKey( 'properties', $data['schema'] ); $properties = $data['schema']['properties']; $this->assertArrayNotHasKey( 'permalink_template', $properties ); $this->assertArrayNotHasKey( 'generated_slug', $properties ); diff --git a/tests/phpunit/tests/REST_API/Stories_Users_Controller.php b/tests/phpunit/tests/REST_API/Stories_Users_Controller.php index fe0d8a5e2349..c2899b63fead 100644 --- a/tests/phpunit/tests/REST_API/Stories_Users_Controller.php +++ b/tests/phpunit/tests/REST_API/Stories_Users_Controller.php @@ -109,7 +109,7 @@ public function test_count_user_posts() { [ self::$user_id, \Google\Web_Stories\Story_Post_Type::POST_TYPE_SLUG, - ] + ] ); $this->assertEquals( 3, $result1 ); @@ -126,7 +126,7 @@ public function test_count_user_posts() { [ self::$user_id, \Google\Web_Stories\Story_Post_Type::POST_TYPE_SLUG, - ] + ] ); $this->assertEquals( 4, $result2 ); @@ -139,7 +139,7 @@ public function test_count_user_posts() { [ self::$user_id, \Google\Web_Stories\Story_Post_Type::POST_TYPE_SLUG, - ] + ] ); $this->assertEquals( 3, $result3 ); @@ -159,7 +159,7 @@ public function test_count_user_posts_invalid() { [ -1, \Google\Web_Stories\Story_Post_Type::POST_TYPE_SLUG, - ] + ] ); $this->assertEquals( 0, $result1 ); @@ -169,7 +169,7 @@ public function test_count_user_posts_invalid() { [ self::$user_id, 'invalid', - ] + ] ); $this->assertEquals( 0, $result1 ); } From 9955cbebcfc244f589d2056dcaf9cdecdf935609 Mon Sep 17 00:00:00 2001 From: Pascal Birchler Date: Fri, 20 Aug 2021 14:16:12 +0200 Subject: [PATCH 13/41] Move `rest_api_init` to `wpSetUpBeforeClass` --- .../tests/REST_API/Autosaves_Controller.php | 18 +++++++++--------- .../tests/REST_API/Embed_Controller.php | 18 +++++++++--------- .../tests/REST_API/Link_Controller.php | 19 +++++++++---------- .../tests/REST_API/Lock_Controller.php | 18 +++++++++--------- .../REST_API/Page_Template_Controller.php | 18 +++++++++--------- .../REST_API/Status_Check_Controller.php | 18 +++++++++--------- .../tests/REST_API/Stories_Controller.php | 18 +++++++++--------- .../REST_API/Stories_Media_Controller.php | 19 ++++++++++--------- .../REST_API/Stories_Users_Controller.php | 17 ++++++++--------- 9 files changed, 81 insertions(+), 82 deletions(-) diff --git a/tests/phpunit/tests/REST_API/Autosaves_Controller.php b/tests/phpunit/tests/REST_API/Autosaves_Controller.php index 013927394879..6e43edc0d378 100644 --- a/tests/phpunit/tests/REST_API/Autosaves_Controller.php +++ b/tests/phpunit/tests/REST_API/Autosaves_Controller.php @@ -40,28 +40,28 @@ public static function wpSetUpBeforeClass( $factory ) { 'role' => 'author', ] ); + + /** @var \WP_REST_Server $wp_rest_server */ + global $wp_rest_server; + $wp_rest_server = new Spy_REST_Server(); + do_action( 'rest_api_init', $wp_rest_server ); } public static function wpTearDownAfterClass() { self::delete_user( self::$author_id ); + + /** @var \WP_REST_Server $wp_rest_server */ + global $wp_rest_server; + $wp_rest_server = null; } public function setUp() { parent::setUp(); - /** @var \WP_REST_Server $wp_rest_server */ - global $wp_rest_server; - $wp_rest_server = new Spy_REST_Server(); - do_action( 'rest_api_init', $wp_rest_server ); - $this->add_caps_to_roles(); } public function tearDown() { - /** @var \WP_REST_Server $wp_rest_server */ - global $wp_rest_server; - $wp_rest_server = null; - $this->remove_caps_from_roles(); parent::tearDown(); diff --git a/tests/phpunit/tests/REST_API/Embed_Controller.php b/tests/phpunit/tests/REST_API/Embed_Controller.php index 2ec2dced3545..b0a74fb29721 100644 --- a/tests/phpunit/tests/REST_API/Embed_Controller.php +++ b/tests/phpunit/tests/REST_API/Embed_Controller.php @@ -83,22 +83,26 @@ public static function wpSetUpBeforeClass( $factory ) { add_filter( 'content_save_pre', 'wp_filter_post_kses' ); add_filter( 'content_filtered_save_pre', 'wp_filter_post_kses' ); + + /** @var \WP_REST_Server $wp_rest_server */ + global $wp_rest_server; + $wp_rest_server = new Spy_REST_Server(); + do_action( 'rest_api_init', $wp_rest_server ); } public static function wpTearDownAfterClass() { self::delete_user( self::$subscriber ); self::delete_user( self::$editor ); self::delete_user( self::$admin ); + + /** @var \WP_REST_Server $wp_rest_server */ + global $wp_rest_server; + $wp_rest_server = null; } public function setUp() { parent::setUp(); - /** @var \WP_REST_Server $wp_rest_server */ - global $wp_rest_server; - $wp_rest_server = new Spy_REST_Server(); - do_action( 'rest_api_init', $wp_rest_server ); - add_filter( 'pre_http_request', [ $this, 'mock_http_request' ], 10, 3 ); $this->request_count = 0; @@ -106,10 +110,6 @@ public function setUp() { } public function tearDown() { - /** @var \WP_REST_Server $wp_rest_server */ - global $wp_rest_server; - $wp_rest_server = null; - remove_filter( 'pre_http_request', [ $this, 'mock_http_request' ] ); $this->remove_caps_from_roles(); diff --git a/tests/phpunit/tests/REST_API/Link_Controller.php b/tests/phpunit/tests/REST_API/Link_Controller.php index 26a193b8b88c..be1171664b9b 100644 --- a/tests/phpunit/tests/REST_API/Link_Controller.php +++ b/tests/phpunit/tests/REST_API/Link_Controller.php @@ -51,33 +51,32 @@ public static function wpSetUpBeforeClass( $factory ) { 'user_email' => 'editor@example.com', ] ); + + /** @var \WP_REST_Server $wp_rest_server */ + global $wp_rest_server; + $wp_rest_server = new Spy_REST_Server(); + do_action( 'rest_api_init', $wp_rest_server ); } public static function wpTearDownAfterClass() { self::delete_user( self::$subscriber ); self::delete_user( self::$editor ); + + /** @var WP_REST_Server $wp_rest_server */ + global $wp_rest_server; + $wp_rest_server = null; } public function setUp() { parent::setUp(); - /** @var WP_REST_Server $wp_rest_server */ - global $wp_rest_server; - $wp_rest_server = new Spy_REST_Server(); - do_action( 'rest_api_init', $wp_rest_server ); - add_filter( 'pre_http_request', [ $this, 'mock_http_request' ], 10, 3 ); - $this->request_count = 0; $this->add_caps_to_roles(); } public function tearDown() { - /** @var WP_REST_Server $wp_rest_server */ - global $wp_rest_server; - $wp_rest_server = null; - remove_filter( 'pre_http_request', [ $this, 'mock_http_request' ] ); $this->remove_caps_from_roles(); diff --git a/tests/phpunit/tests/REST_API/Lock_Controller.php b/tests/phpunit/tests/REST_API/Lock_Controller.php index 6203b11e7e13..708fc106585a 100644 --- a/tests/phpunit/tests/REST_API/Lock_Controller.php +++ b/tests/phpunit/tests/REST_API/Lock_Controller.php @@ -52,29 +52,29 @@ public static function wpSetUpBeforeClass( $factory ) { 'user_email' => 'editor@example.com', ] ); + + /** @var \WP_REST_Server $wp_rest_server */ + global $wp_rest_server; + $wp_rest_server = new Spy_REST_Server(); + do_action( 'rest_api_init', $wp_rest_server ); } public static function wpTearDownAfterClass() { self::delete_user( self::$author_id ); self::delete_user( self::$subscriber ); + + /** @var \WP_REST_Server $wp_rest_server */ + global $wp_rest_server; + $wp_rest_server = null; } public function setUp() { parent::setUp(); - /** @var \WP_REST_Server $wp_rest_server */ - global $wp_rest_server; - $wp_rest_server = new Spy_REST_Server(); - do_action( 'rest_api_init', $wp_rest_server ); - $this->add_caps_to_roles(); } public function tearDown() { - /** @var \WP_REST_Server $wp_rest_server */ - global $wp_rest_server; - $wp_rest_server = null; - $this->remove_caps_from_roles(); parent::tearDown(); diff --git a/tests/phpunit/tests/REST_API/Page_Template_Controller.php b/tests/phpunit/tests/REST_API/Page_Template_Controller.php index 2d13cdcc94e9..eb879c3cf7a2 100644 --- a/tests/phpunit/tests/REST_API/Page_Template_Controller.php +++ b/tests/phpunit/tests/REST_API/Page_Template_Controller.php @@ -116,6 +116,11 @@ public static function wpSetUpBeforeClass( $factory ) { 'post_type' => $post_type, ] ); + + /** @var \WP_REST_Server $wp_rest_server */ + global $wp_rest_server; + $wp_rest_server = new Spy_REST_Server(); + do_action( 'rest_api_init', $wp_rest_server ); } public static function wpTearDownAfterClass() { @@ -123,26 +128,21 @@ public static function wpTearDownAfterClass() { self::delete_user( self::$user2_id ); self::delete_user( self::$user3_id ); self::delete_user( self::$author_id ); + + /** @var \WP_REST_Server $wp_rest_server */ + global $wp_rest_server; + $wp_rest_server = null; } public function setUp() { parent::setUp(); - /** @var \WP_REST_Server $wp_rest_server */ - global $wp_rest_server; - $wp_rest_server = new Spy_REST_Server(); - do_action( 'rest_api_init', $wp_rest_server ); - $this->add_caps_to_roles(); $this->set_permalink_structure( '/%postname%/' ); } public function tearDown() { - /** @var \WP_REST_Server $wp_rest_server */ - global $wp_rest_server; - $wp_rest_server = null; - $this->remove_caps_from_roles(); $this->set_permalink_structure( '' ); diff --git a/tests/phpunit/tests/REST_API/Status_Check_Controller.php b/tests/phpunit/tests/REST_API/Status_Check_Controller.php index 8599c525806c..f9c39c82d8ca 100644 --- a/tests/phpunit/tests/REST_API/Status_Check_Controller.php +++ b/tests/phpunit/tests/REST_API/Status_Check_Controller.php @@ -56,31 +56,31 @@ public static function wpSetUpBeforeClass( $factory ) { 'user_email' => 'editor@example.com', ] ); + + /** @var \WP_REST_Server $wp_rest_server */ + global $wp_rest_server; + $wp_rest_server = new Spy_REST_Server(); + do_action( 'rest_api_init', $wp_rest_server ); } public static function wpTearDownAfterClass() { self::delete_user( self::$subscriber ); self::delete_user( self::$editor ); + + /** @var \WP_REST_Server $wp_rest_server */ + global $wp_rest_server; + $wp_rest_server = null; } public function setUp() { parent::setUp(); - /** @var \WP_REST_Server $wp_rest_server */ - global $wp_rest_server; - $wp_rest_server = new Spy_REST_Server(); - do_action( 'rest_api_init', $wp_rest_server ); - $this->request_count = 0; $this->add_caps_to_roles(); } public function tearDown() { - /** @var \WP_REST_Server $wp_rest_server */ - global $wp_rest_server; - $wp_rest_server = null; - $this->remove_caps_from_roles(); parent::tearDown(); diff --git a/tests/phpunit/tests/REST_API/Stories_Controller.php b/tests/phpunit/tests/REST_API/Stories_Controller.php index 2cd30f26dbcf..fb899fdb3b59 100644 --- a/tests/phpunit/tests/REST_API/Stories_Controller.php +++ b/tests/phpunit/tests/REST_API/Stories_Controller.php @@ -123,6 +123,11 @@ public static function wpSetUpBeforeClass( $factory ) { 'post_type' => $post_type, ] ); + + /** @var \WP_REST_Server $wp_rest_server */ + global $wp_rest_server; + $wp_rest_server = new Spy_REST_Server(); + do_action( 'rest_api_init', $wp_rest_server ); } public static function wpTearDownAfterClass() { @@ -131,26 +136,21 @@ public static function wpTearDownAfterClass() { self::delete_user( self::$user3_id ); self::delete_user( self::$author_id ); self::delete_user( self::$contributor_id ); + + /** @var \WP_REST_Server $wp_rest_server */ + global $wp_rest_server; + $wp_rest_server = null; } public function setUp() { parent::setUp(); - /** @var \WP_REST_Server $wp_rest_server */ - global $wp_rest_server; - $wp_rest_server = new Spy_REST_Server(); - do_action( 'rest_api_init', $wp_rest_server ); - $this->add_caps_to_roles(); $this->set_permalink_structure( '/%postname%/' ); } public function tearDown() { - /** @var \WP_REST_Server $wp_rest_server */ - global $wp_rest_server; - $wp_rest_server = null; - $this->remove_caps_from_roles(); $this->set_permalink_structure( '' ); diff --git a/tests/phpunit/tests/REST_API/Stories_Media_Controller.php b/tests/phpunit/tests/REST_API/Stories_Media_Controller.php index 5cf5fca5b39f..56f3b956222e 100644 --- a/tests/phpunit/tests/REST_API/Stories_Media_Controller.php +++ b/tests/phpunit/tests/REST_API/Stories_Media_Controller.php @@ -71,28 +71,29 @@ public static function wpSetUpBeforeClass( $factory ) { 'display_name' => 'Andrea Adams', ] ); + + /** @var \WP_REST_Server $wp_rest_server */ + global $wp_rest_server; + $wp_rest_server = new Spy_REST_Server(); + do_action( 'rest_api_init', $wp_rest_server ); + } public static function wpTearDownAfterClass() { self::delete_user( self::$user_id ); + + /** @var \WP_REST_Server $wp_rest_server */ + global $wp_rest_server; + $wp_rest_server = null; } public function setUp() { parent::setUp(); - /** @var \WP_REST_Server $wp_rest_server */ - global $wp_rest_server; - $wp_rest_server = new Spy_REST_Server(); - do_action( 'rest_api_init', $wp_rest_server ); - $this->add_caps_to_roles(); } public function tearDown() { - /** @var \WP_REST_Server $wp_rest_server */ - global $wp_rest_server; - $wp_rest_server = null; - $this->remove_caps_from_roles(); parent::tearDown(); diff --git a/tests/phpunit/tests/REST_API/Stories_Users_Controller.php b/tests/phpunit/tests/REST_API/Stories_Users_Controller.php index c2899b63fead..97acfe21b442 100644 --- a/tests/phpunit/tests/REST_API/Stories_Users_Controller.php +++ b/tests/phpunit/tests/REST_API/Stories_Users_Controller.php @@ -54,30 +54,29 @@ public static function wpSetUpBeforeClass( $factory ) { ] ); + /** @var \WP_REST_Server $wp_rest_server */ + global $wp_rest_server; + $wp_rest_server = new Spy_REST_Server(); + do_action( 'rest_api_init', $wp_rest_server ); } public static function wpTearDownAfterClass() { self::delete_user( self::$user_id ); + + /** @var \WP_REST_Server $wp_rest_server */ + global $wp_rest_server; + $wp_rest_server = null; } public function setUp() { parent::setUp(); - /** @var \WP_REST_Server $wp_rest_server */ - global $wp_rest_server; - $wp_rest_server = new Spy_REST_Server(); - do_action( 'rest_api_init', $wp_rest_server ); - $this->add_caps_to_roles(); $this->set_permalink_structure( '/%postname%/' ); } public function tearDown() { - /** @var \WP_REST_Server $wp_rest_server */ - global $wp_rest_server; - $wp_rest_server = null; - $this->remove_caps_from_roles(); $this->set_permalink_structure( '' ); From 62bd00f3cc3e171de6bdb2f93b9d444d5eee116a Mon Sep 17 00:00:00 2001 From: Pascal Birchler Date: Thu, 9 Sep 2021 00:48:37 +0200 Subject: [PATCH 14/41] Add return type --- includes/Infrastructure/ServiceBasedPlugin.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/includes/Infrastructure/ServiceBasedPlugin.php b/includes/Infrastructure/ServiceBasedPlugin.php index 383f078def7d..94e96e79a2d9 100644 --- a/includes/Infrastructure/ServiceBasedPlugin.php +++ b/includes/Infrastructure/ServiceBasedPlugin.php @@ -467,10 +467,11 @@ protected function get_identifier_from_fqcn( $fqcn ): string { * * This takes into account whether the service registration needs to be delayed or not. * - * @since 1.11.0 + * @since 1.12.0 * * @param string $id ID of the service to register. * @param HasRequirements|class-string $class Class of the service to register. + * @return void */ protected function schedule_potential_service_registration( $id, $class ) { if ( is_a( $class, Delayed::class, true ) ) { From da6f69ffab5d84a482c51922d4960d299d1ce80a Mon Sep 17 00:00:00 2001 From: Pascal Birchler Date: Thu, 23 Sep 2021 00:03:01 +0200 Subject: [PATCH 15/41] Code & test fixes --- includes/AMP/Output_Buffer.php | 1 - includes/AMP/Sanitization.php | 7 +- includes/AdSense.php | 6 +- includes/Ad_Manager.php | 4 +- includes/Admin/Cross_Origin_Isolation.php | 56 +++++++----- includes/Admin/Customizer.php | 3 +- includes/Analytics.php | 4 +- includes/Experiments.php | 2 +- includes/Integrations/AMP.php | 4 +- includes/Interfaces/Renderer.php | 2 - includes/Media/Media_Source_Taxonomy.php | 1 - .../Migrations/Update_Publisher_Logos.php | 3 +- includes/REST_API/Autosaves_Controller.php | 4 +- includes/REST_API/Embed_Controller.php | 8 +- includes/REST_API/Link_Controller.php | 1 + includes/REST_API/Status_Check_Controller.php | 1 - .../REST_API/Template_Lock_Controller.php | 1 - includes/Renderer/Story/Embed.php | 1 - includes/Renderer/Story/HTML.php | 2 - includes/Settings.php | 15 +++ includes/Story_Post_Type.php | 4 +- includes/Tracking.php | 17 +++- includes/User/Capabilities.php | 9 ++ includes/User/Preferences.php | 14 +++ .../templates/frontend/single-web-story.php | 1 - .../includes/DependencyInjectedTestCase.php | 6 ++ .../integration/tests/AMP/Output_Buffer.php | 1 - .../integration/tests/AMP/Sanitization.php | 1 - .../tests/Admin/Cross_Origin_Isolation.php | 91 ++++++++----------- .../integration/tests/Admin/Site_Health.php | 1 - .../integration/tests/Admin/TinyMCE.php | 1 - .../integration/tests/Demo_Content.php | 3 - .../integration/tests/Integrations/AMP.php | 1 - .../tests/REST_API/Embed_Controller.php | 75 +++++++++++---- .../tests/REST_API/Hotlinking_Controller.php | 52 +++++++---- .../tests/REST_API/Link_Controller.php | 71 ++++++++++++--- .../REST_API/Page_Template_Controller.php | 3 - .../REST_API/Status_Check_Controller.php | 50 +++++++--- ...r.php => Stories_Autosaves_Controller.php} | 40 +++++--- .../tests/REST_API/Stories_Controller.php | 78 +++++++++++----- ...roller.php => Stories_Lock_Controller.php} | 59 +++++++++--- .../REST_API/Stories_Media_Controller.php | 44 +++++++-- .../Stories_Taxonomies_Controller.php | 29 ++++++ .../REST_API/Stories_Users_Controller.php | 51 +++++++---- .../integration/tests/Renderer/Archives.php | 1 - .../integration/tests/Renderer/Feed.php | 1 - .../integration/tests/Renderer/Oembed.php | 1 - .../integration/tests/Renderer/Single.php | 1 - .../Renderer/Stories/Generic_Renderer.php | 1 - .../integration/tests/Renderer/Story/HTML.php | 2 - tests/phpunit/integration/tests/Tracking.php | 3 - .../integration/tests/User/Preferences.php | 40 +++++++- 52 files changed, 600 insertions(+), 278 deletions(-) rename tests/phpunit/integration/tests/REST_API/{Autosaves_Controller.php => Stories_Autosaves_Controller.php} (88%) rename tests/phpunit/integration/tests/REST_API/{Lock_Controller.php => Stories_Lock_Controller.php} (92%) diff --git a/includes/AMP/Output_Buffer.php b/includes/AMP/Output_Buffer.php index 9ddf4fd6b4b8..382a77d692cd 100644 --- a/includes/AMP/Output_Buffer.php +++ b/includes/AMP/Output_Buffer.php @@ -26,7 +26,6 @@ namespace Google\Web_Stories\AMP; -use DOMElement; use Exception; use Google\Web_Stories\Exception\SanitizationException; use Google\Web_Stories\Infrastructure\Conditional; diff --git a/includes/AMP/Sanitization.php b/includes/AMP/Sanitization.php index 69fbeebf693b..635b392e4bf5 100644 --- a/includes/AMP/Sanitization.php +++ b/includes/AMP/Sanitization.php @@ -26,12 +26,10 @@ namespace Google\Web_Stories\AMP; -use Google\Web_Stories\Experiments; -use Google\Web_Stories\Media\Image_Sizes; use Google\Web_Stories\Model\Story; +use Google\Web_Stories\Services; use Google\Web_Stories\Settings; use Google\Web_Stories\Story_Post_Type; -use Google\Web_Stories\Traits\Publisher; use Google\Web_Stories_Dependencies\AMP_Allowed_Tags_Generated; use Google\Web_Stories_Dependencies\AMP_Content_Sanitizer; use Google\Web_Stories_Dependencies\AMP_Dev_Mode_Sanitizer; @@ -46,7 +44,6 @@ use Google\Web_Stories_Dependencies\AmpProject\Extension; use Google\Web_Stories_Dependencies\AmpProject\Tag; use DOMElement; -use WP_Post; /** * Sanitization class. @@ -407,7 +404,7 @@ protected function get_sanitizers(): array { $post = get_queried_object(); if ( $post instanceof \WP_Post && Story_Post_Type::POST_TYPE_SLUG === get_post_type( $post ) ) { - $video_cache_enabled = (bool) get_option( Settings::SETTING_NAME_VIDEO_CACHE ); + $video_cache_enabled = (bool) Services::get( 'settings' )->get_setting( Settings::SETTING_NAME_VIDEO_CACHE ); $story = new Story(); $story->load_from_post( $post ); diff --git a/includes/AdSense.php b/includes/AdSense.php index ef4089580a1b..fa9415708ad8 100644 --- a/includes/AdSense.php +++ b/includes/AdSense.php @@ -49,7 +49,7 @@ public function register() { * @return string Publisher ID. */ private function get_publisher_id(): string { - return (string) get_option( Settings::SETTING_NAME_ADSENSE_PUBLISHER_ID ); + return (string) Services::get( 'settings' )->get_setting( Settings::SETTING_NAME_ADSENSE_PUBLISHER_ID ); } /** @@ -60,7 +60,7 @@ private function get_publisher_id(): string { * @return string Slot ID. */ private function get_slot_id(): string { - return (string) get_option( Settings::SETTING_NAME_ADSENSE_SLOT_ID ); + return (string) Services::get( 'settings' )->get_setting( Settings::SETTING_NAME_ADSENSE_SLOT_ID ); } /** @@ -71,7 +71,7 @@ private function get_slot_id(): string { * @return bool */ private function is_enabled(): bool { - return ( 'adsense' === (string) get_option( Settings::SETTING_NAME_AD_NETWORK, 'none' ) ); + return ( 'adsense' === (string) Services::get( 'settings' )->get_setting( Settings::SETTING_NAME_AD_NETWORK, 'none' ) ); } /** diff --git a/includes/Ad_Manager.php b/includes/Ad_Manager.php index 95ec3e2abdcf..b6c2bc6f6d06 100644 --- a/includes/Ad_Manager.php +++ b/includes/Ad_Manager.php @@ -49,7 +49,7 @@ public function register() { * @return string Slot ID. */ private function get_slot_id(): string { - return (string) get_option( Settings::SETTING_NAME_AD_MANAGER_SLOT_ID ); + return (string) Services::get( 'settings' )->get_setting( Settings::SETTING_NAME_AD_MANAGER_SLOT_ID ); } /** @@ -60,7 +60,7 @@ private function get_slot_id(): string { * @return bool */ private function is_enabled(): bool { - return ( 'admanager' === (string) get_option( Settings::SETTING_NAME_AD_NETWORK, 'none' ) ); + return ( 'admanager' === (string) Services::get( 'settings' )->get_setting( Settings::SETTING_NAME_AD_NETWORK, 'none' ) ); } /** diff --git a/includes/Admin/Cross_Origin_Isolation.php b/includes/Admin/Cross_Origin_Isolation.php index e5d9fd21d28d..f648dff81b37 100644 --- a/includes/Admin/Cross_Origin_Isolation.php +++ b/includes/Admin/Cross_Origin_Isolation.php @@ -30,6 +30,7 @@ use Google\Web_Stories\Infrastructure\Conditional; use Google\Web_Stories\Service_Base; +use Google\Web_Stories\Services; use Google\Web_Stories\Traits\Screen; use Google\Web_Stories\User\Preferences; @@ -81,6 +82,38 @@ public static function get_registration_action_priority(): int { return 11; } + /** + * Check whether the conditional object is currently needed. + * + * @since 1.6.0 + * + * @return bool Whether the conditional object is needed. + */ + public static function is_needed(): bool { + $user_id = get_current_user_id(); + if ( ! $user_id ) { + return false; + } + + // Cross-origin isolation is not needed if users can't upload files anyway. + if ( ! user_can( $user_id, 'upload_files' ) ) { + return false; + } + + return rest_sanitize_boolean( Services::get( 'user_preferences' )->get_preference( $user_id, Preferences::MEDIA_OPTIMIZATION_META_KEY ) ); + } + + /** + * Get the list of service IDs required for this service to be registered. + * + * @since 1.12.0 + * + * @return string[] List of required services. + */ + public static function get_requirements(): array { + return [ 'user_preferences' ]; + } + /** * Start output buffer and add headers. * @@ -289,27 +322,4 @@ public function custom_print_media_templates() { private function starts_with( string $string, string $start_string ): bool { return 0 === strpos( $string, $start_string ); } - - /** - * Check whether the conditional object is currently needed. - * - * @since 1.6.0 - * - * @return bool Whether the conditional object is needed. - */ - public static function is_needed(): bool { - $user_id = get_current_user_id(); - if ( ! $user_id ) { - return false; - } - - // Cross-origin isolation is not needed if users can't upload files anyway. - if ( ! user_can( $user_id, 'upload_files' ) ) { - return false; - } - - $check = get_user_meta( $user_id, Preferences::MEDIA_OPTIMIZATION_META_KEY, true ); - - return rest_sanitize_boolean( $check ); - } } diff --git a/includes/Admin/Customizer.php b/includes/Admin/Customizer.php index 2769054d8b6a..7f34e95cc6d9 100644 --- a/includes/Admin/Customizer.php +++ b/includes/Admin/Customizer.php @@ -26,6 +26,7 @@ namespace Google\Web_Stories\Admin; +use Google\Web_Stories\Services; use Google\Web_Stories\Story_Query; use Google\Web_Stories\Service_Base; use Google\Web_Stories\Traits\Layout; @@ -536,7 +537,7 @@ public function validate_number_of_columns( $validity, $value ) { * @return string */ public function render_stories(): string { - $options = get_option( self::STORY_OPTION ); + $options = Services::get( 'settings' )->get_setting( self::STORY_OPTION ); if ( empty( $options['show_stories'] ) || true !== $options['show_stories'] ) { return ''; diff --git a/includes/Analytics.php b/includes/Analytics.php index 2a62c22cc1c1..d9fc4c67a3c1 100644 --- a/includes/Analytics.php +++ b/includes/Analytics.php @@ -68,7 +68,7 @@ public function register() { * @return string Tracking ID. */ public function get_tracking_id(): string { - return (string) get_option( Settings::SETTING_NAME_TRACKING_ID ); + return (string) Services::get( 'settings' )->get_setting( Settings::SETTING_NAME_TRACKING_ID ); } /** @@ -229,7 +229,7 @@ public function print_analytics_tag() { } if ( - (bool) get_option( Settings::SETTING_NAME_USING_LEGACY_ANALYTICS ) || + (bool) Services::get( 'settings' )->get_setting( Settings::SETTING_NAME_USING_LEGACY_ANALYTICS ) || ! $this->experiments->is_experiment_enabled( 'enableAutoAnalyticsMigration' ) ) { $this->print_amp_analytics_tag( $tracking_id ); diff --git a/includes/Experiments.php b/includes/Experiments.php index 74a456b29287..d6f8b23605d0 100644 --- a/includes/Experiments.php +++ b/includes/Experiments.php @@ -461,7 +461,7 @@ public function is_experiment_enabled( string $name ): bool { return (bool) $experiment['default']; } - $experiments = get_option( Settings::SETTING_NAME_EXPERIMENTS ); + $experiments = Services::get( 'settings' )->get_setting( Settings::SETTING_NAME_EXPERIMENTS, [] ); return ! empty( $experiments[ $name ] ); } diff --git a/includes/Integrations/AMP.php b/includes/Integrations/AMP.php index 7e31d1d17b7f..1c9da2c58d9d 100644 --- a/includes/Integrations/AMP.php +++ b/includes/Integrations/AMP.php @@ -28,8 +28,8 @@ use DOMElement; use Google\Web_Stories\AMP\Integration\AMP_Story_Sanitizer; -use Google\Web_Stories\Experiments; use Google\Web_Stories\Model\Story; +use Google\Web_Stories\Services; use Google\Web_Stories\Settings; use Google\Web_Stories\Story_Post_Type; use Google\Web_Stories\Service_Base; @@ -139,7 +139,7 @@ public function add_amp_content_sanitizers( $sanitizers ) { return $sanitizers; } - $video_cache_enabled = (bool) get_option( Settings::SETTING_NAME_VIDEO_CACHE ); + $video_cache_enabled = (bool) Services::get( 'settings' )->get_setting( Settings::SETTING_NAME_VIDEO_CACHE ); $story = new Story(); $story->load_from_post( $post ); diff --git a/includes/Interfaces/Renderer.php b/includes/Interfaces/Renderer.php index edb105dc0644..40679b01f0e1 100644 --- a/includes/Interfaces/Renderer.php +++ b/includes/Interfaces/Renderer.php @@ -28,8 +28,6 @@ namespace Google\Web_Stories\Interfaces; -use Google\Web_Stories\Interfaces\FieldState; - /** * Interface Renderer. * diff --git a/includes/Media/Media_Source_Taxonomy.php b/includes/Media/Media_Source_Taxonomy.php index fb4cf8107cb7..4806f1eb5090 100644 --- a/includes/Media/Media_Source_Taxonomy.php +++ b/includes/Media/Media_Source_Taxonomy.php @@ -30,7 +30,6 @@ use Google\Web_Stories\Traits\Screen; use WP_Query; use WP_Post; -use WP_REST_Request; /** * Class Media_Source_Taxonomy diff --git a/includes/Migrations/Update_Publisher_Logos.php b/includes/Migrations/Update_Publisher_Logos.php index b37583dea121..73b030d1fda1 100644 --- a/includes/Migrations/Update_Publisher_Logos.php +++ b/includes/Migrations/Update_Publisher_Logos.php @@ -27,6 +27,7 @@ namespace Google\Web_Stories\Migrations; +use Google\Web_Stories\Services; use Google\Web_Stories\Settings; /** @@ -45,7 +46,7 @@ class Update_Publisher_Logos extends Migrate_Base { */ public function migrate() { $publisher_logo_id = 0; - $publisher_logo_settings = (array) get_option( Settings::SETTING_NAME_PUBLISHER_LOGOS ); + $publisher_logo_settings = (array) Services::get( 'settings' )->get_setting( Settings::SETTING_NAME_PUBLISHER_LOGOS, [] ); if ( ! empty( $publisher_logo_settings['active'] ) ) { $publisher_logo_id = $publisher_logo_settings['active']; diff --git a/includes/REST_API/Autosaves_Controller.php b/includes/REST_API/Autosaves_Controller.php index d3995ea90c0c..d0206876a85f 100644 --- a/includes/REST_API/Autosaves_Controller.php +++ b/includes/REST_API/Autosaves_Controller.php @@ -29,13 +29,10 @@ use Google\Web_Stories\Infrastructure\Delayed; use Google\Web_Stories\Infrastructure\Registerable; use Google\Web_Stories\Infrastructure\Service; -use Google\Web_Stories\Media\Image_Sizes; use Google\Web_Stories\Traits\Post_Type; -use WP_Error; use WP_Post; use WP_REST_Autosaves_Controller; use WP_REST_Controller; -use WP_REST_Posts_Controller; use WP_REST_Request; use WP_REST_Response; use WP_REST_Server; @@ -47,6 +44,7 @@ */ abstract class Autosaves_Controller extends WP_REST_Autosaves_Controller implements Service, Delayed, Registerable { use Post_Type; + /** * Parent post controller. * diff --git a/includes/REST_API/Embed_Controller.php b/includes/REST_API/Embed_Controller.php index 9a41271d4148..4ffd3a915ee0 100644 --- a/includes/REST_API/Embed_Controller.php +++ b/includes/REST_API/Embed_Controller.php @@ -32,6 +32,7 @@ use Google\Web_Stories\Traits\Post_Type; use WP_Error; use WP_Http; +use WP_Network; use WP_Post; use WP_REST_Request; use WP_REST_Response; @@ -237,7 +238,7 @@ private function url_to_post( $url ) { $path = explode( '/', ltrim( $url_parts['path'], '/' ) ); $path = count( $path ) > 2 ? reset( $path ) : false; $network = get_network(); - if ( $path && $network instanceof \WP_Network ) { + if ( $path && $network instanceof WP_Network ) { $qv['path'] = $network->path . $path . '/'; } } @@ -363,10 +364,7 @@ public function prepare_item_for_response( $embed, $request ) { $data = $this->add_additional_fields_to_object( $data, $request ); $data = $this->filter_response_by_context( $data, $context ); - // Wrap the data in a response object. - $response = rest_ensure_response( $data ); - - return $response; + return rest_ensure_response( $data ); } diff --git a/includes/REST_API/Link_Controller.php b/includes/REST_API/Link_Controller.php index 6d34e9cec328..4bca23d9d22b 100644 --- a/includes/REST_API/Link_Controller.php +++ b/includes/REST_API/Link_Controller.php @@ -44,6 +44,7 @@ */ class Link_Controller extends REST_Controller { use Document_Parser, Post_Type; + /** * Constructor. */ diff --git a/includes/REST_API/Status_Check_Controller.php b/includes/REST_API/Status_Check_Controller.php index 1dc9a3a998d5..1f66e4d25d9a 100644 --- a/includes/REST_API/Status_Check_Controller.php +++ b/includes/REST_API/Status_Check_Controller.php @@ -27,7 +27,6 @@ namespace Google\Web_Stories\REST_API; use Google\Web_Stories\Decoder; -use Google\Web_Stories\Experiments; use Google\Web_Stories\Story_Post_Type; use Google\Web_Stories\Traits\Post_Type; use WP_REST_Server; diff --git a/includes/REST_API/Template_Lock_Controller.php b/includes/REST_API/Template_Lock_Controller.php index 6c8ed78cb92a..e9e053023ea9 100644 --- a/includes/REST_API/Template_Lock_Controller.php +++ b/includes/REST_API/Template_Lock_Controller.php @@ -27,7 +27,6 @@ namespace Google\Web_Stories\REST_API; use Google\Web_Stories\Template_Post_Type; -use WP_REST_Controller; /** * Class Template_Lock_Controller diff --git a/includes/Renderer/Story/Embed.php b/includes/Renderer/Story/Embed.php index 59988ea07607..ace1674aa87c 100644 --- a/includes/Renderer/Story/Embed.php +++ b/includes/Renderer/Story/Embed.php @@ -28,7 +28,6 @@ namespace Google\Web_Stories\Renderer\Story; use Google\Web_Stories\Assets; -use Google\Web_Stories\Embed_Base; use Google\Web_Stories\Model\Story; use Google\Web_Stories\AMP_Story_Player_Assets; use Google\Web_Stories\Traits\Amp; diff --git a/includes/Renderer/Story/HTML.php b/includes/Renderer/Story/HTML.php index 542e04c4bd57..4adc0e3d5569 100644 --- a/includes/Renderer/Story/HTML.php +++ b/includes/Renderer/Story/HTML.php @@ -26,8 +26,6 @@ namespace Google\Web_Stories\Renderer\Story; -use Google\Web_Stories\Experiments; -use Google\Web_Stories\Settings; use Google\Web_Stories_Dependencies\AmpProject\Dom\Document; use Google\Web_Stories\Model\Story; diff --git a/includes/Settings.php b/includes/Settings.php index d5cde96c0848..81738f408ad8 100644 --- a/includes/Settings.php +++ b/includes/Settings.php @@ -282,4 +282,19 @@ public function register() { public static function get_registration_action_priority(): int { return 5; } + + /** + * Returns the value for a given setting. + * + * @SuppressWarnings(PHPMD.BooleanArgumentFlag) + * + * @since 1.12.0 + * + * @param string $key Setting key. + * @param mixed $default Optional. Default value to return if the option does not exist. + * @return mixed Setting value. + */ + public function get_setting( $key, $default = false ) { + return get_option( $key, $default ); + } } diff --git a/includes/Story_Post_Type.php b/includes/Story_Post_Type.php index 7274bbf8f826..11acd8f97d82 100644 --- a/includes/Story_Post_Type.php +++ b/includes/Story_Post_Type.php @@ -121,7 +121,7 @@ public function on_plugin_deactivation( $network_wide ) { * @return \WP_Post_Type|\WP_Error */ public function register_post_type() { - $has_archive = 'default' === get_option( Settings::SETTING_NAME_ARCHIVE, 'default' ); + $has_archive = 'default' === Services::get( 'settings' )->get_setting( Settings::SETTING_NAME_ARCHIVE, 'default' ); return register_post_type( self::POST_TYPE_SLUG, @@ -206,7 +206,7 @@ public function unregister_post_type() { * @return void */ protected function register_meta() { - $active_publisher_logo_id = absint( get_option( Settings::SETTING_NAME_ACTIVE_PUBLISHER_LOGO ) ); + $active_publisher_logo_id = absint( Services::get( 'settings' )->get_setting( Settings::SETTING_NAME_ACTIVE_PUBLISHER_LOGO, 0 ) ); register_post_meta( self::POST_TYPE_SLUG, diff --git a/includes/Tracking.php b/includes/Tracking.php index f8f7121516a7..b4f7dbe4a4a3 100644 --- a/includes/Tracking.php +++ b/includes/Tracking.php @@ -128,6 +128,17 @@ public static function get_registration_action(): string { return 'admin_init'; } + /** + * Get the list of service IDs required for this service to be registered. + * + * @since 1.12.0 + * + * @return string[] List of required services. + */ + public static function get_requirements(): array { + return [ 'user_preferences' ]; + } + /** * Returns tracking settings to pass to the inline script. * @@ -161,7 +172,7 @@ private function get_user_properties(): array { $site_kit_status = $this->site_kit->get_plugin_status(); $active_plugins = $site_kit_status['active'] ? 'google-site-kit' : ''; - $analytics = $site_kit_status['analyticsActive'] ? 'google-site-kit' : ! empty( get_option( Settings::SETTING_NAME_TRACKING_ID ) ); + $analytics = $site_kit_status['analyticsActive'] ? 'google-site-kit' : ! empty( Services::get( 'settings' )->get_setting( Settings::SETTING_NAME_TRACKING_ID ) ); return [ 'siteLocale' => get_locale(), @@ -171,7 +182,7 @@ private function get_user_properties(): array { 'wpVersion' => get_bloginfo( 'version' ), 'phpVersion' => PHP_VERSION, 'isMultisite' => (int) is_multisite(), - 'adNetwork' => (string) get_option( Settings::SETTING_NAME_AD_NETWORK, 'none' ), + 'adNetwork' => (string) Services::get( 'settings' )->get_setting( Settings::SETTING_NAME_AD_NETWORK, 'none' ), 'analytics' => $analytics, 'activePlugins' => $active_plugins, ]; @@ -183,6 +194,6 @@ private function get_user_properties(): array { * @return bool True if tracking enabled, and False if not. */ public function is_active(): bool { - return (bool) get_user_meta( get_current_user_id(), Preferences::OPTIN_META_KEY, true ); + return (bool) Services::get( 'user_preferences' )->get_preference( get_current_user_id(), Preferences::OPTIN_META_KEY ); } } diff --git a/includes/User/Capabilities.php b/includes/User/Capabilities.php index c9a307d23268..5e963fdc1924 100644 --- a/includes/User/Capabilities.php +++ b/includes/User/Capabilities.php @@ -192,4 +192,13 @@ protected function get_all_capabilities(): array { return $all_capabilities; } + + /** + * Get the list of service IDs required for this service to be registered. + * + * @return string[] List of required services. + */ + public static function get_requirements(): array { + return [ 'story_post_type', 'taxonomy.category', 'taxonomy.tag' ]; + } } diff --git a/includes/User/Preferences.php b/includes/User/Preferences.php index 8d0b831c35e6..50cef21eb75a 100644 --- a/includes/User/Preferences.php +++ b/includes/User/Preferences.php @@ -124,4 +124,18 @@ public function register() { public function can_edit_current_user( $allowed, $meta_key, $user_id, $current_user_id ): bool { return user_can( $current_user_id, 'edit_user', $user_id ); } + + /** + * Returns the specific preference for a givern user. + * + * @since 1.12.0 + * + * @param int $user_id User ID. + * @param string $key Preference key. + * + * @return mixed User preference value. + */ + public function get_preference( $user_id, $key ) { + return get_user_meta( $user_id, $key, true ); + } } diff --git a/includes/templates/frontend/single-web-story.php b/includes/templates/frontend/single-web-story.php index f5ec8f2203a2..3ec86a243337 100644 --- a/includes/templates/frontend/single-web-story.php +++ b/includes/templates/frontend/single-web-story.php @@ -10,7 +10,6 @@ use Google\Web_Stories\Renderer\Story\HTML; use Google\Web_Stories\Model\Story; -use Google\Web_Stories\Services; /** * Copyright 2020 Google LLC diff --git a/tests/phpunit/integration/includes/DependencyInjectedTestCase.php b/tests/phpunit/integration/includes/DependencyInjectedTestCase.php index a7ab3945965d..bec22206eccf 100644 --- a/tests/phpunit/integration/includes/DependencyInjectedTestCase.php +++ b/tests/phpunit/integration/includes/DependencyInjectedTestCase.php @@ -44,6 +44,12 @@ abstract class DependencyInjectedTestCase extends TestCase { public function setUp() { parent::setUp(); + // Needed because the block will exist already after hooking up the plugin + // on plugins_loaded. This avoids _doing_it_wrong messages + // due to registering the plugin (and thus the block) again. + // TODO: Figure out a better way. + unregister_block_type( 'web-stories/embed' ); + // We're intentionally avoiding the PluginFactory here as it uses a // static instance, because its whole point is to allow reuse across consumers. $this->plugin = new Plugin(); diff --git a/tests/phpunit/integration/tests/AMP/Output_Buffer.php b/tests/phpunit/integration/tests/AMP/Output_Buffer.php index 4986c9643d4d..7030d412c6e7 100644 --- a/tests/phpunit/integration/tests/AMP/Output_Buffer.php +++ b/tests/phpunit/integration/tests/AMP/Output_Buffer.php @@ -20,7 +20,6 @@ use Google\Web_Stories\Experiments; use Google\Web_Stories\Model\Story; use Google\Web_Stories\Renderer\Story\HTML; -use Google\Web_Stories\Settings; use Google\Web_Stories\Story_Post_Type; use Google\Web_Stories\AMP\Sanitization; use Google\Web_Stories\AMP\Optimization; diff --git a/tests/phpunit/integration/tests/AMP/Sanitization.php b/tests/phpunit/integration/tests/AMP/Sanitization.php index a797700f1725..25c68f6e10da 100644 --- a/tests/phpunit/integration/tests/AMP/Sanitization.php +++ b/tests/phpunit/integration/tests/AMP/Sanitization.php @@ -18,7 +18,6 @@ namespace Google\Web_Stories\Tests\Integration\AMP; use DOMElement; -use Google\Web_Stories\Experiments; use Google\Web_Stories_Dependencies\AMP_Dev_Mode_Sanitizer; use Google\Web_Stories_Dependencies\AMP_Layout_Sanitizer; use Google\Web_Stories_Dependencies\AMP_Style_Sanitizer; diff --git a/tests/phpunit/integration/tests/Admin/Cross_Origin_Isolation.php b/tests/phpunit/integration/tests/Admin/Cross_Origin_Isolation.php index ae5717772b45..7178499b43e2 100644 --- a/tests/phpunit/integration/tests/Admin/Cross_Origin_Isolation.php +++ b/tests/phpunit/integration/tests/Admin/Cross_Origin_Isolation.php @@ -17,12 +17,12 @@ namespace Google\Web_Stories\Tests\Integration\Admin; -use Google\Web_Stories\Tests\Integration\TestCase; +use Google\Web_Stories\Tests\Integration\DependencyInjectedTestCase; /** * @coversDefaultClass \Google\Web_Stories\Admin\Cross_Origin_Isolation */ -class Cross_Origin_Isolation extends TestCase { +class Cross_Origin_Isolation extends DependencyInjectedTestCase { /** * Admin user for test. * @@ -37,6 +37,13 @@ class Cross_Origin_Isolation extends TestCase { */ protected static $contributor_id; + /** + * Test instance. + * + * @var \Google\Web_Stories\Admin\Cross_Origin_Isolation + */ + private $instance; + public static function wpSetUpBeforeClass( $factory ) { self::$admin_id = $factory->user->create( [ 'role' => 'administrator' ] @@ -55,8 +62,7 @@ public static function wpTearDownAfterClass() { public function setUp() { parent::setUp(); - $user_preferences = new \Google\Web_Stories\User\Preferences(); - $user_preferences->register(); + $this->instance = $this->injector->make( \Google\Web_Stories\Admin\Cross_Origin_Isolation::class ); } /** @@ -68,15 +74,14 @@ public function test_register() { $GLOBALS['current_screen'] = convert_to_screen( \Google\Web_Stories\Story_Post_Type::POST_TYPE_SLUG ); - $instance = new \Google\Web_Stories\Admin\Cross_Origin_Isolation(); - $instance->register(); + $this->instance->register(); - $this->assertSame( 10, has_action( 'load-post.php', [ $instance, 'admin_header' ] ) ); - $this->assertSame( 10, has_action( 'load-post-new.php', [ $instance, 'admin_header' ] ) ); + $this->assertSame( 10, has_action( 'load-post.php', [ $this->instance, 'admin_header' ] ) ); + $this->assertSame( 10, has_action( 'load-post-new.php', [ $this->instance, 'admin_header' ] ) ); - $this->assertSame( 10, has_filter( 'style_loader_tag', [ $instance, 'style_loader_tag' ] ) ); - $this->assertSame( 10, has_filter( 'script_loader_tag', [ $instance, 'script_loader_tag' ] ) ); - $this->assertSame( 10, has_filter( 'get_avatar', [ $instance, 'get_avatar' ] ) ); + $this->assertSame( 10, has_filter( 'style_loader_tag', [ $this->instance, 'style_loader_tag' ] ) ); + $this->assertSame( 10, has_filter( 'script_loader_tag', [ $this->instance, 'script_loader_tag' ] ) ); + $this->assertSame( 10, has_filter( 'get_avatar', [ $this->instance, 'get_avatar' ] ) ); delete_user_meta( self::$admin_id, \Google\Web_Stories\User\Preferences::MEDIA_OPTIMIZATION_META_KEY ); } @@ -87,9 +92,8 @@ public function test_register() { public function test_is_needed() { wp_set_current_user( self::$admin_id ); update_user_meta( self::$admin_id, \Google\Web_Stories\User\Preferences::MEDIA_OPTIMIZATION_META_KEY, true ); - $instance = new \Google\Web_Stories\Admin\Cross_Origin_Isolation(); - $result = $this->call_private_method( $instance, 'is_needed' ); - $this->assertTrue( $result ); + + $this->assertTrue( \Google\Web_Stories\Admin\Cross_Origin_Isolation::is_needed() ); } /** @@ -98,18 +102,15 @@ public function test_is_needed() { public function test_is_needed_default_user_meta_value() { wp_set_current_user( self::$admin_id ); delete_user_meta( self::$admin_id, \Google\Web_Stories\User\Preferences::MEDIA_OPTIMIZATION_META_KEY ); - $instance = new \Google\Web_Stories\Admin\Cross_Origin_Isolation(); - $result = $this->call_private_method( $instance, 'is_needed' ); - $this->assertTrue( $result ); + + $this->assertTrue( \Google\Web_Stories\Admin\Cross_Origin_Isolation::is_needed() ); } /** * @covers ::is_needed */ public function test_is_needed_no_user() { - $instance = new \Google\Web_Stories\Admin\Cross_Origin_Isolation(); - $result = $this->call_private_method( $instance, 'is_needed' ); - $this->assertFalse( $result ); + $this->assertFalse( \Google\Web_Stories\Admin\Cross_Origin_Isolation::is_needed() ); } /** @@ -118,9 +119,8 @@ public function test_is_needed_no_user() { public function test_is_needed_opt_out() { wp_set_current_user( self::$admin_id ); update_user_meta( self::$admin_id, \Google\Web_Stories\User\Preferences::MEDIA_OPTIMIZATION_META_KEY, false ); - $instance = new \Google\Web_Stories\Admin\Cross_Origin_Isolation(); - $result = $this->call_private_method( $instance, 'is_needed' ); - $this->assertFalse( $result ); + + $this->assertFalse( \Google\Web_Stories\Admin\Cross_Origin_Isolation::is_needed() ); } /** @@ -129,9 +129,8 @@ public function test_is_needed_opt_out() { public function test_is_needed_no_upload_caps() { wp_set_current_user( self::$contributor_id ); update_user_meta( self::$contributor_id, \Google\Web_Stories\User\Preferences::MEDIA_OPTIMIZATION_META_KEY, true ); - $instance = new \Google\Web_Stories\Admin\Cross_Origin_Isolation(); - $result = $this->call_private_method( $instance, 'is_needed' ); - $this->assertFalse( $result ); + + $this->assertFalse( \Google\Web_Stories\Admin\Cross_Origin_Isolation::is_needed() ); } /** @@ -139,11 +138,10 @@ public function test_is_needed_no_upload_caps() { * @covers ::starts_with */ public function test_add_attribute() { - $instance = new \Google\Web_Stories\Admin\Cross_Origin_Isolation(); $html = "test"; $attribute = 'src'; $url = 'http://www.google.com/test.jpg'; - $result = $this->call_private_method( $instance, 'add_attribute', [ $html, $attribute, $url ] ); + $result = $this->call_private_method( $this->instance, 'add_attribute', [ $html, $attribute, $url ] ); $this->assertContains( 'crossorigin', $result ); } /** @@ -151,12 +149,10 @@ public function test_add_attribute() { * @covers ::starts_with */ public function test_add_attribute_local_image() { - $instance = new \Google\Web_Stories\Admin\Cross_Origin_Isolation(); - $url = site_url( '/test.jpg' ); $html = sprintf( "test", $url ); $attribute = 'src'; - $result = $this->call_private_method( $instance, 'add_attribute', [ $html, $attribute, $url ] ); + $result = $this->call_private_method( $this->instance, 'add_attribute', [ $html, $attribute, $url ] ); $this->assertNotContains( 'crossorigin', $result ); } @@ -165,12 +161,10 @@ public function test_add_attribute_local_image() { * @covers ::starts_with */ public function test_add_attribute_relative_image() { - $instance = new \Google\Web_Stories\Admin\Cross_Origin_Isolation(); - $html = "test"; $attribute = 'src'; $url = '/test.jpg'; - $result = $this->call_private_method( $instance, 'add_attribute', [ $html, $attribute, $url ] ); + $result = $this->call_private_method( $this->instance, 'add_attribute', [ $html, $attribute, $url ] ); $this->assertNotContains( 'crossorigin', $result ); } @@ -178,11 +172,9 @@ public function test_add_attribute_relative_image() { * @covers ::starts_with */ public function test_starts_with() { - $instance = new \Google\Web_Stories\Admin\Cross_Origin_Isolation(); - $string = 'hello world'; $start_string = 'hello'; - $result = $this->call_private_method( $instance, 'starts_with', [ $string, $start_string ] ); + $result = $this->call_private_method( $this->instance, 'starts_with', [ $string, $start_string ] ); $this->assertTrue( $result ); } @@ -190,11 +182,9 @@ public function test_starts_with() { * @covers ::starts_with */ public function test_starts_with_fail() { - $instance = new \Google\Web_Stories\Admin\Cross_Origin_Isolation(); - $string = 'hello world'; $start_string = 'world'; - $result = $this->call_private_method( $instance, 'starts_with', [ $string, $start_string ] ); + $result = $this->call_private_method( $this->instance, 'starts_with', [ $string, $start_string ] ); $this->assertFalse( $result ); } @@ -207,8 +197,7 @@ public function test_starts_with_fail() { public function test_is_edit_screen() { $GLOBALS['current_screen'] = convert_to_screen( \Google\Web_Stories\Story_Post_Type::POST_TYPE_SLUG ); - $instance = new \Google\Web_Stories\Admin\Cross_Origin_Isolation(); - $result = $this->call_private_method( $instance, 'is_edit_screen' ); + $result = $this->call_private_method( $this->instance, 'is_edit_screen' ); $this->assertTrue( $result ); } @@ -217,8 +206,7 @@ public function test_is_edit_screen() { * @covers \Google\Web_Stories\Traits\Screen::get_current_screen */ public function test_get_current_screen() { - $instance = new \Google\Web_Stories\Admin\Cross_Origin_Isolation(); - $result = $this->call_private_method( $instance, 'get_current_screen' ); + $result = $this->call_private_method( $this->instance, 'get_current_screen' ); $this->assertFalse( $result ); } @@ -228,10 +216,9 @@ public function test_get_current_screen() { public function test_replace_in_dom() { $site_url = site_url(); - $html = file_get_contents( WEB_STORIES_TEST_DATA_DIR . '/cross_origin_content.html' ); - $html = str_replace( '--SITE_URL--', $site_url, $html ); - $instance = new \Google\Web_Stories\Admin\Cross_Origin_Isolation(); - $result = $this->call_private_method( $instance, 'replace_in_dom', [ $html ] ); + $html = file_get_contents( WEB_STORIES_TEST_DATA_DIR . '/cross_origin_content.html' ); + $html = str_replace( '--SITE_URL--', $site_url, $html ); + $result = $this->call_private_method( $this->instance, 'replace_in_dom', [ $html ] ); $this->assertContains( '', $result ); $this->assertContains( '', $result ); @@ -259,9 +246,8 @@ public function test_replace_in_dom() { * @covers ::replace_in_dom */ public function test_replace_in_dom_invalid() { - $html = 'call_private_method( $instance, 'replace_in_dom', [ $html ] ); + $html = 'call_private_method( $this->instance, 'replace_in_dom', [ $html ] ); $this->assertContains( '', $result ); } @@ -272,8 +258,7 @@ public function test_replace_in_dom_invalid() { */ public function test_custom_print_media_templates() { require_once ABSPATH . WPINC . '/media-template.php'; - $instance = new \Google\Web_Stories\Admin\Cross_Origin_Isolation(); - $output = get_echo( [ $instance, 'custom_print_media_templates' ] ); + $output = get_echo( [ $this->instance, 'custom_print_media_templates' ] ); $this->assertContains( '