From 3418b71ec54fcde45af374d996fbc3e0b850f354 Mon Sep 17 00:00:00 2001 From: Thijs van der heijden Date: Mon, 27 Jul 2026 14:02:01 +0200 Subject: [PATCH 1/5] refactor(schema-aggregator): extract the schema map XML provider The XML REST route was the only place performing the read-through-cache sequence for the schema map. A second consumer is about to need it, so the sequence moves into Schema_Map_Xml_Provider rather than being duplicated. This also gives the sequence its own unit tests; it previously had none, since the route only exercised it through the WP integration test. Co-Authored-By: Claude Opus 5 (1M context) --- .../schema_map/schema-map-xml-provider.php | 73 +++++++++++++++++ .../site-schema-aggregator-xml-route.php | 60 +++----------- .../Abstract_Schema_Map_Xml_Provider_Test.php | 68 ++++++++++++++++ .../Constructor_Test.php | 38 +++++++++ .../Schema_Map_Xml_Provider/Get_Xml_Test.php | 61 ++++++++++++++ ..._Site_Schema_Aggregator_Xml_Route_Test.php | 34 ++------ .../Site_Schema_Aggregator_Xml_Route_Test.php | 79 ++----------------- 7 files changed, 263 insertions(+), 150 deletions(-) create mode 100644 src/schema-aggregator/application/schema_map/schema-map-xml-provider.php create mode 100644 tests/Unit/Schema_Aggregator/Application/Schema_Map/Schema_Map_Xml_Provider/Abstract_Schema_Map_Xml_Provider_Test.php create mode 100644 tests/Unit/Schema_Aggregator/Application/Schema_Map/Schema_Map_Xml_Provider/Constructor_Test.php create mode 100644 tests/Unit/Schema_Aggregator/Application/Schema_Map/Schema_Map_Xml_Provider/Get_Xml_Test.php diff --git a/src/schema-aggregator/application/schema_map/schema-map-xml-provider.php b/src/schema-aggregator/application/schema_map/schema-map-xml-provider.php new file mode 100644 index 00000000000..bf9c1574375 --- /dev/null +++ b/src/schema-aggregator/application/schema_map/schema-map-xml-provider.php @@ -0,0 +1,73 @@ +aggregate_site_schema_map_command_handler = $aggregate_site_schema_map_command_handler; + $this->xml_cache_manager = $xml_cache_manager; + $this->aggregator_config = $aggregator_config; + } + + /** + * Returns the schema map XML, building and caching it when it is not cached yet. + * + * @return string The schema map XML. + */ + public function get_xml(): string { + $cached_xml = $this->xml_cache_manager->get(); + if ( $cached_xml !== null ) { + return $cached_xml; + } + + $command = new Aggregate_Site_Schema_Map_Command( $this->aggregator_config->get_allowed_post_types() ); + $xml = $this->aggregate_site_schema_map_command_handler->handle( $command ); + + $this->xml_cache_manager->set( $xml ); + + return $xml; + } +} diff --git a/src/schema-aggregator/user-interface/site-schema-aggregator-xml-route.php b/src/schema-aggregator/user-interface/site-schema-aggregator-xml-route.php index a6e30740a22..bdbfcf0f548 100644 --- a/src/schema-aggregator/user-interface/site-schema-aggregator-xml-route.php +++ b/src/schema-aggregator/user-interface/site-schema-aggregator-xml-route.php @@ -2,15 +2,10 @@ // phpcs:disable Yoast.NamingConventions.NamespaceName.TooLong -- Needed in the folder structure. namespace Yoast\WP\SEO\Schema_Aggregator\User_Interface; -use WP_Error; use WP_REST_Response; use Yoast\WP\SEO\Main; use Yoast\WP\SEO\Routes\Route_Interface; -use Yoast\WP\SEO\Schema_Aggregator\Application\Aggregate_Site_Schema_Map_Command; -use Yoast\WP\SEO\Schema_Aggregator\Application\Aggregate_Site_Schema_Map_Command_Handler; -use Yoast\WP\SEO\Schema_Aggregator\Application\Cache\Xml_Manager; -use Yoast\WP\SEO\Schema_Aggregator\Infrastructure\Aggregator_Config; -use Yoast\WP\SEO\Schema_Aggregator\Infrastructure\Config; +use Yoast\WP\SEO\Schema_Aggregator\Application\Schema_Map\Schema_Map_Xml_Provider; use Yoast\WP\SEO\Schema_Aggregator\Infrastructure\Schema_Aggregator_Conditional; /** @@ -34,25 +29,11 @@ class Site_Schema_Aggregator_Xml_Route implements Route_Interface { public const GET_SCHEMA_ROUTE = self::ROUTE_PREFIX . '/get-xml'; /** - * The command handler instance. + * The schema map XML provider. * - * @var Aggregate_Site_Schema_Map_Command_Handler + * @var Schema_Map_Xml_Provider */ - private $aggregate_site_schema_map_command_handler; - - /** - * The XML cache manager instance. - * - * @var Xml_Manager - */ - private $xml_cache_manager; - - /** - * The aggregator configuration instance. - * - * @var Aggregator_Config - */ - private $aggregator_config; + private $schema_map_xml_provider; /** * Returns the conditional for this route. @@ -66,20 +47,10 @@ public static function get_conditionals() { /** * Site_Schema_Aggregator_Route constructor. * - * @param Aggregate_Site_Schema_Map_Command_Handler $aggregate_site_schema_map_command_handler The command handler. - * @param Xml_Manager $xml_cache_manager The XML cache - * manager. - * @param Aggregator_Config $aggregator_config The aggregator - * configuration. + * @param Schema_Map_Xml_Provider $schema_map_xml_provider The schema map XML provider. */ - public function __construct( - Aggregate_Site_Schema_Map_Command_Handler $aggregate_site_schema_map_command_handler, - Xml_Manager $xml_cache_manager, - Aggregator_Config $aggregator_config - ) { - $this->aggregate_site_schema_map_command_handler = $aggregate_site_schema_map_command_handler; - $this->xml_cache_manager = $xml_cache_manager; - $this->aggregator_config = $aggregator_config; + public function __construct( Schema_Map_Xml_Provider $schema_map_xml_provider ) { + $this->schema_map_xml_provider = $schema_map_xml_provider; } /** @@ -111,23 +82,10 @@ public function get_permission_callback(): bool { /** * Returns a XML representation of the possible post types that can be used for schema. * - * @return WP_REST_Response|WP_Error The success or failure response. + * @return WP_REST_Response The response. */ public function render_schema_xml() { - $cached_xml = $this->xml_cache_manager->get(); - if ( $cached_xml !== null ) { - $xml = $cached_xml; - } - else { - - $post_types = $this->aggregator_config->get_allowed_post_types(); - - $command = new Aggregate_Site_Schema_Map_Command( $post_types ); - $xml = $this->aggregate_site_schema_map_command_handler->handle( $command ); - - $this->xml_cache_manager->set( $xml ); - } - $response = new WP_REST_Response( $xml, 200 ); + $response = new WP_REST_Response( $this->schema_map_xml_provider->get_xml(), 200 ); $response->header( 'Content-Type', 'application/xml; charset=UTF-8' ); $response->header( 'Cache-Control', 'public, max-age=300' ); diff --git a/tests/Unit/Schema_Aggregator/Application/Schema_Map/Schema_Map_Xml_Provider/Abstract_Schema_Map_Xml_Provider_Test.php b/tests/Unit/Schema_Aggregator/Application/Schema_Map/Schema_Map_Xml_Provider/Abstract_Schema_Map_Xml_Provider_Test.php new file mode 100644 index 00000000000..1bf46d32094 --- /dev/null +++ b/tests/Unit/Schema_Aggregator/Application/Schema_Map/Schema_Map_Xml_Provider/Abstract_Schema_Map_Xml_Provider_Test.php @@ -0,0 +1,68 @@ +command_handler = Mockery::mock( Aggregate_Site_Schema_Map_Command_Handler::class ); + $this->xml_cache_manager = Mockery::mock( Xml_Manager::class ); + $this->aggregator_config = Mockery::mock( Aggregator_Config::class ); + + $this->instance = new Schema_Map_Xml_Provider( + $this->command_handler, + $this->xml_cache_manager, + $this->aggregator_config, + ); + } +} diff --git a/tests/Unit/Schema_Aggregator/Application/Schema_Map/Schema_Map_Xml_Provider/Constructor_Test.php b/tests/Unit/Schema_Aggregator/Application/Schema_Map/Schema_Map_Xml_Provider/Constructor_Test.php new file mode 100644 index 00000000000..cc39a7b6de3 --- /dev/null +++ b/tests/Unit/Schema_Aggregator/Application/Schema_Map/Schema_Map_Xml_Provider/Constructor_Test.php @@ -0,0 +1,38 @@ +assertInstanceOf( + Aggregate_Site_Schema_Map_Command_Handler::class, + $this->getPropertyValue( $this->instance, 'aggregate_site_schema_map_command_handler' ), + ); + $this->assertInstanceOf( + Xml_Manager::class, + $this->getPropertyValue( $this->instance, 'xml_cache_manager' ), + ); + $this->assertInstanceOf( + Aggregator_Config::class, + $this->getPropertyValue( $this->instance, 'aggregator_config' ), + ); + } +} diff --git a/tests/Unit/Schema_Aggregator/Application/Schema_Map/Schema_Map_Xml_Provider/Get_Xml_Test.php b/tests/Unit/Schema_Aggregator/Application/Schema_Map/Schema_Map_Xml_Provider/Get_Xml_Test.php new file mode 100644 index 00000000000..8b920f1ac5c --- /dev/null +++ b/tests/Unit/Schema_Aggregator/Application/Schema_Map/Schema_Map_Xml_Provider/Get_Xml_Test.php @@ -0,0 +1,61 @@ +'; + + $this->xml_cache_manager->expects( 'get' )->once()->andReturn( $cached_xml ); + $this->aggregator_config->expects( 'get_allowed_post_types' )->never(); + $this->command_handler->expects( 'handle' )->never(); + $this->xml_cache_manager->expects( 'set' )->never(); + + $this->assertSame( $cached_xml, $this->instance->get_xml() ); + } + + /** + * Tests that the XML is built and cached when there is no cached XML. + * + * @return void + */ + public function test_get_xml_builds_and_caches_on_a_cache_miss() { + $built_xml = ''; + + $this->xml_cache_manager->expects( 'get' )->once()->andReturn( null ); + $this->aggregator_config->expects( 'get_allowed_post_types' )->once()->andReturn( [ 'post', 'page' ] ); + + $this->command_handler->expects( 'handle' ) + ->once() + ->with( + Mockery::on( + static function ( $command ) { + return $command instanceof Aggregate_Site_Schema_Map_Command + && $command->get_post_types() === [ 'post', 'page' ]; + }, + ), + ) + ->andReturn( $built_xml ); + + $this->xml_cache_manager->expects( 'set' )->once()->with( $built_xml )->andReturn( true ); + + $this->assertSame( $built_xml, $this->instance->get_xml() ); + } +} diff --git a/tests/Unit/Schema_Aggregator/User_Interface/Site_Schema_Aggregator_XML_Route/Abstract_Site_Schema_Aggregator_Xml_Route_Test.php b/tests/Unit/Schema_Aggregator/User_Interface/Site_Schema_Aggregator_XML_Route/Abstract_Site_Schema_Aggregator_Xml_Route_Test.php index 8766b576bab..e57ac3f0e3f 100644 --- a/tests/Unit/Schema_Aggregator/User_Interface/Site_Schema_Aggregator_XML_Route/Abstract_Site_Schema_Aggregator_Xml_Route_Test.php +++ b/tests/Unit/Schema_Aggregator/User_Interface/Site_Schema_Aggregator_XML_Route/Abstract_Site_Schema_Aggregator_Xml_Route_Test.php @@ -4,9 +4,7 @@ namespace Yoast\WP\SEO\Tests\Unit\Schema_Aggregator\User_Interface\Site_Schema_Aggregator_XML_Route; use Mockery; -use Yoast\WP\SEO\Schema_Aggregator\Application\Aggregate_Site_Schema_Map_Command_Handler; -use Yoast\WP\SEO\Schema_Aggregator\Application\Cache\Xml_Manager; -use Yoast\WP\SEO\Schema_Aggregator\Infrastructure\Aggregator_Config; +use Yoast\WP\SEO\Schema_Aggregator\Application\Schema_Map\Schema_Map_Xml_Provider; use Yoast\WP\SEO\Schema_Aggregator\User_Interface\Site_Schema_Aggregator_Xml_Route; use Yoast\WP\SEO\Tests\Unit\TestCase; @@ -27,25 +25,11 @@ abstract class Abstract_Site_Schema_Aggregator_Xml_Route_Test extends TestCase { protected $instance; /** - * Holds the command handler mock. + * Holds the schema map XML provider mock. * - * @var Mockery\MockInterface|Aggregate_Site_Schema_Map_Command_Handler + * @var Mockery\MockInterface|Schema_Map_Xml_Provider */ - protected $command_handler; - - /** - * Holds the XML cache manager mock. - * - * @var Mockery\MockInterface|Xml_Manager - */ - protected $xml_cache_manager; - - /** - * Holds the aggregator config mock. - * - * @var Mockery\MockInterface|Aggregator_Config - */ - protected $aggregator_config; + protected $schema_map_xml_provider; /** * Sets up the test fixtures. @@ -55,14 +39,8 @@ abstract class Abstract_Site_Schema_Aggregator_Xml_Route_Test extends TestCase { protected function set_up() { parent::set_up(); - $this->command_handler = Mockery::mock( Aggregate_Site_Schema_Map_Command_Handler::class ); - $this->xml_cache_manager = Mockery::mock( Xml_Manager::class ); - $this->aggregator_config = Mockery::mock( Aggregator_Config::class ); + $this->schema_map_xml_provider = Mockery::mock( Schema_Map_Xml_Provider::class ); - $this->instance = new Site_Schema_Aggregator_Xml_Route( - $this->command_handler, - $this->xml_cache_manager, - $this->aggregator_config, - ); + $this->instance = new Site_Schema_Aggregator_Xml_Route( $this->schema_map_xml_provider ); } } diff --git a/tests/WP/Schema_Aggregator/User_Interface/Site_Schema_Aggregator_Xml_Route_Test.php b/tests/WP/Schema_Aggregator/User_Interface/Site_Schema_Aggregator_Xml_Route_Test.php index ee0209ef479..7748edde9ec 100644 --- a/tests/WP/Schema_Aggregator/User_Interface/Site_Schema_Aggregator_Xml_Route_Test.php +++ b/tests/WP/Schema_Aggregator/User_Interface/Site_Schema_Aggregator_Xml_Route_Test.php @@ -2,14 +2,8 @@ // @phpcs:disable Yoast.NamingConventions.NamespaceName.TooLong -- This namespace should reflect the namespace of the original class. namespace Yoast\WP\SEO\Tests\WP\Schema_Aggregator\User_Interface; -use Mockery; use WP_REST_Request; use WP_REST_Response; -use Yoast\WP\SEO\Schema_Aggregator\Application\Aggregate_Site_Schema_Map_Command_Handler; -use Yoast\WP\SEO\Schema_Aggregator\Application\Cache\Xml_Manager; -use Yoast\WP\SEO\Schema_Aggregator\Infrastructure\Aggregator_Config; -use Yoast\WP\SEO\Schema_Aggregator\Infrastructure\Config; -use Yoast\WP\SEO\Schema_Aggregator\User_Interface\Site_Schema_Aggregator_Xml_Route; use Yoast\WP\SEO\Tests\WP\TestCase; /** @@ -18,69 +12,27 @@ * @group schema-aggregator * * @covers Yoast\WP\SEO\Schema_Aggregator\User_Interface\Site_Schema_Aggregator_Xml_Route::render_schema_xml - * @covers Yoast\WP\SEO\Schema_Aggregator\User_Interface\Site_Schema_Aggregator_Xml_Route::__construct + * @covers Yoast\WP\SEO\Schema_Aggregator\Application\Schema_Map\Schema_Map_Xml_Provider::get_xml */ final class Site_Schema_Aggregator_Xml_Route_Test extends TestCase { - /** - * Holds the instance. - * - * @var Site_Schema_Aggregator_Xml_Route - */ - private $instance; - - /** - * Holds the aggregate site schema map command handler mock. - * - * @var Aggregate_Site_Schema_Map_Command_Handler - */ - private $aggregate_site_schema_map_command_handler; - - /** - * Holds the XML cache manager mock. - * - * @var Xml_Manager - */ - private $xml_cache_manager; - - /** - * Holds the aggregator config mock. - * - * @var Aggregator_Config - */ - private $aggregator_config; - /** * Set up the test. * * @return void */ public function set_up() { - $this->aggregate_site_schema_map_command_handler = Mockery::mock( Aggregate_Site_Schema_Map_Command_Handler::class ); - $this->xml_cache_manager = Mockery::mock( Xml_Manager::class ); - $this->aggregator_config = Mockery::mock( Aggregator_Config::class ); - $this->instance = new Site_Schema_Aggregator_Xml_Route( $this->aggregate_site_schema_map_command_handler, $this->xml_cache_manager, $this->aggregator_config ); \YoastSEO()->helpers->options->set( 'enable_schema_aggregation_endpoint', true ); \do_action( 'rest_api_init' ); } /** - * Tests the xml map without cache. + * Tests that the route renders the schema map as XML. * * @return void */ - public function test_render_schema_xml_no_cache() { - - $this->xml_cache_manager->expects( 'get' ) - ->once() - ->andReturn( null ); - $this->aggregator_config->expects( 'get_allowed_post_types' ) - ->once() - ->andReturn( [ 'page' ] ); - $this->xml_cache_manager->expects( 'set' ) - ->once(); - + public function test_render_schema_xml() { $request = new WP_REST_Request( 'GET', '/yoast/v1/schema-aggregator/get-xml' ); $response = \rest_get_server()->dispatch( $request ); @@ -94,29 +46,14 @@ public function test_render_schema_xml_no_cache() { } /** - * Tests the xml map with cache. + * Tests that a second request is served from the cache and produces the same XML. * * @return void */ - public function test_render_schema_xml_with_cache() { - $this->xml_cache_manager->expects( 'get' ) - ->once() - ->andReturn( '' ); - $this->aggregator_config->expects( 'get_allowed_post_types' ) - ->never(); - $this->xml_cache_manager->expects( 'set' ) - ->never(); - - $request = new WP_REST_Request( 'GET', '/yoast/v1/schema-aggregator/get-xml' ); - $response = \rest_get_server()->dispatch( $request ); - - $this->assertInstanceOf( WP_REST_Response::class, $response ); - - $response_data = $response->get_data(); + public function test_render_schema_xml_is_cached() { + $first = \rest_get_server()->dispatch( new WP_REST_Request( 'GET', '/yoast/v1/schema-aggregator/get-xml' ) ); + $second = \rest_get_server()->dispatch( new WP_REST_Request( 'GET', '/yoast/v1/schema-aggregator/get-xml' ) ); - $this->assertSame( 200, $response->status ); - $this->assertStringContainsString( 'assertSame( 'application/xml; charset=UTF-8', $response->get_headers()['Content-Type'] ); - $this->assertSame( 'public, max-age=300', $response->get_headers()['Cache-Control'] ); + $this->assertSame( $first->get_data(), $second->get_data() ); } } From 4291d0e8a1dd0eba76b02f03a3a734896df086b6 Mon Sep 17 00:00:00 2001 From: Thijs van der heijden Date: Mon, 27 Jul 2026 14:02:12 +0200 Subject: [PATCH 2/5] feat(schema-aggregator): serve the schema map at /schemamap.xml The schema visualiser used to discover a site's schema map through the Schemamap directive in robots.txt. That directive has been removed, and the visualiser now probes the well-known root /schemamap.xml first, falling back to the wp-json endpoint. Only the fallback was served until now. The route mirrors WP_Sitemaps: a rewrite rule pointing at a query var, and a template_redirect handler that echoes the XML and exits. Yoast_Dynamic_Rewrites is used instead of add_rewrite_rule() so the rule follows the feature toggle without needing a rewrite flush. Integrations register on init priority 10, after yoast_add_dynamic_rewrite_rules has fired, so the rule is added directly the way WPSEO_Sitemaps::register_sitemap() does. Under plain permalinks the rewrite does not run, so /schemamap.xml 404s there, the same as /sitemap_index.xml. Discovery falls back to the wp-json endpoint. Co-Authored-By: Claude Opus 5 (1M context) --- .../schemamap-xml-rewrite-integration.php | 162 ++++++++++++++++++ ...Schemamap_Xml_Rewrite_Integration_Test.php | 47 +++++ .../Add_Query_Vars_Test.php | 38 ++++ .../Add_Rewrite_Rules_Test.php | 35 ++++ .../Get_Conditionals_Test.php | 34 ++++ .../Maybe_Render_Schema_Map_Test.php | 67 ++++++++ .../Redirect_Canonical_Test.php | 50 ++++++ .../Register_Hooks_Test.php | 86 ++++++++++ ...Schemamap_Xml_Rewrite_Integration_Test.php | 136 +++++++++++++++ 9 files changed, 655 insertions(+) create mode 100644 src/schema-aggregator/user-interface/schemamap-xml-rewrite-integration.php create mode 100644 tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Abstract_Schemamap_Xml_Rewrite_Integration_Test.php create mode 100644 tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Add_Query_Vars_Test.php create mode 100644 tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Add_Rewrite_Rules_Test.php create mode 100644 tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Get_Conditionals_Test.php create mode 100644 tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Maybe_Render_Schema_Map_Test.php create mode 100644 tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Redirect_Canonical_Test.php create mode 100644 tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Register_Hooks_Test.php create mode 100644 tests/WP/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration_Test.php diff --git a/src/schema-aggregator/user-interface/schemamap-xml-rewrite-integration.php b/src/schema-aggregator/user-interface/schemamap-xml-rewrite-integration.php new file mode 100644 index 00000000000..bfafb7f754e --- /dev/null +++ b/src/schema-aggregator/user-interface/schemamap-xml-rewrite-integration.php @@ -0,0 +1,162 @@ + The conditionals that must be met to load this. + */ + public static function get_conditionals() { + return [ Schema_Aggregator_Conditional::class, Front_End_Conditional::class ]; + } + + /** + * Schemamap_Xml_Rewrite_Integration constructor. + * + * @param Schema_Map_Xml_Provider $schema_map_xml_provider The schema map XML provider. + */ + public function __construct( Schema_Map_Xml_Provider $schema_map_xml_provider ) { + $this->schema_map_xml_provider = $schema_map_xml_provider; + } + + /** + * Registers the hooks for the integration. + * + * @return void + */ + public function register_hooks() { + /* + * Integrations register their hooks on `init` priority 10, which is after + * `yoast_add_dynamic_rewrite_rules` has already fired on `init` priority 1. The rule is + * therefore added directly, the same way `WPSEO_Sitemaps::register_sitemap()` does for + * sitemaps registered during `init`. Rewrite rules are not read until `WP::parse_request()`, + * so this is still in time. + */ + $this->add_rewrite_rules( Yoast_Dynamic_Rewrites::instance() ); + + \add_filter( 'query_vars', [ $this, 'add_query_vars' ] ); + \add_filter( 'redirect_canonical', [ $this, 'redirect_canonical' ] ); + \add_action( 'template_redirect', [ $this, 'maybe_render_schema_map' ], 0 ); + } + + /** + * Adds the rewrite rule for the schema map. + * + * @param Yoast_Dynamic_Rewrites $dynamic_rewrites Dynamic rewrites handler instance. + * + * @return void + */ + public function add_rewrite_rules( Yoast_Dynamic_Rewrites $dynamic_rewrites ) { + $dynamic_rewrites->add_rule( 'schemamap\.xml$', 'index.php?' . self::QUERY_VAR . '=1', 'top' ); + } + + /** + * Adds the query variable for the schema map. + * + * @param array $query_vars List of query variables to filter. + * + * @return array Filtered query variables. + */ + public function add_query_vars( $query_vars ) { + $query_vars[] = self::QUERY_VAR; + + return $query_vars; + } + + /** + * Stops trailing slashes from being added to the schema map URL. + * + * @param string $redirect The redirect URL currently determined. + * + * @return bool|string False when this is a schema map request, the unchanged redirect otherwise. + */ + public function redirect_canonical( $redirect ) { + if ( \get_query_var( self::QUERY_VAR ) ) { + return false; + } + + return $redirect; + } + + /** + * Outputs the schema map when the current request is for it. + * + * @return void + */ + public function maybe_render_schema_map() { + if ( ! \get_query_var( self::QUERY_VAR ) ) { + return; + } + + $this->send_headers(); + + // DOMDocument::createTextNode() already escaped every value; escaping again would corrupt the XML. + echo $this->schema_map_xml_provider->get_xml(); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped -- Raw XML. + + $this->finish_request(); + } + + /** + * Sends the response headers for the schema map. + * + * The headers replace the ones `WP::send_headers()` already sent: the HTML content type, and the + * no-cache headers that are added for logged-in users. + * + * @return void + */ + protected function send_headers() { + if ( \headers_sent() ) { + return; + } + + // Guards against a 200 body being served under the 404 status of a `?yoast_schemamap=1` request. + \status_header( 200 ); + // Prevent the search engines from indexing the schema map. + \header( 'X-Robots-Tag: noindex, follow', true ); + \header( 'Content-Type: application/xml; charset=UTF-8', true ); + \header( 'Cache-Control: public, max-age=300', true ); + } + + /** + * Ends the request after the schema map has been output. + * + * @codeCoverageIgnore Terminates the request, so it cannot be executed under test. + * + * @return void + */ + protected function finish_request() { + exit(); + } +} diff --git a/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Abstract_Schemamap_Xml_Rewrite_Integration_Test.php b/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Abstract_Schemamap_Xml_Rewrite_Integration_Test.php new file mode 100644 index 00000000000..21cc7725e46 --- /dev/null +++ b/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Abstract_Schemamap_Xml_Rewrite_Integration_Test.php @@ -0,0 +1,47 @@ +schema_map_xml_provider = Mockery::mock( Schema_Map_Xml_Provider::class ); + + $this->instance = new Schemamap_Xml_Rewrite_Integration( $this->schema_map_xml_provider ); + } +} diff --git a/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Add_Query_Vars_Test.php b/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Add_Query_Vars_Test.php new file mode 100644 index 00000000000..eadb352cc96 --- /dev/null +++ b/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Add_Query_Vars_Test.php @@ -0,0 +1,38 @@ +assertSame( [ 'yoast_schemamap' ], $this->instance->add_query_vars( [] ) ); + } + + /** + * Tests that the existing query variables are preserved. + * + * @return void + */ + public function test_add_query_vars_preserves_existing_query_vars() { + $this->assertSame( + [ 'sitemap', 'sitemap_n', 'yoast_schemamap' ], + $this->instance->add_query_vars( [ 'sitemap', 'sitemap_n' ] ), + ); + } +} diff --git a/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Add_Rewrite_Rules_Test.php b/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Add_Rewrite_Rules_Test.php new file mode 100644 index 00000000000..1ce1b3661da --- /dev/null +++ b/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Add_Rewrite_Rules_Test.php @@ -0,0 +1,35 @@ +expects( 'add_rule' ) + ->once() + ->with( 'schemamap\.xml$', 'index.php?yoast_schemamap=1', 'top' ); + + $this->instance->add_rewrite_rules( $dynamic_rewrites ); + } +} diff --git a/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Get_Conditionals_Test.php b/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Get_Conditionals_Test.php new file mode 100644 index 00000000000..b9f5e6b54de --- /dev/null +++ b/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Get_Conditionals_Test.php @@ -0,0 +1,34 @@ +assertSame( $expected, $this->instance::get_conditionals() ); + } +} diff --git a/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Maybe_Render_Schema_Map_Test.php b/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Maybe_Render_Schema_Map_Test.php new file mode 100644 index 00000000000..a8e5c437e19 --- /dev/null +++ b/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Maybe_Render_Schema_Map_Test.php @@ -0,0 +1,67 @@ +once() + ->with( 'yoast_schemamap' ) + ->andReturn( '' ); + + $this->schema_map_xml_provider->expects( 'get_xml' )->never(); + + $this->expectOutputString( '' ); + + $this->instance->maybe_render_schema_map(); + } + + /** + * Tests that the schema map XML is echoed for a schema map request. + * + * @return void + */ + public function test_maybe_render_schema_map_outputs_the_xml() { + $xml = ''; + + Monkey\Functions\expect( 'get_query_var' ) + ->once() + ->with( 'yoast_schemamap' ) + ->andReturn( '1' ); + + $this->schema_map_xml_provider->expects( 'get_xml' )->once()->andReturn( $xml ); + + // The headers and the exit cannot run under test, so both are stubbed out. + $instance = Mockery::mock( Schemamap_Xml_Rewrite_Integration::class, [ $this->schema_map_xml_provider ] ) + ->makePartial() + ->shouldAllowMockingProtectedMethods(); + $instance->expects( 'send_headers' )->once(); + $instance->expects( 'finish_request' )->once(); + + $this->expectOutputString( $xml ); + + $instance->maybe_render_schema_map(); + } +} diff --git a/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Redirect_Canonical_Test.php b/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Redirect_Canonical_Test.php new file mode 100644 index 00000000000..54c4d4eef28 --- /dev/null +++ b/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Redirect_Canonical_Test.php @@ -0,0 +1,50 @@ +once() + ->with( 'yoast_schemamap' ) + ->andReturn( '1' ); + + $this->assertFalse( $this->instance->redirect_canonical( 'https://example.com/schemamap.xml/' ) ); + } + + /** + * Tests that the canonical redirect is left alone on any other request. + * + * @return void + */ + public function test_redirect_canonical_is_untouched_for_other_requests() { + Monkey\Functions\expect( 'get_query_var' ) + ->once() + ->with( 'yoast_schemamap' ) + ->andReturn( '' ); + + $this->assertSame( + 'https://example.com/some-post/', + $this->instance->redirect_canonical( 'https://example.com/some-post/' ), + ); + } +} diff --git a/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Register_Hooks_Test.php b/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Register_Hooks_Test.php new file mode 100644 index 00000000000..fb252ba57a5 --- /dev/null +++ b/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Register_Hooks_Test.php @@ -0,0 +1,86 @@ +reset_dynamic_rewrites(); + + $wp_rewrite = Mockery::mock( 'WP_Rewrite' ); + $wp_rewrite->index = 'index.php'; + $wp_rewrite->expects( 'add_rule' )->once(); + + $GLOBALS['wp_rewrite'] = $wp_rewrite; + } + + /** + * Resets the Yoast_Dynamic_Rewrites singleton so it does not leak its WP_Rewrite mock into other tests. + * + * @return void + */ + protected function tear_down() { + $this->reset_dynamic_rewrites(); + + unset( $GLOBALS['wp_rewrite'] ); + + parent::tear_down(); + } + + /** + * Clears the Yoast_Dynamic_Rewrites static instance. + * + * @return void + */ + private function reset_dynamic_rewrites() { + $instance = new ReflectionProperty( Yoast_Dynamic_Rewrites::class, 'instance' ); + $instance->setAccessible( true ); + $instance->setValue( null, null ); + } + + /** + * Tests the registration of the hooks. + * + * @return void + */ + public function test_register_hooks() { + $this->instance->register_hooks(); + + $this->assertNotFalse( + Monkey\Filters\has( 'query_vars', [ $this->instance, 'add_query_vars' ] ), + ); + $this->assertNotFalse( + Monkey\Filters\has( 'redirect_canonical', [ $this->instance, 'redirect_canonical' ] ), + ); + $this->assertNotFalse( + Monkey\Actions\has( 'template_redirect', [ $this->instance, 'maybe_render_schema_map' ] ), + ); + } +} diff --git a/tests/WP/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration_Test.php b/tests/WP/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration_Test.php new file mode 100644 index 00000000000..039a1fa20d4 --- /dev/null +++ b/tests/WP/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration_Test.php @@ -0,0 +1,136 @@ +helpers->options->set( 'enable_schema_aggregation_endpoint', true ); + + $this->instance = new Schemamap_Xml_Rewrite_Integration( + \YoastSEO()->classes->get( Schema_Map_Xml_Provider::class ), + ); + + $this->set_permalink_structure( '/%postname%/' ); + $this->instance->register_hooks(); + } + + /** + * Restores the default permalink structure. + * + * @return void + */ + public function tear_down() { + $this->set_permalink_structure( '' ); + + parent::tear_down(); + } + + /** + * Tests that the rewrite rule is applied to the rewrite rules option. + * + * @return void + */ + public function test_rewrite_rule_is_registered() { + global $wp_rewrite; + + $this->assertArrayHasKey( 'schemamap\.xml$', $wp_rewrite->wp_rewrite_rules() ); + $this->assertSame( + 'index.php?yoast_schemamap=1', + $wp_rewrite->wp_rewrite_rules()['schemamap\.xml$'], + ); + } + + /** + * Tests that a request for the schema map resolves to the query variable. + * + * @return void + */ + public function test_request_sets_the_query_var() { + $this->go_to( \home_url( '/schemamap.xml' ) ); + + $this->assertFalse( \is_404() ); + $this->assertSame( '1', \get_query_var( 'yoast_schemamap' ) ); + } + + /** + * Tests that a trailing slash still resolves to the query variable. + * + * @return void + */ + public function test_trailing_slash_request_sets_the_query_var() { + $this->go_to( \home_url( '/schemamap.xml/' ) ); + + $this->assertFalse( \is_404() ); + $this->assertSame( '1', \get_query_var( 'yoast_schemamap' ) ); + } + + /** + * Tests that the canonical redirect is cancelled for a schema map request. + * + * @return void + */ + public function test_redirect_canonical_is_suppressed() { + $this->go_to( \home_url( '/schemamap.xml' ) ); + + $this->assertFalse( $this->instance->redirect_canonical( \home_url( '/schemamap.xml/' ) ) ); + } + + /** + * Tests that the schema map is not routed when pretty permalinks are off. + * + * This documents the known limitation: rewrite rules do not run under plain permalinks, so + * discovery falls back to the REST endpoint there. + * + * @return void + */ + public function test_plain_permalinks_do_not_route() { + $this->set_permalink_structure( '' ); + + $this->go_to( \home_url( '/schemamap.xml' ) ); + + $this->assertSame( '', \get_query_var( 'yoast_schemamap' ) ); + } + + /** + * Tests that the rendered XML is what the schema visualizer expects to find. + * + * @return void + */ + public function test_rendered_xml_is_a_sitemap_document() { + $xml = \YoastSEO()->classes->get( Schema_Map_Xml_Provider::class )->get_xml(); + + $this->assertStringStartsWith( 'assertStringContainsString( ' Date: Mon, 27 Jul 2026 15:44:56 +0200 Subject: [PATCH 3/5] fix(schema-aggregator): harden the schemamap.xml response Addresses code review on the /schemamap.xml route. Build the XML before committing any header. get_xml() queries the database and Schema_Map_Xml_Renderer can throw, so a failure after the 200 and the cache headers went out would let a proxy store an HTML error page at the well-known path for the full max-age. The visualiser rejects anything that is not a sitemap document, so that window mattered. Render on pre_get_posts priority 1 with an is_main_query() guard instead of template_redirect priority 0, matching WPSEO_Sitemaps::redirect(). This skips the main posts query, which the response never uses, and leaves fewer third-party callbacks able to echo ahead of the document. Clear Expires alongside Cache-Control. wp_get_nocache_headers() sets both for logged-in users, so overriding only Cache-Control left the two contradicting. Headers now go through Redirect_Helper, which exists to make them testable; header() and headers_sent() are PHP internals that Patchwork cannot stub. Drop the redirect_canonical filter. Core registers redirect_canonical() on template_redirect priority 10 and applies the filter nowhere else, so exiting during pre_get_posts makes the callback unreachable. Its two tests asserted a path no request can take. Also adds real coverage for send_headers() and the build-before-headers ordering, asserts the hook priority rather than just its registration, and replaces a WP cache test that passed whether or not caching existed. Co-Authored-By: Claude Opus 5 (1M context) --- .../schemamap-xml-rewrite-integration.php | 89 ++++++++----- ...Schemamap_Xml_Rewrite_Integration_Test.php | 14 +- .../Maybe_Render_Schema_Map_Test.php | 122 +++++++++++++++--- .../Redirect_Canonical_Test.php | 50 ------- .../Register_Hooks_Test.php | 13 +- .../Send_Headers_Test.php | 43 ++++++ ...Schemamap_Xml_Rewrite_Integration_Test.php | 40 +++--- .../Site_Schema_Aggregator_Xml_Route_Test.php | 11 +- 8 files changed, 247 insertions(+), 135 deletions(-) delete mode 100644 tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Redirect_Canonical_Test.php create mode 100644 tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Send_Headers_Test.php diff --git a/src/schema-aggregator/user-interface/schemamap-xml-rewrite-integration.php b/src/schema-aggregator/user-interface/schemamap-xml-rewrite-integration.php index bfafb7f754e..53ff0194b3c 100644 --- a/src/schema-aggregator/user-interface/schemamap-xml-rewrite-integration.php +++ b/src/schema-aggregator/user-interface/schemamap-xml-rewrite-integration.php @@ -2,7 +2,9 @@ // phpcs:disable Yoast.NamingConventions.NamespaceName.TooLong -- Needed in the folder structure. namespace Yoast\WP\SEO\Schema_Aggregator\User_Interface; +use WP_Query; use Yoast\WP\SEO\Conditionals\Front_End_Conditional; +use Yoast\WP\SEO\Helpers\Redirect_Helper; use Yoast\WP\SEO\Integrations\Integration_Interface; use Yoast\WP\SEO\Schema_Aggregator\Application\Schema_Map\Schema_Map_Xml_Provider; use Yoast\WP\SEO\Schema_Aggregator\Infrastructure\Schema_Aggregator_Conditional; @@ -29,11 +31,18 @@ class Schemamap_Xml_Rewrite_Integration implements Integration_Interface { */ private $schema_map_xml_provider; + /** + * The redirect helper. + * + * @var Redirect_Helper + */ + private $redirect_helper; + /** * Returns the conditionals for this integration. * - * The front end conditional keeps the rewrite rule out of admin requests, where `template_redirect` - * never fires anyway. It also means the rule is absent while Yoast SEO is being deactivated, so no + * The front end conditional keeps the rewrite rule out of admin requests, where the schema map is + * never served. It also means the rule is absent while Yoast SEO is being deactivated, so no * equivalent of the `Deactivating_Yoast_Seo_Conditional` guard in `WPSEO_Sitemaps_Router` is needed. * * @return array The conditionals that must be met to load this. @@ -46,14 +55,21 @@ public static function get_conditionals() { * Schemamap_Xml_Rewrite_Integration constructor. * * @param Schema_Map_Xml_Provider $schema_map_xml_provider The schema map XML provider. + * @param Redirect_Helper $redirect_helper The redirect helper. */ - public function __construct( Schema_Map_Xml_Provider $schema_map_xml_provider ) { + public function __construct( Schema_Map_Xml_Provider $schema_map_xml_provider, Redirect_Helper $redirect_helper ) { $this->schema_map_xml_provider = $schema_map_xml_provider; + $this->redirect_helper = $redirect_helper; } /** * Registers the hooks for the integration. * + * The schema map is rendered on `pre_get_posts` rather than `template_redirect`, the same way + * `WPSEO_Sitemaps::redirect()` renders sitemaps. That skips the main posts query, which this + * response never uses, and leaves fewer third-party callbacks able to echo ahead of the XML — + * anything printed before the document would stop it parsing as a sitemap. + * * @return void */ public function register_hooks() { @@ -67,8 +83,7 @@ public function register_hooks() { $this->add_rewrite_rules( Yoast_Dynamic_Rewrites::instance() ); \add_filter( 'query_vars', [ $this, 'add_query_vars' ] ); - \add_filter( 'redirect_canonical', [ $this, 'redirect_canonical' ] ); - \add_action( 'template_redirect', [ $this, 'maybe_render_schema_map' ], 0 ); + \add_action( 'pre_get_posts', [ $this, 'maybe_render_schema_map' ], 1 ); } /** @@ -96,34 +111,36 @@ public function add_query_vars( $query_vars ) { } /** - * Stops trailing slashes from being added to the schema map URL. + * Outputs the schema map when the main query is a request for it. * - * @param string $redirect The redirect URL currently determined. + * @param WP_Query $query The query that is about to run. * - * @return bool|string False when this is a schema map request, the unchanged redirect otherwise. + * @return void */ - public function redirect_canonical( $redirect ) { - if ( \get_query_var( self::QUERY_VAR ) ) { - return false; + public function maybe_render_schema_map( WP_Query $query ) { + if ( ! $query->is_main_query() || ! $query->get( self::QUERY_VAR ) ) { + return; } - return $redirect; - } - - /** - * Outputs the schema map when the current request is for it. - * - * @return void - */ - public function maybe_render_schema_map() { - if ( ! \get_query_var( self::QUERY_VAR ) ) { + /* + * Without control over the headers the response cannot be typed as XML, and echoing the + * document into a half-written HTML body would produce neither a page nor a schema map. + */ + if ( $this->headers_already_sent() ) { return; } + /* + * Build the document before committing any headers. Generating it hits the database and can + * throw, and a failure after the 200 and the cache headers were sent would let a proxy store + * an HTML error page at this path for the full max-age. + */ + $xml = $this->schema_map_xml_provider->get_xml(); + $this->send_headers(); // DOMDocument::createTextNode() already escaped every value; escaping again would corrupt the XML. - echo $this->schema_map_xml_provider->get_xml(); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped -- Raw XML. + echo $xml; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped -- Raw XML. $this->finish_request(); } @@ -131,22 +148,30 @@ public function maybe_render_schema_map() { /** * Sends the response headers for the schema map. * - * The headers replace the ones `WP::send_headers()` already sent: the HTML content type, and the - * no-cache headers that are added for logged-in users. + * These replace what `WP::send_headers()` already sent: the HTML content type, and the `Expires` + * and `Cache-Control` pair that `wp_get_nocache_headers()` adds for logged-in users. * * @return void */ protected function send_headers() { - if ( \headers_sent() ) { - return; - } - - // Guards against a 200 body being served under the 404 status of a `?yoast_schemamap=1` request. \status_header( 200 ); // Prevent the search engines from indexing the schema map. - \header( 'X-Robots-Tag: noindex, follow', true ); - \header( 'Content-Type: application/xml; charset=UTF-8', true ); - \header( 'Cache-Control: public, max-age=300', true ); + $this->redirect_helper->set_header( 'X-Robots-Tag: noindex, follow' ); + $this->redirect_helper->set_header( 'Content-Type: application/xml; charset=UTF-8' ); + $this->redirect_helper->set_header( 'Cache-Control: public, max-age=300' ); + // The response is public, so the no-cache Expires date from wp_get_nocache_headers() must go. + $this->redirect_helper->remove_header( 'Expires' ); + } + + /** + * Tells whether the response headers have already been sent. + * + * @codeCoverageIgnore Wraps a PHP function that always reports true under the CLI test runner. + * + * @return bool Whether the headers have already been sent. + */ + protected function headers_already_sent(): bool { + return \headers_sent(); } /** diff --git a/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Abstract_Schemamap_Xml_Rewrite_Integration_Test.php b/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Abstract_Schemamap_Xml_Rewrite_Integration_Test.php index 21cc7725e46..329616231d9 100644 --- a/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Abstract_Schemamap_Xml_Rewrite_Integration_Test.php +++ b/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Abstract_Schemamap_Xml_Rewrite_Integration_Test.php @@ -4,6 +4,7 @@ namespace Yoast\WP\SEO\Tests\Unit\Schema_Aggregator\User_Interface\Schemamap_Xml_Rewrite_Integration; use Mockery; +use Yoast\WP\SEO\Helpers\Redirect_Helper; use Yoast\WP\SEO\Schema_Aggregator\Application\Schema_Map\Schema_Map_Xml_Provider; use Yoast\WP\SEO\Schema_Aggregator\User_Interface\Schemamap_Xml_Rewrite_Integration; use Yoast\WP\SEO\Tests\Unit\TestCase; @@ -32,6 +33,13 @@ abstract class Abstract_Schemamap_Xml_Rewrite_Integration_Test extends TestCase */ protected $schema_map_xml_provider; + /** + * Holds the redirect helper mock. + * + * @var Mockery\MockInterface|Redirect_Helper + */ + protected $redirect_helper; + /** * Sets up the test fixtures. * @@ -41,7 +49,11 @@ protected function set_up() { parent::set_up(); $this->schema_map_xml_provider = Mockery::mock( Schema_Map_Xml_Provider::class ); + $this->redirect_helper = Mockery::mock( Redirect_Helper::class ); - $this->instance = new Schemamap_Xml_Rewrite_Integration( $this->schema_map_xml_provider ); + $this->instance = new Schemamap_Xml_Rewrite_Integration( + $this->schema_map_xml_provider, + $this->redirect_helper, + ); } } diff --git a/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Maybe_Render_Schema_Map_Test.php b/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Maybe_Render_Schema_Map_Test.php index a8e5c437e19..cfa3cbdbeed 100644 --- a/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Maybe_Render_Schema_Map_Test.php +++ b/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Maybe_Render_Schema_Map_Test.php @@ -3,8 +3,8 @@ // phpcs:disable Yoast.NamingConventions.NamespaceName.MaxExceeded namespace Yoast\WP\SEO\Tests\Unit\Schema_Aggregator\User_Interface\Schemamap_Xml_Rewrite_Integration; -use Brain\Monkey; use Mockery; +use WP_Query; use Yoast\WP\SEO\Schema_Aggregator\User_Interface\Schemamap_Xml_Rewrite_Integration; /** @@ -14,28 +14,86 @@ * @group Schemamap_Xml_Rewrite_Integration * * @covers Yoast\WP\SEO\Schema_Aggregator\User_Interface\Schemamap_Xml_Rewrite_Integration::maybe_render_schema_map - * @covers Yoast\WP\SEO\Schema_Aggregator\User_Interface\Schemamap_Xml_Rewrite_Integration::send_headers * * @phpcs:disable Yoast.NamingConventions.ObjectNameDepth.MaxExceeded */ final class Maybe_Render_Schema_Map_Test extends Abstract_Schemamap_Xml_Rewrite_Integration_Test { + /** + * Builds a query mock. + * + * @param bool $is_main_query Whether the query is the main query. + * @param string $query_var The value of the schema map query variable. + * + * @return Mockery\MockInterface|WP_Query The query mock. + */ + private function mock_query( $is_main_query, $query_var ) { + $query = Mockery::mock( WP_Query::class ); + $query->allows( 'is_main_query' )->andReturn( $is_main_query ); + $query->allows( 'get' )->with( 'yoast_schemamap' )->andReturn( $query_var ); + + return $query; + } + + /** + * Builds a partial mock with the methods that cannot run under test stubbed out. + * + * @param bool $headers_sent What the headers_already_sent check should report. + * + * @return Mockery\MockInterface|Schemamap_Xml_Rewrite_Integration The partial mock. + */ + private function mock_instance( $headers_sent ) { + $instance = Mockery::mock( + Schemamap_Xml_Rewrite_Integration::class, + [ $this->schema_map_xml_provider, $this->redirect_helper ], + )->makePartial()->shouldAllowMockingProtectedMethods(); + + $instance->allows( 'headers_already_sent' )->andReturn( $headers_sent ); + + return $instance; + } + + /** + * Tests that nothing is rendered for a secondary query. + * + * @return void + */ + public function test_maybe_render_schema_map_ignores_secondary_queries() { + $this->schema_map_xml_provider->expects( 'get_xml' )->never(); + + $this->expectOutputString( '' ); + + $this->instance->maybe_render_schema_map( $this->mock_query( false, '1' ) ); + } + /** * Tests that nothing is rendered when the request is not for the schema map. * * @return void */ public function test_maybe_render_schema_map_does_nothing_for_other_requests() { - Monkey\Functions\expect( 'get_query_var' ) - ->once() - ->with( 'yoast_schemamap' ) - ->andReturn( '' ); + $this->schema_map_xml_provider->expects( 'get_xml' )->never(); + + $this->expectOutputString( '' ); + + $this->instance->maybe_render_schema_map( $this->mock_query( true, '' ) ); + } + + /** + * Tests that nothing is rendered when the response can no longer be typed as XML. + * + * @return void + */ + public function test_maybe_render_schema_map_bails_when_headers_are_sent() { + $instance = $this->mock_instance( true ); + $instance->expects( 'send_headers' )->never(); + $instance->expects( 'finish_request' )->never(); $this->schema_map_xml_provider->expects( 'get_xml' )->never(); $this->expectOutputString( '' ); - $this->instance->maybe_render_schema_map(); + $instance->maybe_render_schema_map( $this->mock_query( true, '1' ) ); } /** @@ -46,22 +104,52 @@ public function test_maybe_render_schema_map_does_nothing_for_other_requests() { public function test_maybe_render_schema_map_outputs_the_xml() { $xml = ''; - Monkey\Functions\expect( 'get_query_var' ) - ->once() - ->with( 'yoast_schemamap' ) - ->andReturn( '1' ); - $this->schema_map_xml_provider->expects( 'get_xml' )->once()->andReturn( $xml ); - // The headers and the exit cannot run under test, so both are stubbed out. - $instance = Mockery::mock( Schemamap_Xml_Rewrite_Integration::class, [ $this->schema_map_xml_provider ] ) - ->makePartial() - ->shouldAllowMockingProtectedMethods(); + $instance = $this->mock_instance( false ); $instance->expects( 'send_headers' )->once(); $instance->expects( 'finish_request' )->once(); $this->expectOutputString( $xml ); - $instance->maybe_render_schema_map(); + $instance->maybe_render_schema_map( $this->mock_query( true, '1' ) ); + } + + /** + * Tests that the XML is built before any header is committed. + * + * A build failure after the 200 and the cache headers went out would let a proxy store an error + * page at this path for the full max-age, so the ordering is part of the contract. + * + * @return void + */ + public function test_maybe_render_schema_map_builds_the_xml_before_sending_headers() { + $calls = []; + + $this->schema_map_xml_provider->expects( 'get_xml' ) + ->once() + ->andReturnUsing( + static function () use ( &$calls ) { + $calls[] = 'get_xml'; + + return ''; + }, + ); + + $instance = $this->mock_instance( false ); + $instance->expects( 'send_headers' ) + ->once() + ->andReturnUsing( + static function () use ( &$calls ) { + $calls[] = 'send_headers'; + }, + ); + $instance->expects( 'finish_request' )->once(); + + $this->expectOutputString( '' ); + + $instance->maybe_render_schema_map( $this->mock_query( true, '1' ) ); + + $this->assertSame( [ 'get_xml', 'send_headers' ], $calls ); } } diff --git a/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Redirect_Canonical_Test.php b/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Redirect_Canonical_Test.php deleted file mode 100644 index 54c4d4eef28..00000000000 --- a/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Redirect_Canonical_Test.php +++ /dev/null @@ -1,50 +0,0 @@ -once() - ->with( 'yoast_schemamap' ) - ->andReturn( '1' ); - - $this->assertFalse( $this->instance->redirect_canonical( 'https://example.com/schemamap.xml/' ) ); - } - - /** - * Tests that the canonical redirect is left alone on any other request. - * - * @return void - */ - public function test_redirect_canonical_is_untouched_for_other_requests() { - Monkey\Functions\expect( 'get_query_var' ) - ->once() - ->with( 'yoast_schemamap' ) - ->andReturn( '' ); - - $this->assertSame( - 'https://example.com/some-post/', - $this->instance->redirect_canonical( 'https://example.com/some-post/' ), - ); - } -} diff --git a/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Register_Hooks_Test.php b/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Register_Hooks_Test.php index fb252ba57a5..d6314d64dbe 100644 --- a/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Register_Hooks_Test.php +++ b/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Register_Hooks_Test.php @@ -76,11 +76,14 @@ public function test_register_hooks() { $this->assertNotFalse( Monkey\Filters\has( 'query_vars', [ $this->instance, 'add_query_vars' ] ), ); - $this->assertNotFalse( - Monkey\Filters\has( 'redirect_canonical', [ $this->instance, 'redirect_canonical' ] ), - ); - $this->assertNotFalse( - Monkey\Actions\has( 'template_redirect', [ $this->instance, 'maybe_render_schema_map' ] ), + + /* + * The priority is asserted, not just the registration: rendering has to happen before the + * main query runs and before other callbacks get a chance to echo ahead of the XML. + */ + $this->assertSame( + 1, + Monkey\Actions\has( 'pre_get_posts', [ $this->instance, 'maybe_render_schema_map' ] ), ); } } diff --git a/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Send_Headers_Test.php b/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Send_Headers_Test.php new file mode 100644 index 00000000000..0bb743b4e92 --- /dev/null +++ b/tests/Unit/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration/Send_Headers_Test.php @@ -0,0 +1,43 @@ +once()->with( 200 ); + + $this->redirect_helper->expects( 'set_header' )->once()->with( 'X-Robots-Tag: noindex, follow' ); + $this->redirect_helper->expects( 'set_header' )->once()->with( 'Content-Type: application/xml; charset=UTF-8' ); + $this->redirect_helper->expects( 'set_header' )->once()->with( 'Cache-Control: public, max-age=300' ); + + /* + * WP::send_headers() pairs its no-cache Cache-Control with an Expires date in the past for + * logged-in users. Replacing Cache-Control alone would leave the two contradicting. + */ + $this->redirect_helper->expects( 'remove_header' )->once()->with( 'Expires' ); + + $send_headers = new ReflectionMethod( $this->instance, 'send_headers' ); + $send_headers->setAccessible( true ); + $send_headers->invoke( $this->instance ); + } +} diff --git a/tests/WP/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration_Test.php b/tests/WP/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration_Test.php index 039a1fa20d4..b58f802c6b0 100644 --- a/tests/WP/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration_Test.php +++ b/tests/WP/Schema_Aggregator/User_Interface/Schemamap_Xml_Rewrite_Integration_Test.php @@ -2,6 +2,7 @@ // @phpcs:disable Yoast.NamingConventions.NamespaceName.TooLong -- This namespace should reflect the namespace of the original class. namespace Yoast\WP\SEO\Tests\WP\Schema_Aggregator\User_Interface; +use Mockery; use Yoast\WP\SEO\Schema_Aggregator\Application\Schema_Map\Schema_Map_Xml_Provider; use Yoast\WP\SEO\Schema_Aggregator\User_Interface\Schemamap_Xml_Rewrite_Integration; use Yoast\WP\SEO\Tests\WP\TestCase; @@ -14,7 +15,6 @@ * @covers Yoast\WP\SEO\Schema_Aggregator\User_Interface\Schemamap_Xml_Rewrite_Integration::register_hooks * @covers Yoast\WP\SEO\Schema_Aggregator\User_Interface\Schemamap_Xml_Rewrite_Integration::add_rewrite_rules * @covers Yoast\WP\SEO\Schema_Aggregator\User_Interface\Schemamap_Xml_Rewrite_Integration::add_query_vars - * @covers Yoast\WP\SEO\Schema_Aggregator\User_Interface\Schemamap_Xml_Rewrite_Integration::redirect_canonical * * @phpcs:disable Yoast.NamingConventions.ObjectNameDepth.MaxExceeded */ @@ -37,12 +37,25 @@ public function set_up() { \YoastSEO()->helpers->options->set( 'enable_schema_aggregation_endpoint', true ); + /* + * The provider is only ever a constructor dependency, so the DI compiler inlines it and it + * cannot be fetched from the container. These tests cover routing rather than the document, + * which Site_Schema_Aggregator_Xml_Route_Test already exercises against the real provider. + */ $this->instance = new Schemamap_Xml_Rewrite_Integration( - \YoastSEO()->classes->get( Schema_Map_Xml_Provider::class ), + Mockery::mock( Schema_Map_Xml_Provider::class ), + \YoastSEO()->helpers->redirect, ); $this->set_permalink_structure( '/%postname%/' ); $this->instance->register_hooks(); + + /* + * go_to() runs the main query, which would reach the render callback and exit the PHPUnit + * process. The rewrite rule and the query var are what these tests assert, so the callback + * is unhooked; rendering is covered by the unit tests. + */ + \remove_action( 'pre_get_posts', [ $this->instance, 'maybe_render_schema_map' ], 1 ); } /** @@ -95,17 +108,6 @@ public function test_trailing_slash_request_sets_the_query_var() { $this->assertSame( '1', \get_query_var( 'yoast_schemamap' ) ); } - /** - * Tests that the canonical redirect is cancelled for a schema map request. - * - * @return void - */ - public function test_redirect_canonical_is_suppressed() { - $this->go_to( \home_url( '/schemamap.xml' ) ); - - $this->assertFalse( $this->instance->redirect_canonical( \home_url( '/schemamap.xml/' ) ) ); - } - /** * Tests that the schema map is not routed when pretty permalinks are off. * @@ -121,16 +123,4 @@ public function test_plain_permalinks_do_not_route() { $this->assertSame( '', \get_query_var( 'yoast_schemamap' ) ); } - - /** - * Tests that the rendered XML is what the schema visualizer expects to find. - * - * @return void - */ - public function test_rendered_xml_is_a_sitemap_document() { - $xml = \YoastSEO()->classes->get( Schema_Map_Xml_Provider::class )->get_xml(); - - $this->assertStringStartsWith( 'assertStringContainsString( 'dispatch( new WP_REST_Request( 'GET', '/yoast/v1/schema-aggregator/get-xml' ) ); - $second = \rest_get_server()->dispatch( new WP_REST_Request( 'GET', '/yoast/v1/schema-aggregator/get-xml' ) ); + public function test_render_schema_xml_populates_the_cache() { + \delete_transient( 'yoast_schema_aggregator_xml_sitemap_v1' ); - $this->assertSame( $first->get_data(), $second->get_data() ); + $response = \rest_get_server()->dispatch( new WP_REST_Request( 'GET', '/yoast/v1/schema-aggregator/get-xml' ) ); + + $this->assertSame( $response->get_data(), \get_transient( 'yoast_schema_aggregator_xml_sitemap_v1' ) ); } } From 655ecb714af1e7749be74ac92df997009fa40e6e Mon Sep 17 00:00:00 2001 From: Thijs van der heijden Date: Tue, 28 Jul 2026 13:50:40 +0200 Subject: [PATCH 4/5] docs(schema-aggregator): explain the schema map render test seams headers_already_sent() and finish_request() are protected one-line wrappers, which read as unexplained indirection. Both exist so Maybe_Render_Schema_Map_Test can stub them on a Mockery partial mock: exit() would terminate the test runner, and the test process cannot control what headers_sent() reports. Record that on the methods, along with why they are protected rather than private. The @codeCoverageIgnore reason on headers_already_sent() was carrying the whole explanation, so it is trimmed back to just the coverage justification. Co-Authored-By: Claude Opus 5 (1M context) --- .../schemamap-xml-rewrite-integration.php | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/schema-aggregator/user-interface/schemamap-xml-rewrite-integration.php b/src/schema-aggregator/user-interface/schemamap-xml-rewrite-integration.php index 53ff0194b3c..c0a5974d2e7 100644 --- a/src/schema-aggregator/user-interface/schemamap-xml-rewrite-integration.php +++ b/src/schema-aggregator/user-interface/schemamap-xml-rewrite-integration.php @@ -166,7 +166,11 @@ protected function send_headers() { /** * Tells whether the response headers have already been sent. * - * @codeCoverageIgnore Wraps a PHP function that always reports true under the CLI test runner. + * Wrapped in an overridable method so the unit tests can drive both branches of + * maybe_render_schema_map(); the test process cannot control what headers_sent() reports. + * Protected rather than private because Mockery cannot stub private methods. + * + * @codeCoverageIgnore It only wraps a PHP function. * * @return bool Whether the headers have already been sent. */ @@ -177,7 +181,11 @@ protected function headers_already_sent(): bool { /** * Ends the request after the schema map has been output. * - * @codeCoverageIgnore Terminates the request, so it cannot be executed under test. + * The exit() is wrapped in an overridable method so the unit tests can stub it out; executing it + * would terminate the test runner. Protected rather than private because Mockery cannot stub + * private methods. + * + * @codeCoverageIgnore It only wraps a language construct that terminates the request. * * @return void */ From ffee2ccb5bd36bb611f0e9e4550bf15467cc664b Mon Sep 17 00:00:00 2001 From: Thijs van der heijden Date: Fri, 31 Jul 2026 14:56:52 +0200 Subject: [PATCH 5/5] tiny doc changes --- .../user-interface/site-schema-aggregator-xml-route.php | 2 +- .../User_Interface/Site_Schema_Aggregator_Xml_Route_Test.php | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/schema-aggregator/user-interface/site-schema-aggregator-xml-route.php b/src/schema-aggregator/user-interface/site-schema-aggregator-xml-route.php index bdbfcf0f548..92e89ef103b 100644 --- a/src/schema-aggregator/user-interface/site-schema-aggregator-xml-route.php +++ b/src/schema-aggregator/user-interface/site-schema-aggregator-xml-route.php @@ -80,7 +80,7 @@ public function get_permission_callback(): bool { } /** - * Returns a XML representation of the possible post types that can be used for schema. + * Returns a XML representation of the post types that are used for schema. * * @return WP_REST_Response The response. */ diff --git a/tests/WP/Schema_Aggregator/User_Interface/Site_Schema_Aggregator_Xml_Route_Test.php b/tests/WP/Schema_Aggregator/User_Interface/Site_Schema_Aggregator_Xml_Route_Test.php index b7d9f9c40f1..4c2d423bd78 100644 --- a/tests/WP/Schema_Aggregator/User_Interface/Site_Schema_Aggregator_Xml_Route_Test.php +++ b/tests/WP/Schema_Aggregator/User_Interface/Site_Schema_Aggregator_Xml_Route_Test.php @@ -22,6 +22,8 @@ final class Site_Schema_Aggregator_Xml_Route_Test extends TestCase { * @return void */ public function set_up() { + parent::set_up(); + \YoastSEO()->helpers->options->set( 'enable_schema_aggregation_endpoint', true ); \do_action( 'rest_api_init' );