Guard against infinite recursion error when validating AMP pages usin…

…g a Reader theme (#5410) Co-authored-by: Alain Schlesser <alain.schlesser@gmail.com>
ampproject · Sep 22, 2020 · 307028c · 307028c
1 parent b9d9d30
commit 307028c
Show file tree

Hide file tree

Showing 24 changed files with 357 additions and 113 deletions.
diff --git a/.phpstorm.meta.php b/.phpstorm.meta.php
@@ -35,4 +35,10 @@
 			'validated_url_stylesheet_gc'       => \AmpProject\AmpWP\BackgroundTask\ValidatedUrlStylesheetDataGarbageCollection::class,
 		] )
 	);
+
+	// For the injector, the return type should be the same as what the provided FQCN represents.
+	override(
+		\AmpProject\AmpWP\Infrastructure\Injector::make(),
+		map( [ '' => '@' ] )
+	);
 }
diff --git a/includes/admin/class-amp-template-customizer.php b/includes/admin/class-amp-template-customizer.php
@@ -71,12 +71,14 @@ protected function __construct( WP_Customize_Manager $wp_customize, ReaderThemeL
 	 * @since 0.4
 	 * @access public
 	 *
-	 * @param WP_Customize_Manager $wp_customize Customizer instance.
+	 * @param WP_Customize_Manager $wp_customize        Customizer instance.
+	 * @param ReaderThemeLoader    $reader_theme_loader Reader theme loader.
 	 * @return AMP_Template_Customizer Instance.
 	 */
-	public static function init( WP_Customize_Manager $wp_customize ) {
-		$reader_theme_loader = Services::get( 'reader_theme_loader' );
-
+	public static function init( WP_Customize_Manager $wp_customize, ReaderThemeLoader $reader_theme_loader = null ) {
+		if ( null === $reader_theme_loader ) {
+			$reader_theme_loader = Services::get( 'reader_theme_loader' );
+		}
 		$self = new self( $wp_customize, $reader_theme_loader );
 
 		$is_reader_mode   = ( AMP_Theme_Support::READER_MODE_SLUG === AMP_Options_Manager::get_option( Option::THEME_SUPPORT ) );

diff --git a/phpstan.neon.dist b/phpstan.neon.dist
@@ -2,6 +2,10 @@ includes:
 	# @see https://github.com/phpstan/phpstan-src/blob/b9f62d63f2deaa0a5e97f51073e41a422c48aa01/conf/bleedingEdge.neon
 	- phar://phpstan.phar/conf/bleedingEdge.neon
 services:
+	-
+		class: AmpProject\AmpWP\Tests\PhpStan\ServiceContainerDynamicReturnTypeExtension
+		tags:
+			- phpstan.broker.dynamicMethodReturnTypeExtension
 	-
 		class: AmpProject\AmpWP\Tests\PhpStan\ServicesDynamicReturnTypeExtension
 		tags:

diff --git a/src/AmpWpPlugin.php b/src/AmpWpPlugin.php
@@ -160,6 +160,7 @@ protected function get_shared_instances() {
 			Instrumentation\StopWatch::class,
 			DevTools\CallbackReflection::class,
 			DevTools\FileReflection::class,
+			ReaderThemeLoader::class,
 		];
 	}
 

diff --git a/src/DevTools/FileReflection.php b/src/DevTools/FileReflection.php
@@ -168,9 +168,18 @@ public function reset_theme_variables() {
 	 * }
 	 */
 	public function get_file_source( $file ) {
+		static $recursion_protection = false;
+
+		if ( $recursion_protection ) {
+			return [];
+		}
+
+		$recursion_protection = true;
+
 		$matches = [];
 
 		if ( $this->is_parent_theme_file( $file, $matches ) ) {
+			$recursion_protection = false;
 			return $this->get_file_source_array(
 				self::TYPE_THEME,
 				$this->get_template_slug(),
@@ -179,6 +188,7 @@ public function get_file_source( $file ) {
 		}
 
 		if ( $this->is_child_theme_file( $file, $matches ) ) {
+			$recursion_protection = false;
 			return $this->get_file_source_array(
 				self::TYPE_THEME,
 				$this->get_stylesheet_slug(),
@@ -187,6 +197,7 @@ public function get_file_source( $file ) {
 		}
 
 		if ( $this->is_plugin_file( $file, $matches ) ) {
+			$recursion_protection = false;
 			return $this->get_file_source_array(
 				self::TYPE_PLUGIN,
 				$matches['slug'],
@@ -195,6 +206,7 @@ public function get_file_source( $file ) {
 		}
 
 		if ( $this->is_mu_plugin_file( $file, $matches ) ) {
+			$recursion_protection = false;
 			return $this->get_file_source_array(
 				self::TYPE_MU_PLUGIN,
 				$matches['slug'],
@@ -203,13 +215,15 @@ public function get_file_source( $file ) {
 		}
 
 		if ( $this->is_core_file( $file, $matches ) ) {
+			$recursion_protection = false;
 			return $this->get_file_source_array(
 				self::TYPE_CORE,
 				$matches['slug'],
 				$matches['file']
 			);
 		}
 
+		$recursion_protection = false;
 		return [];
 	}
 

diff --git a/src/ReaderThemeLoader.php b/src/ReaderThemeLoader.php
@@ -128,7 +128,12 @@ public function filter_wp_prepare_themes_to_indicate_reader_theme( $prepared_the
 			return $prepared_themes;
 		}
 
-		$reader_theme = AMP_Options_Manager::get_option( Option::READER_THEME );
+		$reader_theme_obj = $this->get_reader_theme();
+		if ( ! $reader_theme_obj instanceof WP_Theme ) {
+			return $prepared_themes;
+		}
+		$reader_theme = $reader_theme_obj->get_stylesheet();
+
 		if ( isset( $prepared_themes[ $reader_theme ] ) ) {
 
 			// Make sure the AMP Reader theme appears right after the active theme in the list.
@@ -164,7 +169,7 @@ public function filter_wp_prepare_themes_to_indicate_reader_theme( $prepared_the
 						'a' => [ 'href' => true ],
 					]
 				),
-				esc_url( add_query_arg( 'page', AMP_Options_Manager::OPTION_NAME, admin_url( 'admin.php' ) ) )
+				esc_url( add_query_arg( 'page', AMP_Options_Manager::OPTION_NAME, admin_url( 'admin.php' ) ) . '#reader-themes' )
 			);
 		}
 
@@ -181,7 +186,11 @@ public function inject_theme_single_template_modifications() {
 			return;
 		}
 
-		$reader_theme = AMP_Options_Manager::get_option( Option::READER_THEME );
+		$reader_theme = $this->get_reader_theme();
+		if ( ! $reader_theme instanceof WP_Theme ) {
+			return;
+		}
+
 		?>
 		<script>
 			(function( themeSingleTmpl ) {
@@ -249,7 +258,7 @@ function ( match, startDiv ) {
 				}
 			}) (
 				document.getElementById( 'tmpl-theme' ),
-				document.querySelector( <?php echo wp_json_encode( sprintf( '#%s-name > span', $reader_theme ) ); ?> )
+				document.querySelector( <?php echo wp_json_encode( sprintf( '#%s-name > span', $reader_theme->get_stylesheet() ) ); ?> )
 			);
 		</script>
 		<?php
@@ -264,6 +273,10 @@ function ( match, startDiv ) {
 	 * @return WP_Theme|null Theme if selected and no errors.
 	 */
 	public function get_reader_theme() {
+		if ( $this->reader_theme instanceof WP_Theme ) {
+			return $this->reader_theme;
+		}
+
 		$reader_theme_slug = AMP_Options_Manager::get_option( Option::READER_THEME );
 		if ( ! $reader_theme_slug ) {
 			return null;

diff --git a/src/Services.php b/src/Services.php
@@ -7,6 +7,8 @@
 
 namespace AmpProject\AmpWP;
 
+use AmpProject\AmpWP\Infrastructure\Injector;
+use AmpProject\AmpWP\Infrastructure\Plugin;
 use AmpProject\AmpWP\Infrastructure\Service;
 use AmpProject\AmpWP\Infrastructure\ServiceContainer;
 
@@ -22,6 +24,27 @@
  */
 final class Services {
 
+	/**
+	 * Plugin object instance.
+	 *
+	 * @var Plugin
+	 */
+	private static $plugin;
+
+	/**
+	 * Service container object instance.
+	 *
+	 * @var ServiceContainer
+	 */
+	private static $container;
+
+	/**
+	 * Dependency injector object instance.
+	 *
+	 * @var Injector
+	 */
+	private static $injector;
+
 	/**
 	 * Get a particular service out of the service container.
 	 *
@@ -32,18 +55,42 @@ public static function get( $service ) {
 		return self::get_container()->get( $service );
 	}
 
+	/**
+	 * Get an instance of the plugin.
+	 *
+	 * @return Plugin Plugin object instance.
+	 */
+	public static function get_plugin() {
+		if ( null === self::$plugin ) {
+			self::$plugin = AmpWpPluginFactory::create();
+		}
+
+		return self::$plugin;
+	}
+
 	/**
 	 * Get an instance of the service container.
 	 *
-	 * @return ServiceContainer
+	 * @return ServiceContainer Service container object instance.
 	 */
 	public static function get_container() {
-		static $container = null;
+		if ( null === self::$container ) {
+			self::$container = self::get_plugin()->get_container();
+		}
+
+		return self::$container;
+	}
 
-		if ( null === $container ) {
-			$container = AmpWpPluginFactory::create()->get_container();
+	/**
+	 * Get an instance of the dependency injector.
+	 *
+	 * @return Injector Dependency injector object instance.
+	 */
+	public static function get_injector() {
+		if ( null === self::$injector ) {
+			self::$injector = self::get_container()->get( 'injector' );
 		}
 
-		return $container;
+		return self::$injector;
 	}
 }
diff --git a/tests/php/src/DependencyInjectedTestCase.php b/tests/php/src/DependencyInjectedTestCase.php
@@ -0,0 +1,68 @@
+<?php
+
+namespace AmpProject\AmpWP\Tests;
+
+use AmpProject\AmpWP\AmpWpPlugin;
+use AmpProject\AmpWP\Infrastructure\Injector;
+use AmpProject\AmpWP\Infrastructure\ServiceContainer;
+use AmpProject\AmpWP\Services;
+use AmpProject\AmpWP\Tests\Helpers\PrivateAccess;
+use WP_UnitTestCase;
+
+abstract class DependencyInjectedTestCase extends WP_UnitTestCase {
+
+	use PrivateAccess;
+
+	/**
+	 * Plugin instance to test with.
+	 *
+	 * @var AmpWpPlugin
+	 */
+	protected $plugin;
+
+	/**
+	 * Service container instance to test with.
+	 *
+	 * @var ServiceContainer
+	 */
+	protected $container;
+
+	/**
+	 * Injector instance to test with.
+	 *
+	 * @var Injector
+	 */
+	protected $injector;
+
+	/**
+	 * Set up the service architecture before each test run.
+	 */
+	public function setUp() {
+		parent::setUp();
+
+		// We're intentionally avoiding the AmpWpPluginFactory here as it uses a
+		// static instance, because its whole point is to allow reuse across consumers.
+		$this->plugin = new AmpWpPlugin();
+		$this->plugin->register();
+
+		$this->container = $this->plugin->get_container();
+		$this->injector  = $this->container->get( 'injector' );
+
+		// The static Services helper has to be modified to use the same objects
+		// as the ones that are injected into the tests.
+		$this->set_private_property( Services::class, 'plugin', $this->plugin );
+		$this->set_private_property( Services::class, 'container', $this->container );
+		$this->set_private_property( Services::class, 'injector', $this->injector );
+	}
+
+	/**
+	 * Clean up again after each test run.
+	 */
+	public function tearDown() {
+		parent::tearDown();
+
+		$this->set_private_property( Services::class, 'plugin', null );
+		$this->set_private_property( Services::class, 'container', null );
+		$this->set_private_property( Services::class, 'injector', null );
+	}
+}
diff --git a/tests/php/src/DevTools/CallbackReflectionTest.php b/tests/php/src/DevTools/CallbackReflectionTest.php
@@ -9,11 +9,9 @@
 
 use AmpProject\AmpWP\DevTools\CallbackReflection;
 use AmpProject\AmpWP\DevTools\FileReflection;
-use AmpProject\AmpWP\PluginRegistry;
-use AmpProject\AmpWP\Services;
+use AmpProject\AmpWP\Tests\DependencyInjectedTestCase;
 use ReflectionFunction;
 use ReflectionMethod;
-use WP_UnitTestCase;
 use AmpProject\AmpWP\Tests\Helpers\LoadsCoreThemes;
 
 /**
@@ -23,7 +21,7 @@
  *
  * @coversDefaultClass \AmpProject\AmpWP\DevTools\CallbackReflection
  */
-class CallbackReflectionTest extends WP_UnitTestCase {
+class CallbackReflectionTest extends DependencyInjectedTestCase {
 
 	use LoadsCoreThemes;
 
@@ -39,10 +37,7 @@ public function setUp() {
 
 		$this->register_core_themes();
 
-		$plugin_registry = new PluginRegistry();
-		$file_reflection = new FileReflection( $plugin_registry );
-		$file_reflection->register();
-		$this->callback_reflection = new CallbackReflection( $file_reflection );
+		$this->callback_reflection = $this->injector->make( CallbackReflection::class );
 
 		$theme_root = dirname( dirname( __DIR__ ) ) . '/data/themes';
 		add_filter(

diff --git a/tests/php/src/DevTools/ErrorPageTest.php b/tests/php/src/DevTools/ErrorPageTest.php
@@ -2,12 +2,12 @@
 
 namespace AmpProject\AmpWP\Tests\DevTools;
 
-use AmpProject\AmpWP\Services;
+use AmpProject\AmpWP\DevTools\ErrorPage;
+use AmpProject\AmpWP\Tests\DependencyInjectedTestCase;
 use AmpProject\AmpWP\Tests\Helpers\AssertContainsCompatibility;
 use RuntimeException;
-use WP_UnitTestCase;
 
-final class ErrorPageTest extends WP_UnitTestCase {
+final class ErrorPageTest extends DependencyInjectedTestCase {
 	use AssertContainsCompatibility;
 
 	public function test_error_page_output() {
@@ -18,7 +18,7 @@ public function test_error_page_output() {
 			stream_get_meta_data( $capture )['uri']
 		);
 
-		$output = Services::get( 'dev_tools.error_page' )
+		$output = $this->injector->make( ErrorPage::class )
 			->with_title( 'Error Page Title' )
 			->with_message( 'Error Page Message' )
 			->with_exception( new RuntimeException( 'FAILURE', 42 ) )