From 591d1f1d2de4a129784943fa079380e0c63150c1 Mon Sep 17 00:00:00 2001 From: Leonidas Milosis Date: Wed, 24 Apr 2024 13:45:30 +0300 Subject: [PATCH 01/35] Move checking if indexable should be saved out of the builder and into a wrapper --- src/builders/indexable-builder.php | 51 +++--------------------------- src/helpers/indexable-helper.php | 45 ++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 47 deletions(-) diff --git a/src/builders/indexable-builder.php b/src/builders/indexable-builder.php index b882c1f0dd2..f0c4b727004 100644 --- a/src/builders/indexable-builder.php +++ b/src/builders/indexable-builder.php @@ -267,49 +267,6 @@ protected function ensure_indexable( $indexable, $defaults = [] ) { return $indexable; } - /** - * Saves and returns an indexable (on production environments only). - * - * @param Indexable $indexable The indexable. - * @param Indexable|null $indexable_before The indexable before possible changes. - * - * @return Indexable The indexable. - */ - protected function save_indexable( $indexable, $indexable_before = null ) { - $intend_to_save = $this->indexable_helper->should_index_indexables(); - - /** - * Filter: 'wpseo_should_save_indexable' - Allow developers to enable / disable - * saving the indexable when the indexable is updated. Warning: overriding - * the intended action may cause problems when moving from a staging to a - * production environment because indexable permalinks may get set incorrectly. - * - * @param bool $intend_to_save True if YoastSEO intends to save the indexable. - * @param Indexable $indexable The indexable to be saved. - */ - $intend_to_save = \apply_filters( 'wpseo_should_save_indexable', $intend_to_save, $indexable ); - - if ( ! $intend_to_save ) { - return $indexable; - } - - // Save the indexable before running the WordPress hook. - $indexable->save(); - - if ( $indexable_before ) { - /** - * Action: 'wpseo_save_indexable' - Allow developers to perform an action - * when the indexable is updated. - * - * @param Indexable $indexable The saved indexable. - * @param Indexable $indexable_before The indexable before saving. - */ - \do_action( 'wpseo_save_indexable', $indexable, $indexable_before ); - } - - return $indexable; - } - /** * Build and author indexable from an author id if it does not exist yet, or if the author indexable needs to be upgraded. * @@ -372,7 +329,7 @@ public function build( $indexable, $defaults = null ) { $indexable = $this->post_builder->build( $indexable->object_id, $indexable ); // Save indexable, to make sure it can be queried when building related objects like the author indexable and hierarchy. - $indexable = $this->save_indexable( $indexable, $indexable_before ); + $indexable = $this->indexable_helper->save_indexable( $indexable, $indexable_before ); // For attachments, we have to make sure to patch any potentially previously cleaned up SEO links. if ( \is_a( $indexable, Indexable::class ) && $indexable->object_sub_type === 'attachment' ) { @@ -398,7 +355,7 @@ public function build( $indexable, $defaults = null ) { $indexable = $this->term_builder->build( $indexable->object_id, $indexable ); // Save indexable, to make sure it can be queried when building hierarchy. - $indexable = $this->save_indexable( $indexable, $indexable_before ); + $indexable = $this->indexable_helper->save_indexable( $indexable, $indexable_before ); $this->hierarchy_builder->build( $indexable ); @@ -422,7 +379,7 @@ public function build( $indexable, $defaults = null ) { break; } - return $this->save_indexable( $indexable, $indexable_before ); + return $this->indexable_helper->save_indexable( $indexable, $indexable_before ); } catch ( Source_Exception $exception ) { if ( ! $this->is_type_with_no_id( $indexable->object_type ) && ( ! isset( $indexable->object_id ) || \is_null( $indexable->object_id ) ) ) { @@ -449,7 +406,7 @@ public function build( $indexable, $defaults = null ) { // Make sure that the indexing process doesn't get stuck in a loop on this broken indexable. $indexable = $this->version_manager->set_latest( $indexable ); - return $this->save_indexable( $indexable, $indexable_before ); + return $this->indexable_helper->save_indexable( $indexable, $indexable_before ); } catch ( Not_Built_Exception $exception ) { return false; diff --git a/src/helpers/indexable-helper.php b/src/helpers/indexable-helper.php index 81317e4826e..ea98ee6b4fd 100644 --- a/src/helpers/indexable-helper.php +++ b/src/helpers/indexable-helper.php @@ -262,4 +262,49 @@ public function check_if_default_field( $indexable, $field ) { return false; } + + /** + * Saves and returns an indexable (on production environments only). + * + * Moved from Yoast\WP\SEO\Builders\Indexable_Builder. + * + * @param Indexable $indexable The indexable. + * @param Indexable|null $indexable_before The indexable before possible changes. + * + * @return bool True if default value. + */ + public function save_indexable( $indexable, $indexable_before ) { + $intend_to_save = $this->should_index_indexables(); + + /** + * Filter: 'wpseo_should_save_indexable' - Allow developers to enable / disable + * saving the indexable when the indexable is updated. Warning: overriding + * the intended action may cause problems when moving from a staging to a + * production environment because indexable permalinks may get set incorrectly. + * + * @param bool $intend_to_save True if YoastSEO intends to save the indexable. + * @param Indexable $indexable The indexable to be saved. + */ + $intend_to_save = \apply_filters( 'wpseo_should_save_indexable', $intend_to_save, $indexable ); + + if ( ! $intend_to_save ) { + return $indexable; + } + + // Save the indexable before running the WordPress hook. + $indexable->save(); + + if ( $indexable_before ) { + /** + * Action: 'wpseo_save_indexable' - Allow developers to perform an action + * when the indexable is updated. + * + * @param Indexable $indexable The saved indexable. + * @param Indexable $indexable_before The indexable before saving. + */ + \do_action( 'wpseo_save_indexable', $indexable, $indexable_before ); + } + + return $indexable; + } } From 9c1fc3f0952d44f3952bece331d2b4078c4f008c Mon Sep 17 00:00:00 2001 From: Leonidas Milosis Date: Thu, 25 Apr 2024 11:33:13 +0300 Subject: [PATCH 02/35] Don't create indexables when filter is used and a post is created/updated --- src/helpers/indexable-helper.php | 2 +- .../watchers/indexable-post-watcher.php | 19 +++++++-- .../Watchers/Indexable_Post_Watcher_Test.php | 42 ++++++++++++++----- 3 files changed, 48 insertions(+), 15 deletions(-) diff --git a/src/helpers/indexable-helper.php b/src/helpers/indexable-helper.php index ea98ee6b4fd..f5cd6ab2b7c 100644 --- a/src/helpers/indexable-helper.php +++ b/src/helpers/indexable-helper.php @@ -273,7 +273,7 @@ public function check_if_default_field( $indexable, $field ) { * * @return bool True if default value. */ - public function save_indexable( $indexable, $indexable_before ) { + public function save_indexable( $indexable, $indexable_before = null ) { $intend_to_save = $this->should_index_indexables(); /** diff --git a/src/integrations/watchers/indexable-post-watcher.php b/src/integrations/watchers/indexable-post-watcher.php index c76b6c10124..be9a60583e4 100644 --- a/src/integrations/watchers/indexable-post-watcher.php +++ b/src/integrations/watchers/indexable-post-watcher.php @@ -8,6 +8,7 @@ use Yoast\WP\SEO\Builders\Indexable_Link_Builder; use Yoast\WP\SEO\Conditionals\Migrations_Conditional; use Yoast\WP\SEO\Helpers\Author_Archive_Helper; +use Yoast\WP\SEO\Helpers\Indexable_Helper; use Yoast\WP\SEO\Helpers\Post_Helper; use Yoast\WP\SEO\Integrations\Cleanup_Integration; use Yoast\WP\SEO\Integrations\Integration_Interface; @@ -59,6 +60,13 @@ class Indexable_Post_Watcher implements Integration_Interface { */ private $author_archive; + /** + * The indexable helper. + * + * @var Indexable_Helper + */ + private $indexable_helper; + /** * Holds the Post_Helper instance. * @@ -90,6 +98,7 @@ public static function get_conditionals() { * @param Indexable_Hierarchy_Repository $hierarchy_repository The hierarchy repository to use. * @param Indexable_Link_Builder $link_builder The link builder. * @param Author_Archive_Helper $author_archive The author archive helper. + * @param Indexable_Helper $indexable_helper The indexable helper. * @param Post_Helper $post The post helper. * @param Logger $logger The logger. */ @@ -99,6 +108,7 @@ public function __construct( Indexable_Hierarchy_Repository $hierarchy_repository, Indexable_Link_Builder $link_builder, Author_Archive_Helper $author_archive, + Indexable_Helper $indexable_helper, Post_Helper $post, Logger $logger ) { @@ -107,6 +117,7 @@ public function __construct( $this->hierarchy_repository = $hierarchy_repository; $this->link_builder = $link_builder; $this->author_archive = $author_archive; + $this->indexable_helper = $indexable_helper; $this->post = $post; $this->logger = $logger; } @@ -173,7 +184,7 @@ public function updated_indexable( $indexable, $post ) { $this->update_relations( $post ); - $indexable->save(); + $this->indexable_helper->save_indexable( $indexable ); } /** @@ -208,7 +219,7 @@ public function build_indexable( $post_id ) { if ( $post && $indexable && \in_array( $post->post_status, $this->post->get_public_post_statuses(), true ) ) { $this->link_builder->build( $indexable, $post->post_content ); // Save indexable to persist the updated link count. - $indexable->save(); + $this->indexable_helper->save_indexable( $indexable ); $this->updated_indexable( $indexable, $post ); } } catch ( Exception $exception ) { @@ -229,7 +240,7 @@ protected function update_has_public_posts( $indexable ) { $author_indexable = $this->repository->find_by_id_and_type( $indexable->author_id, 'user' ); if ( $author_indexable ) { $author_indexable->has_public_posts = $this->author_archive->author_has_public_posts( $author_indexable->object_id ); - $author_indexable->save(); + $this->indexable_helper->save_indexable( $author_indexable ); $this->reschedule_cleanup_if_author_has_no_posts( $author_indexable ); } @@ -273,7 +284,7 @@ protected function update_relations( $post ) { // Ignore everything that is not an actual indexable. if ( \is_a( $indexable, Indexable::class ) ) { $indexable->object_last_modified = \max( $indexable->object_last_modified, $post->post_modified_gmt ); - $indexable->save(); + $this->indexable_helper->save_indexable( $indexable ); } } } diff --git a/tests/Unit/Integrations/Watchers/Indexable_Post_Watcher_Test.php b/tests/Unit/Integrations/Watchers/Indexable_Post_Watcher_Test.php index 228572f4bb8..ea5d8b69b07 100644 --- a/tests/Unit/Integrations/Watchers/Indexable_Post_Watcher_Test.php +++ b/tests/Unit/Integrations/Watchers/Indexable_Post_Watcher_Test.php @@ -10,6 +10,7 @@ use Yoast\WP\SEO\Builders\Indexable_Link_Builder; use Yoast\WP\SEO\Conditionals\Migrations_Conditional; use Yoast\WP\SEO\Helpers\Author_Archive_Helper; +use Yoast\WP\SEO\Helpers\Indexable_Helper; use Yoast\WP\SEO\Helpers\Post_Helper; use Yoast\WP\SEO\Integrations\Cleanup_Integration; use Yoast\WP\SEO\Integrations\Watchers\Indexable_Post_Watcher; @@ -55,7 +56,7 @@ final class Indexable_Post_Watcher_Test extends TestCase { /** * The link builder. * - * @var Indexable_Link_Builder + * @var Mockery\MockInterface|Indexable_Link_Builder */ protected $link_builder; @@ -66,6 +67,13 @@ final class Indexable_Post_Watcher_Test extends TestCase { */ private $author_archive; + /** + * The indexable helper mock. + * + * @var Mockery\MockInterface|Indexable_Helper + */ + private $indexable_helper; + /** * Represents the class we are testing. * @@ -100,6 +108,7 @@ protected function set_up() { $this->hierarchy_repository = Mockery::mock( Indexable_Hierarchy_Repository::class ); $this->link_builder = Mockery::mock( Indexable_Link_Builder::class ); $this->author_archive = Mockery::mock( Author_Archive_Helper::class ); + $this->indexable_helper = Mockery::mock( Indexable_Helper::class ); $this->post = Mockery::mock( Post_Helper::class ); $this->logger = Mockery::mock( Logger::class ); $this->instance = Mockery::mock( @@ -110,6 +119,7 @@ protected function set_up() { $this->hierarchy_repository, $this->link_builder, $this->author_archive, + $this->indexable_helper, $this->post, $this->logger, ] @@ -219,10 +229,6 @@ public function test_build_indexable() { $indexable_mock = Mockery::mock( Indexable_Mock::class ); - $indexable_mock - ->expects( 'save' ) - ->once(); - $this->repository ->expects( 'find_by_id_and_type' ) ->once() @@ -252,6 +258,10 @@ public function test_build_indexable() { ->once() ->with( $indexable_mock, $post_content ); + $this->indexable_helper + ->expects( 'save_indexable' ) + ->once(); + $this->instance ->expects( 'update_has_public_posts' ) ->with( $indexable_mock ) @@ -328,7 +338,6 @@ public function test_build_indexable_does_not_exist() { ]; $indexable_mock = Mockery::mock( Indexable_Mock::class ); - $indexable_mock->expects( 'save' )->once(); $this->repository->expects( 'find_by_id_and_type' )->once()->with( $post_id, 'post', false )->andReturn( false ); $this->builder->expects( 'build_for_id_and_type' )->once()->with( $post_id, 'post', false )->andReturn( $indexable_mock ); @@ -349,6 +358,10 @@ public function test_build_indexable_does_not_exist() { ->once() ->andReturn( [ 'publish' ] ); + $this->indexable_helper + ->expects( 'save_indexable' ) + ->once(); + $this->instance ->expects( 'update_has_public_posts' ) ->with( $indexable_mock ) @@ -404,7 +417,10 @@ public function test_update_has_public_posts_with_post() { ->with( 11 ) ->once() ->andReturn( true ); - $author_indexable->expects( 'save' )->once(); + + $this->indexable_helper + ->expects( 'save_indexable' ) + ->once(); $this->post->expects( 'update_has_public_posts_on_attachments' )->once()->with( 33, null )->andReturnTrue(); @@ -490,8 +506,6 @@ public function test_reschedule_cleanup_when_author_does_not_have_posts() { $author_indexable = Mockery::mock( Indexable_Mock::class ); $author_indexable->object_id = 11; - $author_indexable->expects( 'save' ); - $this->repository->expects( 'find_by_id_and_type' ) ->with( 11, 'user' ) ->once() @@ -499,6 +513,11 @@ public function test_reschedule_cleanup_when_author_does_not_have_posts() { $this->author_archive->expects( 'author_has_public_posts' ) ->with( 11 ) ->andReturnFalse(); + + $this->indexable_helper + ->expects( 'save_indexable' ) + ->once(); + $this->post->expects( 'update_has_public_posts_on_attachments' ) ->once() ->with( 33, null ) @@ -531,7 +550,6 @@ public function test_update_relations() { $indexable = Mockery::mock( Indexable_Mock::class ); $indexable->object_last_modified = '1234-12-12 00:00:00'; - $indexable->expects( 'save' )->once(); $this->instance ->expects( 'get_related_indexables' ) @@ -539,6 +557,10 @@ public function test_update_relations() { ->with( $post ) ->andReturn( [ $indexable ] ); + $this->indexable_helper + ->expects( 'save_indexable' ) + ->once(); + $this->instance->update_relations( $post ); $this->assertSame( '1234-12-12 12:12:12', $indexable->object_last_modified ); From 89f5f7c431f5cdd0922cef107fdf96eb2b62fc60 Mon Sep 17 00:00:00 2001 From: Leonidas Milosis Date: Thu, 25 Apr 2024 12:45:21 +0300 Subject: [PATCH 03/35] Stop saving indexable a second time needlessly --- src/integrations/watchers/indexable-post-watcher.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/integrations/watchers/indexable-post-watcher.php b/src/integrations/watchers/indexable-post-watcher.php index be9a60583e4..21564d94c28 100644 --- a/src/integrations/watchers/indexable-post-watcher.php +++ b/src/integrations/watchers/indexable-post-watcher.php @@ -183,8 +183,6 @@ public function updated_indexable( $indexable, $post ) { } $this->update_relations( $post ); - - $this->indexable_helper->save_indexable( $indexable ); } /** From 3c860fc1b94f9bde5f50ef91c7f93f89e49ab1ef Mon Sep 17 00:00:00 2001 From: Leonidas Milosis Date: Thu, 25 Apr 2024 15:16:28 +0300 Subject: [PATCH 04/35] Don't create indexables when filter is used and a term is created/updated --- .../watchers/indexable-term-watcher.php | 29 +++++++++++++------ .../Watchers/Indexable_Term_Watcher_Test.php | 26 ++++++++++++----- 2 files changed, 38 insertions(+), 17 deletions(-) diff --git a/src/integrations/watchers/indexable-term-watcher.php b/src/integrations/watchers/indexable-term-watcher.php index 0dec8710054..338d5cdffb4 100644 --- a/src/integrations/watchers/indexable-term-watcher.php +++ b/src/integrations/watchers/indexable-term-watcher.php @@ -5,6 +5,7 @@ use Yoast\WP\SEO\Builders\Indexable_Builder; use Yoast\WP\SEO\Builders\Indexable_Link_Builder; use Yoast\WP\SEO\Conditionals\Migrations_Conditional; +use Yoast\WP\SEO\Helpers\Indexable_Helper; use Yoast\WP\SEO\Helpers\Site_Helper; use Yoast\WP\SEO\Integrations\Integration_Interface; use Yoast\WP\SEO\Repositories\Indexable_Repository; @@ -35,6 +36,13 @@ class Indexable_Term_Watcher implements Integration_Interface { */ protected $link_builder; + /** + * The indexable helper. + * + * @var Indexable_Helper + */ + private $indexable_helper; + /** * Represents the site helper. * @@ -54,21 +62,24 @@ public static function get_conditionals() { /** * Indexable_Term_Watcher constructor. * - * @param Indexable_Repository $repository The repository to use. - * @param Indexable_Builder $builder The post builder to use. - * @param Indexable_Link_Builder $link_builder The lint builder to use. - * @param Site_Helper $site The site helper. + * @param Indexable_Repository $repository The repository to use. + * @param Indexable_Builder $builder The post builder to use. + * @param Indexable_Link_Builder $link_builder The lint builder to use. + * @param Indexable_Helper $indexable_helper The indexable helper. + * @param Site_Helper $site The site helper. */ public function __construct( Indexable_Repository $repository, Indexable_Builder $builder, Indexable_Link_Builder $link_builder, + Indexable_Helper $indexable_helper, Site_Helper $site ) { - $this->repository = $repository; - $this->builder = $builder; - $this->link_builder = $link_builder; - $this->site = $site; + $this->repository = $repository; + $this->builder = $builder; + $this->link_builder = $link_builder; + $this->indexable_helper = $indexable_helper; + $this->site = $site; } /** @@ -136,6 +147,6 @@ public function build_indexable( $term_id ) { $this->link_builder->build( $indexable, $term->description ); $indexable->object_last_modified = \max( $indexable->object_last_modified, \current_time( 'mysql' ) ); - $indexable->save(); + $this->indexable_helper->save_indexable( $indexable ); } } diff --git a/tests/Unit/Integrations/Watchers/Indexable_Term_Watcher_Test.php b/tests/Unit/Integrations/Watchers/Indexable_Term_Watcher_Test.php index 194d4b4fbf0..dda79588f9f 100644 --- a/tests/Unit/Integrations/Watchers/Indexable_Term_Watcher_Test.php +++ b/tests/Unit/Integrations/Watchers/Indexable_Term_Watcher_Test.php @@ -7,6 +7,7 @@ use Yoast\WP\SEO\Builders\Indexable_Builder; use Yoast\WP\SEO\Builders\Indexable_Link_Builder; use Yoast\WP\SEO\Conditionals\Migrations_Conditional; +use Yoast\WP\SEO\Helpers\Indexable_Helper; use Yoast\WP\SEO\Helpers\Site_Helper; use Yoast\WP\SEO\Integrations\Watchers\Indexable_Term_Watcher; use Yoast\WP\SEO\Models\Indexable; @@ -46,6 +47,13 @@ final class Indexable_Term_Watcher_Test extends TestCase { */ protected $link_builder; + /** + * Represents the indexable helper. + * + * @var Mockery\MockInterface|Indexable_Helper + */ + private $indexable_helper; + /** * Represents the site helper. * @@ -68,15 +76,17 @@ final class Indexable_Term_Watcher_Test extends TestCase { protected function set_up() { parent::set_up(); - $this->repository = Mockery::mock( Indexable_Repository::class ); - $this->builder = Mockery::mock( Indexable_Builder::class ); - $this->link_builder = Mockery::mock( Indexable_Link_Builder::class ); - $this->site = Mockery::mock( Site_Helper::class ); + $this->repository = Mockery::mock( Indexable_Repository::class ); + $this->builder = Mockery::mock( Indexable_Builder::class ); + $this->link_builder = Mockery::mock( Indexable_Link_Builder::class ); + $this->indexable_helper = Mockery::mock( Indexable_Helper::class ); + $this->site = Mockery::mock( Site_Helper::class ); $this->instance = new Indexable_Term_Watcher( $this->repository, $this->builder, $this->link_builder, + $this->indexable_helper, $this->site ); } @@ -205,8 +215,8 @@ public function test_build_indexable() { 'This is a term description, with a link.' ); - $indexable - ->expects( 'save' ) + $this->indexable_helper + ->expects( 'save_indexable' ) ->once(); $this->instance->build_indexable( 1 ); @@ -369,8 +379,8 @@ public function test_build_does_not_exist() { 'This is a term description, with a link.' ); - $indexable - ->expects( 'save' ) + $this->indexable_helper + ->expects( 'save_indexable' ) ->once(); $this->instance->build_indexable( 1 ); From 6b2570ac51628846ed4d234a25dd323a41623748 Mon Sep 17 00:00:00 2001 From: Leonidas Milosis Date: Thu, 25 Apr 2024 15:52:39 +0300 Subject: [PATCH 05/35] Don't create indexables when filter is used and a author is created/updated --- .../watchers/indexable-author-watcher.php | 26 +++++++++++++---- .../Indexable_Author_Watcher_Test.php | 29 +++++++++++++++---- 2 files changed, 44 insertions(+), 11 deletions(-) diff --git a/src/integrations/watchers/indexable-author-watcher.php b/src/integrations/watchers/indexable-author-watcher.php index b224ff9d82b..00960cdafe2 100644 --- a/src/integrations/watchers/indexable-author-watcher.php +++ b/src/integrations/watchers/indexable-author-watcher.php @@ -4,6 +4,7 @@ use Yoast\WP\SEO\Builders\Indexable_Builder; use Yoast\WP\SEO\Conditionals\Migrations_Conditional; +use Yoast\WP\SEO\Helpers\Indexable_Helper; use Yoast\WP\SEO\Integrations\Integration_Interface; use Yoast\WP\SEO\Repositories\Indexable_Repository; @@ -26,6 +27,13 @@ class Indexable_Author_Watcher implements Integration_Interface { */ protected $builder; + /** + * The indexable helper. + * + * @var Indexable_Helper + */ + private $indexable_helper; + /** * Returns the conditionals based on which this loadable should be active. * @@ -38,12 +46,18 @@ public static function get_conditionals() { /** * Indexable_Author_Watcher constructor. * - * @param Indexable_Repository $repository The repository to use. - * @param Indexable_Builder $builder The builder to use. + * @param Indexable_Repository $repository The repository to use. + * @param Indexable_Helper $indexable_helper The indexable helper. + * @param Indexable_Builder $builder The builder to use. */ - public function __construct( Indexable_Repository $repository, Indexable_Builder $builder ) { - $this->repository = $repository; - $this->builder = $builder; + public function __construct( + Indexable_Repository $repository, + Indexable_Helper $indexable_helper, + Indexable_Builder $builder + ) { + $this->repository = $repository; + $this->indexable_helper = $indexable_helper; + $this->builder = $builder; } /** @@ -90,7 +104,7 @@ public function build_indexable( $user_id ) { if ( $indexable ) { $indexable->object_last_modified = \max( $indexable->object_last_modified, \current_time( 'mysql' ) ); - $indexable->save(); + $this->indexable_helper->save_indexable( $indexable ); } } diff --git a/tests/Unit/Integrations/Watchers/Indexable_Author_Watcher_Test.php b/tests/Unit/Integrations/Watchers/Indexable_Author_Watcher_Test.php index ddac0b2cd55..b7d9a610b82 100644 --- a/tests/Unit/Integrations/Watchers/Indexable_Author_Watcher_Test.php +++ b/tests/Unit/Integrations/Watchers/Indexable_Author_Watcher_Test.php @@ -7,6 +7,7 @@ use Yoast\WP\Lib\ORM; use Yoast\WP\SEO\Builders\Indexable_Builder; use Yoast\WP\SEO\Conditionals\Migrations_Conditional; +use Yoast\WP\SEO\Helpers\Indexable_Helper; use Yoast\WP\SEO\Integrations\Watchers\Indexable_Author_Watcher; use Yoast\WP\SEO\Models\Indexable; use Yoast\WP\SEO\Repositories\Indexable_Repository; @@ -38,6 +39,13 @@ final class Indexable_Author_Watcher_Test extends TestCase { */ private $builder; + /** + * Represents the indexable helper. + * + * @var Mockery\MockInterface|Indexable_Helper + */ + private $indexable_helper; + /** * Represents the instance to test. * @@ -53,9 +61,14 @@ final class Indexable_Author_Watcher_Test extends TestCase { protected function set_up() { parent::set_up(); - $this->repository = Mockery::mock( Indexable_Repository::class ); - $this->builder = Mockery::mock( Indexable_Builder::class ); - $this->instance = new Indexable_Author_Watcher( $this->repository, $this->builder ); + $this->repository = Mockery::mock( Indexable_Repository::class ); + $this->indexable_helper = Mockery::mock( Indexable_Helper::class ); + $this->builder = Mockery::mock( Indexable_Builder::class ); + $this->instance = new Indexable_Author_Watcher( + $this->repository, + $this->indexable_helper, + $this->builder + ); } /** @@ -144,7 +157,6 @@ public function test_build_indexable() { $indexable_mock->orm = Mockery::mock( ORM::class ); $indexable_mock->orm->expects( 'get' )->with( 'object_last_modified' )->andReturn( '1234-12-12 00:00:00' ); $indexable_mock->orm->expects( 'set' )->with( 'object_last_modified', '1234-12-12 12:12:12' ); - $indexable_mock->expects( 'save' )->once(); $this->repository ->expects( 'find_by_id_and_type' ) @@ -158,6 +170,10 @@ public function test_build_indexable() { ->with( $id, 'user', $indexable_mock ) ->andReturn( $indexable_mock ); + $this->indexable_helper + ->expects( 'save_indexable' ) + ->once(); + $this->instance->build_indexable( $id ); } @@ -177,7 +193,6 @@ public function test_build_indexable_not_found() { $indexable_mock->orm = Mockery::mock( ORM::class ); $indexable_mock->orm->expects( 'get' )->with( 'object_last_modified' )->andReturn( '1234-12-12 00:00:00' ); $indexable_mock->orm->expects( 'set' )->with( 'object_last_modified', '1234-12-12 12:12:12' ); - $indexable_mock->expects( 'save' )->once(); $this->repository ->expects( 'find_by_id_and_type' ) @@ -191,6 +206,10 @@ public function test_build_indexable_not_found() { ->with( $id, 'user', false ) ->andReturn( $indexable_mock ); + $this->indexable_helper + ->expects( 'save_indexable' ) + ->once(); + $this->instance->build_indexable( $id ); } } From f7ece8d2c82e0c5cd5f6ad10afac7e6ca8489418 Mon Sep 17 00:00:00 2001 From: Leonidas Milosis Date: Fri, 26 Apr 2024 09:45:34 +0300 Subject: [PATCH 06/35] Rename variables to indicate better that they are indexable instances --- src/builders/primary-term-builder.php | 16 ++++++++-------- .../watchers/primary-term-watcher.php | 6 +++--- src/repositories/primary-term-repository.php | 10 +++++----- 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/builders/primary-term-builder.php b/src/builders/primary-term-builder.php index 445827911ee..ce48c36be2a 100644 --- a/src/builders/primary-term-builder.php +++ b/src/builders/primary-term-builder.php @@ -76,21 +76,21 @@ protected function save_primary_term( $post_id, $taxonomy ) { $term_id = $this->meta->get_value( 'primary_' . $taxonomy, $post_id ); $term_selected = ! empty( $term_id ); - $primary_term = $this->repository->find_by_post_id_and_taxonomy( $post_id, $taxonomy, $term_selected ); + $primary_term_indexable = $this->repository->find_by_post_id_and_taxonomy( $post_id, $taxonomy, $term_selected ); // Removes the indexable when no term found. if ( ! $term_selected ) { - if ( $primary_term ) { - $primary_term->delete(); + if ( $primary_term_indexable ) { + $primary_term_indexable->delete(); } return; } - $primary_term->term_id = $term_id; - $primary_term->post_id = $post_id; - $primary_term->taxonomy = $taxonomy; - $primary_term->blog_id = \get_current_blog_id(); - $primary_term->save(); + $primary_term_indexable->term_id = $term_id; + $primary_term_indexable->post_id = $post_id; + $primary_term_indexable->taxonomy = $taxonomy; + $primary_term_indexable->blog_id = \get_current_blog_id(); + $primary_term_indexable->save(); } } diff --git a/src/integrations/watchers/primary-term-watcher.php b/src/integrations/watchers/primary-term-watcher.php index 140fc0cbde3..5eeadb18cf3 100644 --- a/src/integrations/watchers/primary-term-watcher.php +++ b/src/integrations/watchers/primary-term-watcher.php @@ -149,13 +149,13 @@ protected function save_primary_term( $post_id, $taxonomy ) { */ public function delete_primary_terms( $post_id ) { foreach ( $this->primary_term->get_primary_term_taxonomies( $post_id ) as $taxonomy ) { - $primary_term = $this->repository->find_by_post_id_and_taxonomy( $post_id, $taxonomy->name, false ); + $primary_term_indexable = $this->repository->find_by_post_id_and_taxonomy( $post_id, $taxonomy->name, false ); - if ( ! $primary_term ) { + if ( ! $primary_term_indexable ) { continue; } - $primary_term->delete(); + $primary_term_indexable->delete(); } } } diff --git a/src/repositories/primary-term-repository.php b/src/repositories/primary-term-repository.php index 244e34d6aea..8fc0c93e93c 100644 --- a/src/repositories/primary-term-repository.php +++ b/src/repositories/primary-term-repository.php @@ -33,17 +33,17 @@ public function find_by_post_id_and_taxonomy( $post_id, $taxonomy, $auto_create /** * Instance of the primary term. * - * @var Primary_Term $primary_term + * @var Primary_Term $primary_term_indexable */ - $primary_term = $this->query() + $primary_term_indexable = $this->query() ->where( 'post_id', $post_id ) ->where( 'taxonomy', $taxonomy ) ->find_one(); - if ( $auto_create && ! $primary_term ) { - $primary_term = $this->query()->create(); + if ( $auto_create && ! $primary_term_indexable ) { + $primary_term_indexable = $this->query()->create(); } - return $primary_term; + return $primary_term_indexable; } } From e8b0790cfab3542fcaaf0bba83ee76a27d1c6398 Mon Sep 17 00:00:00 2001 From: Leonidas Milosis Date: Fri, 26 Apr 2024 09:52:53 +0300 Subject: [PATCH 07/35] Don't create primary term indexables when filter is used --- src/builders/primary-term-builder.php | 29 +++++++++----- .../Builders/Primary_Term_Builder_Test.php | 40 +++++++++++++++---- 2 files changed, 53 insertions(+), 16 deletions(-) diff --git a/src/builders/primary-term-builder.php b/src/builders/primary-term-builder.php index ce48c36be2a..a636776a566 100644 --- a/src/builders/primary-term-builder.php +++ b/src/builders/primary-term-builder.php @@ -2,6 +2,7 @@ namespace Yoast\WP\SEO\Builders; +use Yoast\WP\SEO\Helpers\Indexable_Helper; use Yoast\WP\SEO\Helpers\Meta_Helper; use Yoast\WP\SEO\Helpers\Primary_Term_Helper; use Yoast\WP\SEO\Repositories\Primary_Term_Repository; @@ -20,6 +21,13 @@ class Primary_Term_Builder { */ protected $repository; + /** + * The indexable helper. + * + * @var Indexable_Helper + */ + private $indexable_helper; + /** * The primary term helper. * @@ -37,18 +45,21 @@ class Primary_Term_Builder { /** * Primary_Term_Builder constructor. * - * @param Primary_Term_Repository $repository The primary term repository. - * @param Primary_Term_Helper $primary_term The primary term helper. - * @param Meta_Helper $meta The meta helper. + * @param Primary_Term_Repository $repository The primary term repository. + * @param Indexable_Helper $indexable_helper The indexable helper. + * @param Primary_Term_Helper $primary_term The primary term helper. + * @param Meta_Helper $meta The meta helper. */ public function __construct( Primary_Term_Repository $repository, + Indexable_Helper $indexable_helper, Primary_Term_Helper $primary_term, Meta_Helper $meta ) { - $this->repository = $repository; - $this->primary_term = $primary_term; - $this->meta = $meta; + $this->repository = $repository; + $this->indexable_helper = $indexable_helper; + $this->primary_term = $primary_term; + $this->meta = $meta; } /** @@ -75,8 +86,8 @@ public function build( $post_id ) { protected function save_primary_term( $post_id, $taxonomy ) { $term_id = $this->meta->get_value( 'primary_' . $taxonomy, $post_id ); - $term_selected = ! empty( $term_id ); - $primary_term_indexable = $this->repository->find_by_post_id_and_taxonomy( $post_id, $taxonomy, $term_selected ); + $term_selected = ! empty( $term_id ); + $primary_term_indexable = $this->repository->find_by_post_id_and_taxonomy( $post_id, $taxonomy, $term_selected ); // Removes the indexable when no term found. if ( ! $term_selected ) { @@ -91,6 +102,6 @@ protected function save_primary_term( $post_id, $taxonomy ) { $primary_term_indexable->post_id = $post_id; $primary_term_indexable->taxonomy = $taxonomy; $primary_term_indexable->blog_id = \get_current_blog_id(); - $primary_term_indexable->save(); + $this->indexable_helper->save_indexable( $primary_term_indexable ); } } diff --git a/tests/Unit/Builders/Primary_Term_Builder_Test.php b/tests/Unit/Builders/Primary_Term_Builder_Test.php index 89bf049dc77..fb11f359791 100644 --- a/tests/Unit/Builders/Primary_Term_Builder_Test.php +++ b/tests/Unit/Builders/Primary_Term_Builder_Test.php @@ -5,6 +5,7 @@ use Brain\Monkey\Functions; use Mockery; use Yoast\WP\SEO\Builders\Primary_Term_Builder; +use Yoast\WP\SEO\Helpers\Indexable_Helper; use Yoast\WP\SEO\Helpers\Meta_Helper; use Yoast\WP\SEO\Helpers\Primary_Term_Helper; use Yoast\WP\SEO\Models\Primary_Term; @@ -37,6 +38,13 @@ final class Primary_Term_Builder_Test extends TestCase { */ private $repository; + /** + * Holds the indexable helper. + * + * @var Mockery\MockInterface|Indexable_Helper + */ + private $indexable_helper; + /** * Holds the primary term helper. * @@ -59,12 +67,18 @@ final class Primary_Term_Builder_Test extends TestCase { protected function set_up() { parent::set_up(); - $this->repository = Mockery::mock( Primary_Term_Repository::class ); - $this->primary_term = Mockery::mock( Primary_Term_Helper::class ); - $this->meta = Mockery::mock( Meta_Helper::class ); - $this->instance = Mockery::mock( + $this->repository = Mockery::mock( Primary_Term_Repository::class ); + $this->indexable_helper = Mockery::mock( Indexable_Helper::class ); + $this->primary_term = Mockery::mock( Primary_Term_Helper::class ); + $this->meta = Mockery::mock( Meta_Helper::class ); + $this->instance = Mockery::mock( Primary_Term_Builder_Double::class, - [ $this->repository, $this->primary_term, $this->meta ] + [ + $this->repository, + $this->indexable_helper, + $this->primary_term, + $this->meta, + ] ) ->shouldAllowMockingProtectedMethods() ->makePartial(); @@ -80,6 +94,7 @@ protected function set_up() { public function test_successfully_creates_primary_term_builder() { $primary_term_builder = new Primary_Term_Builder( $this->repository, + $this->indexable_helper, $this->primary_term, $this->meta ); @@ -89,6 +104,11 @@ public function test_successfully_creates_primary_term_builder() { $this->getPropertyValue( $primary_term_builder, 'repository' ) ); + $this->assertInstanceOf( + Indexable_Helper::class, + $this->getPropertyValue( $primary_term_builder, 'indexable_helper' ) + ); + $this->assertInstanceOf( Primary_Term_Helper::class, $this->getPropertyValue( $primary_term_builder, 'primary_term' ) @@ -168,7 +188,6 @@ public function test_build_empty_taxonomies() { */ public function test_save_primary_term() { $primary_term = Mockery::mock( Primary_Term_Mock::class ); - $primary_term->expects( 'save' )->once(); Functions\expect( 'get_current_blog_id' )->once()->andReturn( 1 ); @@ -183,6 +202,10 @@ public function test_save_primary_term() { ->with( 'primary_category', 1 ) ->andReturn( 1337 ); + $this->indexable_helper + ->expects( 'save_indexable' ) + ->once(); + $this->instance->save_primary_term( 1, 'category' ); $this->assertEquals( 1337, $primary_term->term_id ); @@ -200,7 +223,6 @@ public function test_save_primary_term() { */ public function test_save_primary_term_of_custom_taxonomy() { $primary_term = Mockery::mock( Primary_Term_Mock::class ); - $primary_term->expects( 'save' )->once(); Functions\expect( 'get_current_blog_id' )->once()->andReturn( 1 ); @@ -215,6 +237,10 @@ public function test_save_primary_term_of_custom_taxonomy() { ->with( 'primary_custom', 1 ) ->andReturn( 1337 ); + $this->indexable_helper + ->expects( 'save_indexable' ) + ->once(); + $this->instance->save_primary_term( 1, 'custom' ); $this->assertEquals( 1337, $primary_term->term_id ); From 5c216de390a7d07ac97b845123286672eabffad3 Mon Sep 17 00:00:00 2001 From: Leonidas Milosis Date: Fri, 26 Apr 2024 13:09:47 +0300 Subject: [PATCH 08/35] Don't create indexable for homepage when the relevant options are saved --- .../watchers/indexable-home-page-watcher.php | 29 +++++++++++++++---- .../Indexable_Home_Page_Watcher_Test.php | 24 +++++++++++++-- 2 files changed, 45 insertions(+), 8 deletions(-) diff --git a/src/integrations/watchers/indexable-home-page-watcher.php b/src/integrations/watchers/indexable-home-page-watcher.php index bbe8a193fca..913780c62b1 100644 --- a/src/integrations/watchers/indexable-home-page-watcher.php +++ b/src/integrations/watchers/indexable-home-page-watcher.php @@ -4,6 +4,7 @@ use Yoast\WP\SEO\Builders\Indexable_Builder; use Yoast\WP\SEO\Conditionals\Migrations_Conditional; +use Yoast\WP\SEO\Helpers\Indexable_Helper; use Yoast\WP\SEO\Integrations\Integration_Interface; use Yoast\WP\SEO\Repositories\Indexable_Repository; @@ -21,6 +22,13 @@ class Indexable_Home_Page_Watcher implements Integration_Interface { */ protected $repository; + /** + * The indexable helper. + * + * @var Indexable_Helper + */ + protected $indexable_helper; + /** * The indexable builder. * @@ -40,12 +48,18 @@ public static function get_conditionals() { /** * Indexable_Home_Page_Watcher constructor. * - * @param Indexable_Repository $repository The repository to use. - * @param Indexable_Builder $builder The post builder to use. + * @param Indexable_Repository $repository The repository to use. + * @param Indexable_Helper $indexable_helper The indexable helper. + * @param Indexable_Builder $builder The post builder to use. */ - public function __construct( Indexable_Repository $repository, Indexable_Builder $builder ) { - $this->repository = $repository; - $this->builder = $builder; + public function __construct( + Indexable_Repository $repository, + Indexable_Helper $indexable_helper, + Indexable_Builder $builder + ) { + $this->repository = $repository; + $this->indexable_helper = $indexable_helper; + $this->builder = $builder; } /** @@ -108,6 +122,11 @@ public function check_option( $old_value, $new_value, $option ) { */ public function build_indexable() { $indexable = $this->repository->find_for_home_page( false ); + + if ( $indexable === false && ! $this->indexable_helper->should_index_indexables() ) { + return; + } + $indexable = $this->builder->build_for_home_page( $indexable ); if ( $indexable ) { diff --git a/tests/Unit/Integrations/Watchers/Indexable_Home_Page_Watcher_Test.php b/tests/Unit/Integrations/Watchers/Indexable_Home_Page_Watcher_Test.php index aea176e05d4..5daeae0d21c 100644 --- a/tests/Unit/Integrations/Watchers/Indexable_Home_Page_Watcher_Test.php +++ b/tests/Unit/Integrations/Watchers/Indexable_Home_Page_Watcher_Test.php @@ -6,6 +6,7 @@ use Mockery; use Yoast\WP\SEO\Builders\Indexable_Builder; use Yoast\WP\SEO\Conditionals\Migrations_Conditional; +use Yoast\WP\SEO\Helpers\Indexable_Helper; use Yoast\WP\SEO\Integrations\Watchers\Indexable_Home_Page_Watcher; use Yoast\WP\SEO\Models\Indexable; use Yoast\WP\SEO\Repositories\Indexable_Repository; @@ -30,6 +31,13 @@ final class Indexable_Home_Page_Watcher_Test extends TestCase { */ private $repository; + /** + * Represents the indexable helper. + * + * @var Mockery\MockInterface|Indexable_Helper + */ + private $indexable_helper; + /** * Represents the indexable builder. * @@ -52,9 +60,14 @@ final class Indexable_Home_Page_Watcher_Test extends TestCase { protected function set_up() { parent::set_up(); - $this->repository = Mockery::mock( Indexable_Repository::class ); - $this->builder = Mockery::mock( Indexable_Builder::class ); - $this->instance = new Indexable_Home_Page_Watcher( $this->repository, $this->builder ); + $this->repository = Mockery::mock( Indexable_Repository::class ); + $this->indexable_helper = Mockery::mock( Indexable_Helper::class ); + $this->builder = Mockery::mock( Indexable_Builder::class ); + $this->instance = new Indexable_Home_Page_Watcher( + $this->repository, + $this->indexable_helper, + $this->builder + ); } /** @@ -205,6 +218,11 @@ public function test_build_indexable_without_indexable() { ->with( false ) ->andReturn( false ); + $this->indexable_helper + ->expects( 'should_index_indexables' ) + ->once() + ->andReturn( true ); + $this->builder ->expects( 'build_for_home_page' ) ->once() From 50aee98b0679adbad7e2e02a788b34f3ed7e7fd4 Mon Sep 17 00:00:00 2001 From: Leonidas Milosis Date: Fri, 26 Apr 2024 15:27:04 +0300 Subject: [PATCH 09/35] Don't create indexable for post archives when the relevant options are saved --- .../indexable-post-type-archive-watcher.php | 26 ++++++++--- ...dexable_Post_Type_Archive_Watcher_Test.php | 45 +++++++++++++++---- 2 files changed, 56 insertions(+), 15 deletions(-) diff --git a/src/integrations/watchers/indexable-post-type-archive-watcher.php b/src/integrations/watchers/indexable-post-type-archive-watcher.php index 6b5eb237641..2a2f8edc3c6 100644 --- a/src/integrations/watchers/indexable-post-type-archive-watcher.php +++ b/src/integrations/watchers/indexable-post-type-archive-watcher.php @@ -4,6 +4,7 @@ use Yoast\WP\SEO\Builders\Indexable_Builder; use Yoast\WP\SEO\Conditionals\Migrations_Conditional; +use Yoast\WP\SEO\Helpers\Indexable_Helper; use Yoast\WP\SEO\Integrations\Integration_Interface; use Yoast\WP\SEO\Repositories\Indexable_Repository; @@ -21,6 +22,13 @@ class Indexable_Post_Type_Archive_Watcher implements Integration_Interface { */ protected $repository; + /** + * The indexable helper. + * + * @var Indexable_Helper + */ + private $indexable_helper; + /** * The indexable builder. * @@ -40,12 +48,18 @@ public static function get_conditionals() { /** * Indexable_Post_Type_Archive_Watcher constructor. * - * @param Indexable_Repository $repository The repository to use. - * @param Indexable_Builder $builder The post builder to use. + * @param Indexable_Repository $repository The repository to use. + * @param Indexable_Helper $indexable_helper The indexable helper. + * @param Indexable_Builder $builder The post builder to use. */ - public function __construct( Indexable_Repository $repository, Indexable_Builder $builder ) { - $this->repository = $repository; - $this->builder = $builder; + public function __construct( + Indexable_Repository $repository, + Indexable_Helper $indexable_helper, + Indexable_Builder $builder + ) { + $this->repository = $repository; + $this->indexable_helper = $indexable_helper; + $this->builder = $builder; } /** @@ -127,7 +141,7 @@ public function build_indexable( $post_type ) { if ( $indexable ) { $indexable->object_last_modified = \max( $indexable->object_last_modified, \current_time( 'mysql' ) ); - $indexable->save(); + $this->indexable_helper->save_indexable( $indexable ); } } } diff --git a/tests/Unit/Integrations/Watchers/Indexable_Post_Type_Archive_Watcher_Test.php b/tests/Unit/Integrations/Watchers/Indexable_Post_Type_Archive_Watcher_Test.php index 32780c46841..4a421416228 100644 --- a/tests/Unit/Integrations/Watchers/Indexable_Post_Type_Archive_Watcher_Test.php +++ b/tests/Unit/Integrations/Watchers/Indexable_Post_Type_Archive_Watcher_Test.php @@ -6,6 +6,7 @@ use Mockery; use Yoast\WP\SEO\Builders\Indexable_Builder; use Yoast\WP\SEO\Conditionals\Migrations_Conditional; +use Yoast\WP\SEO\Helpers\Indexable_Helper; use Yoast\WP\SEO\Integrations\Watchers\Indexable_Post_Type_Archive_Watcher; use Yoast\WP\SEO\Models\Indexable; use Yoast\WP\SEO\Repositories\Indexable_Repository; @@ -32,6 +33,13 @@ final class Indexable_Post_Type_Archive_Watcher_Test extends TestCase { */ private $repository; + /** + * Represents the indexable helper. + * + * @var Mockery\MockInterface|Indexable_Helper + */ + private $indexable_helper; + /** * Represents the indexable builder. * @@ -54,9 +62,14 @@ final class Indexable_Post_Type_Archive_Watcher_Test extends TestCase { protected function set_up() { parent::set_up(); - $this->repository = Mockery::mock( Indexable_Repository::class ); - $this->builder = Mockery::mock( Indexable_Builder::class ); - $this->instance = new Indexable_Post_Type_Archive_Watcher( $this->repository, $this->builder ); + $this->repository = Mockery::mock( Indexable_Repository::class ); + $this->indexable_helper = Mockery::mock( Indexable_Helper::class ); + $this->builder = Mockery::mock( Indexable_Builder::class ); + $this->instance = new Indexable_Post_Type_Archive_Watcher( + $this->repository, + $this->indexable_helper, + $this->builder + ); } /** @@ -101,7 +114,6 @@ public function test_check_option_first_time_save() { $indexable_mock->orm = Mockery::mock( ORM::class ); $indexable_mock->orm->expects( 'get' )->with( 'object_last_modified' )->andReturn( '1234-12-12 00:00:00' ); $indexable_mock->orm->expects( 'set' )->with( 'object_last_modified', '1234-12-12 12:12:12' ); - $indexable_mock->expects( 'save' )->once(); $this->repository ->expects( 'find_for_post_type_archive' ) @@ -115,6 +127,10 @@ public function test_check_option_first_time_save() { ->with( 'my-post-type', $indexable_mock ) ->andReturn( $indexable_mock ); + $this->indexable_helper + ->expects( 'save_indexable' ) + ->once(); + $this->assertTrue( $this->instance->check_option( false, [ 'title-ptarchive-my-post-type' => 'baz' ] ) ); } @@ -146,7 +162,6 @@ public function test_update_wpseo_titles_value() { $indexable_mock->orm = Mockery::mock( ORM::class ); $indexable_mock->orm->expects( 'get' )->with( 'object_last_modified' )->andReturn( '1234-12-12 00:00:00' ); $indexable_mock->orm->expects( 'set' )->with( 'object_last_modified', '1234-12-12 12:12:12' ); - $indexable_mock->expects( 'save' )->once(); $this->repository ->expects( 'find_for_post_type_archive' ) @@ -160,6 +175,10 @@ public function test_update_wpseo_titles_value() { ->with( 'my-post-type', $indexable_mock ) ->andReturn( $indexable_mock ); + $this->indexable_helper + ->expects( 'save_indexable' ) + ->once(); + $this->assertTrue( $this->instance->check_option( [ 'title-ptarchive-my-post-type' => 'bar' ], [ 'title-ptarchive-my-post-type' => 'baz' ] ) ); } @@ -179,7 +198,6 @@ public function test_update_wpseo_titles_value_new() { $indexable_mock->orm = Mockery::mock( ORM::class ); $indexable_mock->orm->expects( 'get' )->with( 'object_last_modified' )->andReturn( '1234-12-12 00:00:00' ); $indexable_mock->orm->expects( 'set' )->with( 'object_last_modified', '1234-12-12 12:12:12' ); - $indexable_mock->expects( 'save' )->once(); $this->repository ->expects( 'find_for_post_type_archive' ) @@ -193,6 +211,10 @@ public function test_update_wpseo_titles_value_new() { ->with( 'my-post-type', $indexable_mock ) ->andReturn( $indexable_mock ); + $this->indexable_helper + ->expects( 'save_indexable' ) + ->once(); + $this->assertTrue( $this->instance->check_option( [], [ 'title-ptarchive-my-post-type' => 'baz' ] ) ); } @@ -212,13 +234,11 @@ public function test_update_wpseo_titles_value_switched() { $indexable_mock->orm = Mockery::mock( ORM::class ); $indexable_mock->orm->expects( 'get' )->with( 'object_last_modified' )->andReturn( '1234-12-12 00:00:00' ); $indexable_mock->orm->expects( 'set' )->with( 'object_last_modified', '1234-12-12 12:12:12' ); - $indexable_mock->expects( 'save' )->once(); $other_indexable_mock = Mockery::mock( Indexable::class ); $other_indexable_mock->orm = Mockery::mock( ORM::class ); $other_indexable_mock->orm->expects( 'get' )->with( 'object_last_modified' )->andReturn( '1234-12-12 00:00:00' ); $other_indexable_mock->orm->expects( 'set' )->with( 'object_last_modified', '1234-12-12 12:12:12' ); - $other_indexable_mock->expects( 'save' )->once(); $this->repository ->expects( 'find_for_post_type_archive' ) @@ -244,6 +264,10 @@ public function test_update_wpseo_titles_value_switched() { ->with( 'other-post-type', $other_indexable_mock ) ->andReturn( $other_indexable_mock ); + $this->indexable_helper + ->expects( 'save_indexable' ) + ->times( 2 ); + $this->assertTrue( $this->instance->check_option( [ 'title-ptarchive-my-post-type' => 'baz' ], [ 'title-ptarchive-other-post-type' => 'baz' ] ) ); } @@ -290,7 +314,6 @@ public function test_build_indexable_without_indexable() { $indexable_mock->orm = Mockery::mock( ORM::class ); $indexable_mock->orm->expects( 'get' )->with( 'object_last_modified' )->andReturn( '1234-12-12 00:00:00' ); $indexable_mock->orm->expects( 'set' )->with( 'object_last_modified', '1234-12-12 12:12:12' ); - $indexable_mock->expects( 'save' )->once(); $this->repository ->expects( 'find_for_post_type_archive' ) @@ -304,6 +327,10 @@ public function test_build_indexable_without_indexable() { ->with( 'my-post-type', false ) ->andReturn( $indexable_mock ); + $this->indexable_helper + ->expects( 'save_indexable' ) + ->once(); + $this->instance->build_indexable( 'my-post-type' ); } } From e1ffac0109135dfa0088b77280c1bd387b76cf72 Mon Sep 17 00:00:00 2001 From: Leonidas Milosis Date: Mon, 29 Apr 2024 10:14:26 +0300 Subject: [PATCH 10/35] Don't create indexable for posts when AIOSEO data is imported --- src/actions/importing/aioseo/aioseo-posts-importing-action.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/actions/importing/aioseo/aioseo-posts-importing-action.php b/src/actions/importing/aioseo/aioseo-posts-importing-action.php index 86f500eaecb..257bdfdd207 100644 --- a/src/actions/importing/aioseo/aioseo-posts-importing-action.php +++ b/src/actions/importing/aioseo/aioseo-posts-importing-action.php @@ -284,7 +284,7 @@ public function index() { if ( $this->indexable_helper->check_if_default_indexable( $indexable, $check_defaults_fields ) ) { $indexable = $this->map( $indexable, $aioseo_indexable ); - $indexable->save(); + $this->indexable_helper->save_indexable( $indexable ); // To ensure that indexables can be rebuild after a reset, we have to store the data in the postmeta table too. $this->indexable_to_postmeta->map_to_postmeta( $indexable ); From 458cd5a44aed2af31c748707022ba67f43888e3d Mon Sep 17 00:00:00 2001 From: Leonidas Milosis Date: Mon, 29 Apr 2024 14:43:01 +0300 Subject: [PATCH 11/35] Don't create indexable when hierarchy permalinks change --- .../watchers/indexable-ancestor-watcher.php | 13 ++++++++++++- .../Indexable_Ancestor_Watcher_Test.php | 17 ++++++++++++++--- .../Indexable_Ancestor_Watcher_Test.php | 10 ++++++++++ 3 files changed, 36 insertions(+), 4 deletions(-) diff --git a/src/integrations/watchers/indexable-ancestor-watcher.php b/src/integrations/watchers/indexable-ancestor-watcher.php index 88024bbb627..9c16794bfdc 100644 --- a/src/integrations/watchers/indexable-ancestor-watcher.php +++ b/src/integrations/watchers/indexable-ancestor-watcher.php @@ -4,6 +4,7 @@ use Yoast\WP\SEO\Builders\Indexable_Hierarchy_Builder; use Yoast\WP\SEO\Conditionals\Migrations_Conditional; +use Yoast\WP\SEO\Helpers\Indexable_Helper; use Yoast\WP\SEO\Helpers\Permalink_Helper; use Yoast\WP\SEO\Helpers\Post_Type_Helper; use Yoast\WP\SEO\Integrations\Integration_Interface; @@ -39,6 +40,13 @@ class Indexable_Ancestor_Watcher implements Integration_Interface { */ protected $indexable_hierarchy_repository; + /** + * The indexable helper. + * + * @var Indexable_Helper + */ + private $indexable_helper; + /** * Represents the permalink helper. * @@ -59,6 +67,7 @@ class Indexable_Ancestor_Watcher implements Integration_Interface { * @param Indexable_Repository $indexable_repository The indexable repository. * @param Indexable_Hierarchy_Builder $indexable_hierarchy_builder The indexable hierarchy builder. * @param Indexable_Hierarchy_Repository $indexable_hierarchy_repository The indexable hierarchy repository. + * @param Indexable_Helper $indexable_helper The indexable helper. * @param Permalink_Helper $permalink_helper The permalink helper. * @param Post_Type_Helper $post_type_helper The post type helper. */ @@ -66,12 +75,14 @@ public function __construct( Indexable_Repository $indexable_repository, Indexable_Hierarchy_Builder $indexable_hierarchy_builder, Indexable_Hierarchy_Repository $indexable_hierarchy_repository, + Indexable_Helper $indexable_helper, Permalink_Helper $permalink_helper, Post_Type_Helper $post_type_helper ) { $this->indexable_repository = $indexable_repository; $this->indexable_hierarchy_builder = $indexable_hierarchy_builder; $this->indexable_hierarchy_repository = $indexable_hierarchy_repository; + $this->indexable_helper = $indexable_helper; $this->permalink_helper = $permalink_helper; $this->post_type_helper = $post_type_helper; } @@ -177,7 +188,7 @@ protected function update_hierarchy_and_permalink( $indexable ) { $this->indexable_hierarchy_builder->build( $indexable ); $indexable->permalink = $this->permalink_helper->get_permalink_for_indexable( $indexable ); - $indexable->save(); + $this->indexable_helper->save_indexable( $indexable ); } } diff --git a/tests/Unit/Integrations/Watchers/Indexable_Ancestor_Watcher_Test.php b/tests/Unit/Integrations/Watchers/Indexable_Ancestor_Watcher_Test.php index a1485aa71fa..48eaa204dc8 100644 --- a/tests/Unit/Integrations/Watchers/Indexable_Ancestor_Watcher_Test.php +++ b/tests/Unit/Integrations/Watchers/Indexable_Ancestor_Watcher_Test.php @@ -6,6 +6,7 @@ use Mockery; use Yoast\WP\SEO\Builders\Indexable_Hierarchy_Builder; use Yoast\WP\SEO\Conditionals\Migrations_Conditional; +use Yoast\WP\SEO\Helpers\Indexable_Helper; use Yoast\WP\SEO\Helpers\Permalink_Helper; use Yoast\WP\SEO\Helpers\Post_Type_Helper; use Yoast\WP\SEO\Integrations\Watchers\Indexable_Ancestor_Watcher; @@ -85,6 +86,7 @@ protected function set_up() { $this->indexable_repository = Mockery::mock( Indexable_Repository::class ); $this->indexable_hierarchy_builder = Mockery::mock( Indexable_Hierarchy_Builder::class ); $this->indexable_hierarchy_repository = Mockery::mock( Indexable_Hierarchy_Repository::class ); + $this->indexable_helper = Mockery::mock( Indexable_Helper::class ); $this->permalink_helper = Mockery::mock( Permalink_Helper::class ); $this->post_type_helper = Mockery::mock( Post_Type_Helper::class ); @@ -92,6 +94,7 @@ protected function set_up() { $this->indexable_repository, $this->indexable_hierarchy_builder, $this->indexable_hierarchy_repository, + $this->indexable_helper, $this->permalink_helper, $this->post_type_helper ); @@ -200,7 +203,6 @@ public function test_reset_children() { $child_indexable = Mockery::mock( Indexable_Mock::class ); $child_indexable->permalink = 'https://example.org/old-child-permalink'; - $child_indexable->expects( 'save' )->once(); $this->indexable_hierarchy_builder->expects( 'build' )->with( $child_indexable ); @@ -221,6 +223,10 @@ public function test_reset_children() { ->with( [ 1 ] ) ->andReturn( [ $child_indexable ] ); + $this->indexable_helper + ->expects( 'save_indexable' ) + ->once(); + $this->assertTrue( $this->instance->reset_children( $indexable, $indexable_before ) ); } @@ -245,7 +251,6 @@ public function test_reset_children_for_term() { $child_indexable->object_type = 'term'; $child_indexable->permalink = 'https://example.org/old-child-permalink'; $child_indexable->object_id = 23; - $child_indexable->expects( 'save' )->once(); $this->indexable_hierarchy_repository ->expects( 'find_children' ) @@ -305,6 +310,10 @@ public function test_reset_children_for_term() { $this->set_expectations_for_update_hierarchy_and_permalink( $indexable_term_1, $indexable_term_2, $additional_indexable ); + $this->indexable_helper + ->expects( 'save_indexable' ) + ->once(); + $this->assertTrue( $this->instance->reset_children( $indexable, $indexable_before ) ); $this->assertSame( 'https://example.org/permalink', $indexable_term_1->permalink ); $this->assertSame( 'https://example.org/permalink', $indexable_term_2->permalink ); @@ -444,7 +453,9 @@ private function set_expectations_for_update_hierarchy_and_permalink( ...$indexa ->with( $indexable ) ->andReturn( 'https://example.org/permalink' ); - $indexable->expects( 'save' ); + $this->indexable_helper + ->expects( 'save_indexable' ) + ->once(); } } } diff --git a/tests/WP/Admin/Watchers/Indexable_Ancestor_Watcher_Test.php b/tests/WP/Admin/Watchers/Indexable_Ancestor_Watcher_Test.php index 332dd594f91..21bf8af4e51 100644 --- a/tests/WP/Admin/Watchers/Indexable_Ancestor_Watcher_Test.php +++ b/tests/WP/Admin/Watchers/Indexable_Ancestor_Watcher_Test.php @@ -6,6 +6,7 @@ use WP_Post; use Yoast\WP\Lib\Model; use Yoast\WP\SEO\Builders\Indexable_Hierarchy_Builder; +use Yoast\WP\SEO\Helpers\Indexable_Helper; use Yoast\WP\SEO\Helpers\Permalink_Helper; use Yoast\WP\SEO\Helpers\Post_Type_Helper; use Yoast\WP\SEO\Repositories\Indexable_Hierarchy_Repository; @@ -45,6 +46,13 @@ final class Indexable_Ancestor_Watcher_Test extends TestCase { */ private $indexable_hierarchy_repository; + /** + * Represents the Indexable_Helper. + * + * @var Mockery\MockInterface|Indexable_Helper + */ + private $indexable_helper; + /** * Represents the Permalink_Helper. * @@ -77,6 +85,7 @@ public function set_up() { $this->indexable_repository = Mockery::mock( Indexable_Repository::class ); $this->indexable_hierarchy_builder = Mockery::mock( Indexable_Hierarchy_Builder::class ); $this->indexable_hierarchy_repository = Mockery::mock( Indexable_Hierarchy_Repository::class ); + $this->indexable_helper = Mockery::mock( Indexable_Helper::class ); $this->permalink_helper = Mockery::mock( Permalink_Helper::class ); $this->post_type_helper = Mockery::mock( Post_Type_Helper::class ); @@ -84,6 +93,7 @@ public function set_up() { $this->indexable_repository, $this->indexable_hierarchy_builder, $this->indexable_hierarchy_repository, + $this->indexable_helper, $this->permalink_helper, $this->post_type_helper ); From 59982760adeee1bab922417f4a2761f73556b293 Mon Sep 17 00:00:00 2001 From: Leonidas Milosis Date: Mon, 29 Apr 2024 18:05:53 +0300 Subject: [PATCH 12/35] Fix unit tests --- .../Abstract_Indexable_Builder_TestCase.php | 14 ++++ .../Build_For_Date_Archive_Test.php | 6 +- .../Build_For_Home_Page_Test.php | 3 +- .../Build_For_Id_And_Type_Test.php | 3 +- .../Build_For_Post_Type_Archive_Test.php | 3 +- .../Build_For_System_Page_Test.php | 3 +- .../Builders/Indexable_Builder/Build_Test.php | 28 ++++---- .../Maybe_Build_Author_Indexable_Test.php | 4 +- .../Indexable_Builder/Save_Indexable_Test.php | 72 +++---------------- .../Builders/Indexable_Builder_Double.php | 2 +- 10 files changed, 46 insertions(+), 92 deletions(-) diff --git a/tests/Unit/Builders/Indexable_Builder/Abstract_Indexable_Builder_TestCase.php b/tests/Unit/Builders/Indexable_Builder/Abstract_Indexable_Builder_TestCase.php index 7541a510121..c96849448d8 100644 --- a/tests/Unit/Builders/Indexable_Builder/Abstract_Indexable_Builder_TestCase.php +++ b/tests/Unit/Builders/Indexable_Builder/Abstract_Indexable_Builder_TestCase.php @@ -197,6 +197,20 @@ public function expect_save_indexable_skip() { ->andReturnFalse(); } + /** + * Expectation in save_indexable. + * + * @param Indexable_Mock $indexable The indexable to expect. + * + * @return void + */ + public function expect_save_indexable( $indexable ) { + $this->indexable_helper + ->expects( 'save_indexable' ) + ->once() + ->andReturn( $indexable ); + } + /** * Expectations for ensure_indexable method. * diff --git a/tests/Unit/Builders/Indexable_Builder/Build_For_Date_Archive_Test.php b/tests/Unit/Builders/Indexable_Builder/Build_For_Date_Archive_Test.php index 9028fcc254f..de8ea612464 100644 --- a/tests/Unit/Builders/Indexable_Builder/Build_For_Date_Archive_Test.php +++ b/tests/Unit/Builders/Indexable_Builder/Build_For_Date_Archive_Test.php @@ -55,8 +55,7 @@ public function test_build_for_date_archive_with_indexable() { ->with( $this->indexable ) ->andReturn( $this->indexable ); - // Saving is outside the scope of this test. - $this->expect_save_indexable_skip(); + $this->expect_save_indexable( $this->indexable ); $this->assertSame( $this->indexable, $this->instance->build_for_date_archive( $this->indexable ) ); } @@ -77,7 +76,6 @@ private function expect_build( $defaults ) { ->with( $this->indexable ) ->andReturn( $this->indexable ); - // Saving is outside the scope of this test. - $this->expect_save_indexable_skip(); + $this->expect_save_indexable( $this->indexable ); } } diff --git a/tests/Unit/Builders/Indexable_Builder/Build_For_Home_Page_Test.php b/tests/Unit/Builders/Indexable_Builder/Build_For_Home_Page_Test.php index 65732cb73be..9d69050f815 100644 --- a/tests/Unit/Builders/Indexable_Builder/Build_For_Home_Page_Test.php +++ b/tests/Unit/Builders/Indexable_Builder/Build_For_Home_Page_Test.php @@ -56,7 +56,6 @@ public function expect_build( $defaults ) { ->with( $this->indexable ) ->andReturn( $this->indexable ); - // Saving is outside the scope of this test. - $this->expect_save_indexable_skip(); + $this->expect_save_indexable( $this->indexable ); } } diff --git a/tests/Unit/Builders/Indexable_Builder/Build_For_Id_And_Type_Test.php b/tests/Unit/Builders/Indexable_Builder/Build_For_Id_And_Type_Test.php index 38759650731..ad067d37ed4 100644 --- a/tests/Unit/Builders/Indexable_Builder/Build_For_Id_And_Type_Test.php +++ b/tests/Unit/Builders/Indexable_Builder/Build_For_Id_And_Type_Test.php @@ -63,7 +63,6 @@ public function expect_build( $defaults ) { $this->expect_maybe_build_author_indexable(); - // Saving is outside the scope of this test. - $this->expect_save_indexable_skip(); + $this->expect_save_indexable( $this->indexable ); } } diff --git a/tests/Unit/Builders/Indexable_Builder/Build_For_Post_Type_Archive_Test.php b/tests/Unit/Builders/Indexable_Builder/Build_For_Post_Type_Archive_Test.php index ee3bea10515..ba163aa3101 100644 --- a/tests/Unit/Builders/Indexable_Builder/Build_For_Post_Type_Archive_Test.php +++ b/tests/Unit/Builders/Indexable_Builder/Build_For_Post_Type_Archive_Test.php @@ -60,7 +60,6 @@ public function expect_build( $defaults ) { ->with( 'post', $this->indexable ) ->andReturn( $this->indexable ); - // Saving is outside the scope of this test. - $this->expect_save_indexable_skip(); + $this->expect_save_indexable( $this->indexable ); } } diff --git a/tests/Unit/Builders/Indexable_Builder/Build_For_System_Page_Test.php b/tests/Unit/Builders/Indexable_Builder/Build_For_System_Page_Test.php index ff3f2a3c4ca..45c8c77e68f 100644 --- a/tests/Unit/Builders/Indexable_Builder/Build_For_System_Page_Test.php +++ b/tests/Unit/Builders/Indexable_Builder/Build_For_System_Page_Test.php @@ -64,7 +64,6 @@ public function expect_build( $defaults ) { ->with( '404', $this->indexable ) ->andReturn( $this->indexable ); - // Saving is outside the scope of this test. - $this->expect_save_indexable_skip(); + $this->expect_save_indexable( $this->indexable ); } } diff --git a/tests/Unit/Builders/Indexable_Builder/Build_Test.php b/tests/Unit/Builders/Indexable_Builder/Build_Test.php index 7acc294c081..7e470b6fba0 100644 --- a/tests/Unit/Builders/Indexable_Builder/Build_Test.php +++ b/tests/Unit/Builders/Indexable_Builder/Build_Test.php @@ -47,7 +47,7 @@ public function test_build() { $this->expect_maybe_build_author_indexable(); - $this->expect_save_indexable_skip(); + $this->expect_save_indexable( $this->indexable ); $this->assertSame( $this->indexable, $this->instance->build( $this->indexable ) ); } @@ -78,8 +78,7 @@ public function test_build_with_post_attachment() { $this->expect_maybe_build_author_indexable(); - // Saving is outside the scope of this test. - $this->expect_save_indexable_skip(); + $this->expect_save_indexable( $this->indexable ); $result = $this->instance->build( $this->indexable ); @@ -112,7 +111,7 @@ public function test_build_with_term_given() { ->once() ->with( $this->indexable ); - $this->expect_save_indexable_skip(); + $this->expect_save_indexable( $this->indexable ); $this->assertSame( $this->indexable, $this->instance->build( $this->indexable ) ); } @@ -171,7 +170,7 @@ static function ( $indexable ) { } ); - $this->expect_save_indexable_skip(); + $this->expect_save_indexable( $this->indexable ); $result = $this->instance->build( $this->indexable ); @@ -193,7 +192,7 @@ public function test_build_for_id_and_type_with_unknown_type_given() { $this->expect_deep_copy_indexable( $this->indexable ); - $this->expect_save_indexable_skip(); + $this->expect_save_indexable( $this->indexable ); $this->assertSame( $this->indexable, $this->instance->build( $this->indexable ) ); } @@ -259,12 +258,13 @@ static function ( $indexable ) { } ); - $this->expect_save_indexable_skip(); - - $result = $this->instance->build( false, $defaults ); - $expected_indexable = clone $empty_indexable; $expected_indexable->post_status = 'unindexed'; + $expected_indexable->version = 2; + + $this->expect_save_indexable( $expected_indexable ); + + $result = $this->instance->build( false, $defaults ); $this->assertEquals( $empty_indexable, $result ); } @@ -305,12 +305,12 @@ static function ( $indexable ) { } ); - $this->expect_save_indexable_skip(); - - $result = $this->instance->build( $this->indexable ); - $expected_indexable = clone $this->indexable; $expected_indexable->post_status = 'unindexed'; + + $this->expect_save_indexable( $expected_indexable ); + + $result = $this->instance->build( $this->indexable ); $this->assertEquals( $expected_indexable, $result ); } diff --git a/tests/Unit/Builders/Indexable_Builder/Maybe_Build_Author_Indexable_Test.php b/tests/Unit/Builders/Indexable_Builder/Maybe_Build_Author_Indexable_Test.php index ff33e9fb166..f43155a6e81 100644 --- a/tests/Unit/Builders/Indexable_Builder/Maybe_Build_Author_Indexable_Test.php +++ b/tests/Unit/Builders/Indexable_Builder/Maybe_Build_Author_Indexable_Test.php @@ -79,7 +79,7 @@ public function test_build_new_author_indexable() { ->with( $author_indexable->object_id, $author_indexable ) ->andReturn( $author_indexable ); - $this->expect_save_indexable_skip(); + $this->expect_save_indexable( $author_indexable ); $this->instance->exposed_maybe_build_author_indexable( $author_id ); } @@ -122,7 +122,7 @@ public function test_update_author_indexable() { ->with( $author_indexable->object_id, $author_indexable ) ->andReturn( $author_indexable ); - $this->expect_save_indexable_skip(); + $this->expect_save_indexable( $author_indexable ); $this->instance->exposed_maybe_build_author_indexable( $author_id ); } diff --git a/tests/Unit/Builders/Indexable_Builder/Save_Indexable_Test.php b/tests/Unit/Builders/Indexable_Builder/Save_Indexable_Test.php index 4cf7218ea91..a7ef8283afb 100644 --- a/tests/Unit/Builders/Indexable_Builder/Save_Indexable_Test.php +++ b/tests/Unit/Builders/Indexable_Builder/Save_Indexable_Test.php @@ -2,7 +2,6 @@ namespace Yoast\WP\SEO\Tests\Unit\Builders\Indexable_Builder; -use Brain\Monkey; use Mockery; use Yoast\WP\SEO\Tests\Unit\Doubles\Builders\Indexable_Builder_Double; @@ -50,47 +49,11 @@ protected function set_up() { public static function save_indexable_provider() { $before = Mockery::mock( Indexable::class ); return [ - 'Should index and save' => [ - 'indexable_before' => null, - 'should_index_indexables' => true, - 'wpseo_should_save_indexable' => true, - 'save_times' => 1, - 'action_times' => 0, + 'Index with no indexable before' => [ + 'indexable_before' => null, ], - 'Should not index and not save' => [ - 'indexable_before' => null, - 'should_index_indexables' => false, - 'wpseo_should_save_indexable' => false, - 'save_times' => 0, - 'action_times' => 0, - ], - 'Should not index but wpseo_should_save_indexable filter is true, should save' => [ - 'indexable_before' => null, - 'should_index_indexables' => false, - 'wpseo_should_save_indexable' => true, - 'save_times' => 1, - 'action_times' => 0, - ], - 'Should index but wpseo_should_save_indexable filter is false, should not save' => [ - 'indexable_before' => null, - 'should_index_indexables' => true, - 'wpseo_should_save_indexable' => false, - 'save_times' => 0, - 'action_times' => 0, - ], - 'Updating existing indexable' => [ - 'indexable_before' => $before, - 'should_index_indexables' => true, - 'wpseo_should_save_indexable' => true, - 'save_times' => 1, - 'action_times' => 1, - ], - 'Updating existing indexable when should_index_indexables is false' => [ - 'indexable_before' => $before, - 'should_index_indexables' => false, - 'wpseo_should_save_indexable' => true, - 'save_times' => 1, - 'action_times' => 1, + 'Index with indexable before' => [ + 'indexable_before' => $before, ], ]; } @@ -102,33 +65,16 @@ public static function save_indexable_provider() { * * @dataProvider save_indexable_provider * - * @param Indexable_Mock $indexable_before The indexable to expect. - * @param bool $should_index The return value of should_index_indexables method. - * @param bool $wpseo_should_save The return value for wpseo_should_save_indexable. - * @param int $save_times The times save method should be executed. - * @param int $action_times The times wpseo_save_indexable action should be executed. + * @param Indexable_Mock $indexable_before The indexable to expect. * @return void */ - public function test_save_indexable( $indexable_before, $should_index, $wpseo_should_save, $save_times, $action_times ) { + public function test_save_indexable( $indexable_before ) { $this->indexable_helper - ->expects( 'should_index_indexables' ) + ->expects( 'save_indexable' ) ->once() - ->withNoArgs() - ->andReturn( $should_index ); - - Monkey\Filters\expectApplied( 'wpseo_should_save_indexable' ) - ->once() - ->with( $should_index, $this->indexable ) - ->andReturn( $wpseo_should_save ); - - $this->indexable - ->expects( 'save' ) - ->times( $save_times ); - - Monkey\Actions\expectDone( 'wpseo_save_indexable' ) - ->times( $action_times ) - ->with( $this->indexable, $indexable_before ); + ->with( $this->indexable, $indexable_before ) + ->andReturn( $this->indexable ); $result = $this->instance->exposed_save_indexable( $this->indexable, $indexable_before ); diff --git a/tests/Unit/Doubles/Builders/Indexable_Builder_Double.php b/tests/Unit/Doubles/Builders/Indexable_Builder_Double.php index a000f30110b..f09c0299ee5 100644 --- a/tests/Unit/Doubles/Builders/Indexable_Builder_Double.php +++ b/tests/Unit/Doubles/Builders/Indexable_Builder_Double.php @@ -19,7 +19,7 @@ final class Indexable_Builder_Double extends Indexable_Builder { * @return Indexable The indexable. */ public function exposed_save_indexable( $indexable, $indexable_before = null ) { - return $this->save_indexable( $indexable, $indexable_before ); + return $this->indexable_helper->save_indexable( $indexable, $indexable_before ); } /** From f75eae091f8194798aa4675ca55d402c28240717 Mon Sep 17 00:00:00 2001 From: Leonidas Milosis Date: Tue, 30 Apr 2024 10:01:21 +0300 Subject: [PATCH 13/35] Don't create indexable via the hierarchy repository --- .../indexable-hierarchy-repository.php | 23 ++++++++++- .../Indexable_Hierarchy_Repository_Test.php | 38 +++++++++++++++++-- 2 files changed, 56 insertions(+), 5 deletions(-) diff --git a/src/repositories/indexable-hierarchy-repository.php b/src/repositories/indexable-hierarchy-repository.php index 4ff8beb07f4..64f815b5922 100644 --- a/src/repositories/indexable-hierarchy-repository.php +++ b/src/repositories/indexable-hierarchy-repository.php @@ -5,6 +5,7 @@ use Yoast\WP\Lib\Model; use Yoast\WP\Lib\ORM; use Yoast\WP\SEO\Builders\Indexable_Hierarchy_Builder; +use Yoast\WP\SEO\Helpers\Indexable_Helper; use Yoast\WP\SEO\Models\Indexable; /** @@ -19,6 +20,13 @@ class Indexable_Hierarchy_Repository { */ protected $builder; + /** + * Represents the indexable helper. + * + * @var Indexable_Helper + */ + protected $indexable_helper; + /** * Sets the hierarchy builder. * @@ -32,6 +40,19 @@ public function set_builder( Indexable_Hierarchy_Builder $builder ) { $this->builder = $builder; } + /** + * Sets the indexable helper. + * + * @required + * + * @param Indexable_Helper $indexable_helper The indexable helper. + * + * @return void + */ + public function set_helper( Indexable_Helper $indexable_helper ) { + $this->indexable_helper = $indexable_helper; + } + /** * Removes all ancestors for an indexable. * @@ -62,7 +83,7 @@ public function add_ancestor( $indexable_id, $ancestor_id, $depth ) { ] ); - return $hierarchy->save(); + return $this->indexable_helper->save_indexable( $hierarchy ); } /** diff --git a/tests/Unit/Repositories/Indexable_Hierarchy_Repository_Test.php b/tests/Unit/Repositories/Indexable_Hierarchy_Repository_Test.php index fd47876ecea..9f533c14096 100644 --- a/tests/Unit/Repositories/Indexable_Hierarchy_Repository_Test.php +++ b/tests/Unit/Repositories/Indexable_Hierarchy_Repository_Test.php @@ -7,6 +7,7 @@ use wpdb; use Yoast\WP\Lib\ORM; use Yoast\WP\SEO\Builders\Indexable_Hierarchy_Builder; +use Yoast\WP\SEO\Helpers\Indexable_Helper; use Yoast\WP\SEO\Repositories\Indexable_Hierarchy_Repository; use Yoast\WP\SEO\Tests\Unit\Doubles\Models\Indexable_Hierarchy_Mock; use Yoast\WP\SEO\Tests\Unit\Doubles\Models\Indexable_Mock; @@ -36,14 +37,22 @@ final class Indexable_Hierarchy_Repository_Test extends TestCase { */ protected $builder; + /** + * Represents the indexable helper. + * + * @var Mockery\Mock|Indexable_Helper + */ + protected $indexable_helper; + /** * {@inheritDoc} */ protected function set_up() { parent::set_up(); - $this->instance = Mockery::mock( Indexable_Hierarchy_Repository::class )->makePartial(); - $this->builder = Mockery::mock( Indexable_Hierarchy_Builder::class ); + $this->instance = Mockery::mock( Indexable_Hierarchy_Repository::class )->makePartial(); + $this->builder = Mockery::mock( Indexable_Hierarchy_Builder::class ); + $this->indexable_helper = Mockery::mock( Indexable_Helper::class ); } /** @@ -62,6 +71,22 @@ public function test_set_builder() { ); } + /** + * Tests setting the helper object. + * + * @covers ::set_helper + * + * @return void + */ + public function test_set_helper() { + $this->instance->set_helper( $this->indexable_helper ); + + $this->assertInstanceOf( + Indexable_Helper::class, + $this->getPropertyValue( $this->instance, 'indexable_helper' ) + ); + } + /** * Tests the retrieval of the find_ancestors when having already results. * @@ -201,6 +226,8 @@ public function test_clear_ancestors() { * @return void */ public function test_add_ancestor() { + $this->instance->set_helper( $this->indexable_helper ); + $hierarchy = Mockery::mock( Indexable_Hierarchy_Mock::class ); $hierarchy->indexable_id = 1; $hierarchy->ancestor_id = 2; @@ -209,8 +236,6 @@ public function test_add_ancestor() { Functions\expect( 'get_current_blog_id' )->once()->andReturn( 1 ); - $hierarchy->expects( 'save' )->once()->andReturn( true ); - $orm_object = Mockery::mock( ORM::class )->makePartial(); $orm_object @@ -226,6 +251,11 @@ public function test_add_ancestor() { ->andReturn( $hierarchy ); $this->instance->expects( 'query' )->andReturn( $orm_object ); + $this->indexable_helper + ->expects( 'save_indexable' ) + ->once() + ->andReturnTrue(); + $this->assertTrue( $this->instance->add_ancestor( 1, 2, 1 ) ); } From 590987618ba2ea5c07024bc274a3e95f79670a20 Mon Sep 17 00:00:00 2001 From: Leonidas Milosis Date: Tue, 30 Apr 2024 15:34:21 +0300 Subject: [PATCH 14/35] Don't create indexable via the link building actions --- .../abstract-link-indexing-action.php | 25 +++++++++++---- .../Abstract_Link_Indexing_Action_Test.php | 26 +++++++++++---- .../Post_Link_Indexing_Action_Test.php | 32 ++++++++++++++----- .../Term_Link_Indexing_Action_Test.php | 32 ++++++++++++++----- 4 files changed, 86 insertions(+), 29 deletions(-) diff --git a/src/actions/indexing/abstract-link-indexing-action.php b/src/actions/indexing/abstract-link-indexing-action.php index d25ca433a0f..0ae29336ce3 100644 --- a/src/actions/indexing/abstract-link-indexing-action.php +++ b/src/actions/indexing/abstract-link-indexing-action.php @@ -4,6 +4,7 @@ use wpdb; use Yoast\WP\SEO\Builders\Indexable_Link_Builder; +use Yoast\WP\SEO\Helpers\Indexable_Helper; use Yoast\WP\SEO\Models\SEO_Links; use Yoast\WP\SEO\Repositories\Indexable_Repository; @@ -19,6 +20,13 @@ abstract class Abstract_Link_Indexing_Action extends Abstract_Indexing_Action { */ protected $link_builder; + /** + * The indexable helper. + * + * @var Indexable_Helper + */ + protected $indexable_helper; + /** * The indexable repository. * @@ -36,18 +44,21 @@ abstract class Abstract_Link_Indexing_Action extends Abstract_Indexing_Action { /** * Indexable_Post_Indexing_Action constructor * - * @param Indexable_Link_Builder $link_builder The indexable link builder. - * @param Indexable_Repository $repository The indexable repository. - * @param wpdb $wpdb The WordPress database instance. + * @param Indexable_Link_Builder $link_builder The indexable link builder. + * @param Indexable_Helper $indexable_helper The indexable repository. + * @param Indexable_Repository $repository The indexable repository. + * @param wpdb $wpdb The WordPress database instance. */ public function __construct( Indexable_Link_Builder $link_builder, + Indexable_Helper $indexable_helper, Indexable_Repository $repository, wpdb $wpdb ) { - $this->link_builder = $link_builder; - $this->repository = $repository; - $this->wpdb = $wpdb; + $this->link_builder = $link_builder; + $this->indexable_helper = $indexable_helper; + $this->repository = $repository; + $this->wpdb = $wpdb; } /** @@ -63,7 +74,7 @@ public function index() { $indexable = $this->repository->find_by_id_and_type( $object->id, $object->type ); if ( $indexable ) { $this->link_builder->build( $indexable, $object->content ); - $indexable->save(); + $this->indexable_helper->save_indexable( $indexable ); $indexables[] = $indexable; } diff --git a/tests/Unit/Actions/Indexing/Abstract_Link_Indexing_Action_Test.php b/tests/Unit/Actions/Indexing/Abstract_Link_Indexing_Action_Test.php index 87a3cb1b6a1..dde441f14a9 100644 --- a/tests/Unit/Actions/Indexing/Abstract_Link_Indexing_Action_Test.php +++ b/tests/Unit/Actions/Indexing/Abstract_Link_Indexing_Action_Test.php @@ -7,6 +7,7 @@ use wpdb; use Yoast\WP\SEO\Actions\Indexing\Abstract_Link_Indexing_Action; use Yoast\WP\SEO\Builders\Indexable_Link_Builder; +use Yoast\WP\SEO\Helpers\Indexable_Helper; use Yoast\WP\SEO\Helpers\Taxonomy_Helper; use Yoast\WP\SEO\Repositories\Indexable_Repository; use Yoast\WP\SEO\Tests\Unit\TestCase; @@ -22,23 +23,30 @@ final class Abstract_Link_Indexing_Action_Test extends TestCase { /** - * The link builder. + * Represents the link builder. * - * @var Indexable_Link_Builder + * @var Mockery\MockInterface|Indexable_Link_Builder */ protected $link_builder; /** - * The post type helper. + * Represents the taxonomy helper. * - * @var Taxonomy_Helper + * @var Mockery\MockInterface|Taxonomy_Helper */ protected $taxonomy_helper; /** - * The indexable repository. + * Represents the indexable helper. * - * @var Indexable_Repository + * @var Mockery\MockInterface|Indexable_Helper + */ + protected $indexable_helper; + + /** + * Represents the indexable repository. + * + * @var Mockery\MockInterface|Indexable_Repository */ protected $repository; @@ -69,6 +77,7 @@ protected function set_up() { $wpdb = (object) [ 'prefix' => 'wp_' ]; $this->link_builder = Mockery::mock( Indexable_Link_Builder::class ); + $this->indexable_helper = Mockery::mock( Indexable_Helper::class ); $this->repository = Mockery::mock( Indexable_Repository::class ); $this->wpdb = Mockery::mock( wpdb::class ); $this->wpdb->term_taxonomy = 'wp_term_taxonomy'; @@ -77,6 +86,7 @@ protected function set_up() { Abstract_Link_Indexing_Action::class, [ $this->link_builder, + $this->indexable_helper, $this->repository, $this->wpdb, ] @@ -95,6 +105,10 @@ public function test_construct() { Indexable_Link_Builder::class, $this->getPropertyValue( $this->instance, 'link_builder' ) ); + $this->assertInstanceOf( + Indexable_Helper::class, + $this->getPropertyValue( $this->instance, 'indexable_helper' ) + ); $this->assertInstanceOf( Indexable_Repository::class, $this->getPropertyValue( $this->instance, 'repository' ) diff --git a/tests/Unit/Actions/Indexing/Post_Link_Indexing_Action_Test.php b/tests/Unit/Actions/Indexing/Post_Link_Indexing_Action_Test.php index f4d79fdd4d2..35a5210cd84 100644 --- a/tests/Unit/Actions/Indexing/Post_Link_Indexing_Action_Test.php +++ b/tests/Unit/Actions/Indexing/Post_Link_Indexing_Action_Test.php @@ -8,6 +8,7 @@ use wpdb; use Yoast\WP\SEO\Actions\Indexing\Post_Link_Indexing_Action; use Yoast\WP\SEO\Builders\Indexable_Link_Builder; +use Yoast\WP\SEO\Helpers\Indexable_Helper; use Yoast\WP\SEO\Helpers\Post_Type_Helper; use Yoast\WP\SEO\Repositories\Indexable_Repository; use Yoast\WP\SEO\Tests\Unit\Doubles\Models\Indexable_Mock; @@ -24,23 +25,30 @@ final class Post_Link_Indexing_Action_Test extends TestCase { /** - * The link builder. + * Represents the link builder. * - * @var Indexable_Link_Builder + * @var Mockery\MockInterface|Indexable_Link_Builder */ protected $link_builder; /** - * The post type helper. + * Represents the post type helper. * - * @var Post_Type_Helper + * @var Mockery\MockInterface|Post_Type_Helper */ protected $post_type_helper; /** - * The indexable repository. + * Represents the indexable helper. * - * @var Indexable_Repository + * @var Mockery\MockInterface|Indexable_Helper + */ + protected $indexable_helper; + + /** + * Represents the indexable repository. + * + * @var Mockery\MockInterface|Indexable_Repository */ protected $repository; @@ -72,12 +80,14 @@ protected function set_up() { $this->link_builder = Mockery::mock( Indexable_Link_Builder::class ); $this->post_type_helper = Mockery::mock( Post_Type_Helper::class ); + $this->indexable_helper = Mockery::mock( Indexable_Helper::class ); $this->repository = Mockery::mock( Indexable_Repository::class ); $this->wpdb = Mockery::mock( wpdb::class ); $this->wpdb->posts = 'wp_posts'; $this->instance = new Post_Link_Indexing_Action( $this->link_builder, + $this->indexable_helper, $this->repository, $this->wpdb ); @@ -274,7 +284,10 @@ public function test_index() { foreach ( $posts as $post ) { $indexable = Mockery::mock( Indexable_Mock::class ); $indexable->link_count = 10; - $indexable->expects( 'save' )->once(); + + $this->indexable_helper + ->expects( 'save_indexable' ) + ->once(); $this->link_builder->expects( 'build' )->with( $indexable, $post->post_content ); @@ -348,7 +361,10 @@ public function test_index_without_link_count() { $indexable = Mockery::mock( Indexable_Mock::class ); $indexable->link_count = null; - $indexable->expects( 'save' )->times( 3 ); + + $this->indexable_helper + ->expects( 'save_indexable' ) + ->times( 3 ); $this->repository->expects( 'find_by_id_and_type' )->once()->with( 1, 'post' )->andReturn( $indexable ); $this->repository->expects( 'find_by_id_and_type' )->once()->with( 3, 'post' )->andReturn( $indexable ); diff --git a/tests/Unit/Actions/Indexing/Term_Link_Indexing_Action_Test.php b/tests/Unit/Actions/Indexing/Term_Link_Indexing_Action_Test.php index 8dbbc361a14..a2985ea6786 100644 --- a/tests/Unit/Actions/Indexing/Term_Link_Indexing_Action_Test.php +++ b/tests/Unit/Actions/Indexing/Term_Link_Indexing_Action_Test.php @@ -8,6 +8,7 @@ use wpdb; use Yoast\WP\SEO\Actions\Indexing\Term_Link_Indexing_Action; use Yoast\WP\SEO\Builders\Indexable_Link_Builder; +use Yoast\WP\SEO\Helpers\Indexable_Helper; use Yoast\WP\SEO\Helpers\Taxonomy_Helper; use Yoast\WP\SEO\Repositories\Indexable_Repository; use Yoast\WP\SEO\Tests\Unit\Doubles\Models\Indexable_Mock; @@ -24,23 +25,30 @@ final class Term_Link_Indexing_Action_Test extends TestCase { /** - * The link builder. + * Represents the link builder. * - * @var Indexable_Link_Builder + * @var Mockery\MockInterface|Indexable_Link_Builder */ protected $link_builder; /** - * The post type helper. + * Represents the taxonomy helper. * - * @var Taxonomy_Helper + * @var Mockery\MockInterface|Taxonomy_Helper */ protected $taxonomy_helper; /** - * The indexable repository. + * Represents the indexable helper. * - * @var Indexable_Repository + * @var Mockery\MockInterface|Indexable_Helper + */ + protected $indexable_helper; + + /** + * Represents the indexable repository. + * + * @var Mockery\MockInterface|Indexable_Repository */ protected $repository; @@ -72,12 +80,14 @@ protected function set_up() { $this->link_builder = Mockery::mock( Indexable_Link_Builder::class ); $this->taxonomy_helper = Mockery::mock( Taxonomy_Helper::class ); + $this->indexable_helper = Mockery::mock( Indexable_Helper::class ); $this->repository = Mockery::mock( Indexable_Repository::class ); $this->wpdb = Mockery::mock( wpdb::class ); $this->wpdb->term_taxonomy = 'wp_term_taxonomy'; $this->instance = new Term_Link_Indexing_Action( $this->link_builder, + $this->indexable_helper, $this->repository, $this->wpdb ); @@ -320,7 +330,10 @@ public function test_index() { foreach ( $terms as $term ) { $indexable = Mockery::mock( Indexable_Mock::class ); $indexable->link_count = 10; - $indexable->expects( 'save' )->once(); + + $this->indexable_helper + ->expects( 'save_indexable' ) + ->once(); $this->link_builder->expects( 'build' )->with( $indexable, $term->description ); @@ -387,7 +400,10 @@ public function test_index_without_link_count() { $indexable = Mockery::mock( Indexable_Mock::class ); $indexable->link_count = null; - $indexable->expects( 'save' )->times( 3 ); + + $this->indexable_helper + ->expects( 'save_indexable' ) + ->times( 3 ); $this->repository->expects( 'find_by_id_and_type' )->once()->with( 1, 'term' )->andReturn( $indexable ); $this->repository->expects( 'find_by_id_and_type' )->once()->with( 3, 'term' )->andReturn( $indexable ); From 8e67d6be766ef9fc8221deb51c4204c250b7faff Mon Sep 17 00:00:00 2001 From: Leonidas Milosis Date: Fri, 10 May 2024 13:32:28 +0300 Subject: [PATCH 15/35] Don't create link indexables when a post or term is created/updated --- src/builders/indexable-link-builder.php | 28 ++++++++++++++++++- ...stract_Indexable_Link_Builder_TestCase.php | 12 +++++++- .../Indexable_Link_Builder/Build_Test.php | 9 ++++++ .../Create_Internal_Link_Test.php | 16 +++++++++++ .../Get_Permalink_Test.php | 3 +- ...Update_Incoming_Links_For_Related_Test.php | 3 +- 6 files changed, 67 insertions(+), 4 deletions(-) diff --git a/src/builders/indexable-link-builder.php b/src/builders/indexable-link-builder.php index a45944e5348..8894a39bb9f 100644 --- a/src/builders/indexable-link-builder.php +++ b/src/builders/indexable-link-builder.php @@ -6,6 +6,7 @@ use WP_HTML_Tag_Processor; use WPSEO_Image_Utils; use Yoast\WP\SEO\Helpers\Image_Helper; +use Yoast\WP\SEO\Helpers\Indexable_Helper; use Yoast\WP\SEO\Helpers\Options_Helper; use Yoast\WP\SEO\Helpers\Post_Helper; use Yoast\WP\SEO\Helpers\Url_Helper; @@ -40,6 +41,13 @@ class Indexable_Link_Builder { */ protected $image_helper; + /** + * The indexable helper. + * + * @var Indexable_Helper + */ + protected $indexable_helper; + /** * The post helper. * @@ -68,17 +76,20 @@ class Indexable_Link_Builder { * @param Url_Helper $url_helper The URL helper. * @param Post_Helper $post_helper The post helper. * @param Options_Helper $options_helper The options helper. + * @param Indexable_Helper $indexable_helper The indexable helper. */ public function __construct( SEO_Links_Repository $seo_links_repository, Url_Helper $url_helper, Post_Helper $post_helper, - Options_Helper $options_helper + Options_Helper $options_helper, + Indexable_Helper $indexable_helper ) { $this->seo_links_repository = $seo_links_repository; $this->url_helper = $url_helper; $this->post_helper = $post_helper; $this->options_helper = $options_helper; + $this->indexable_helper = $indexable_helper; } /** @@ -108,6 +119,21 @@ public function set_dependencies( * @return SEO_Links[] The created SEO links. */ public function build( $indexable, $content ) { + $intend_to_save = $this->indexable_helper->should_index_indexables(); + + /** + * Filter: 'wpseo_should_save_indexable' - Allow developers to enable / disable + * saving the indexable when the indexable is updated. Warning: overriding + * the intended action may cause problems when moving from a staging to a + * production environment because indexable permalinks may get set incorrectly. + * + * @param bool $intend_to_save True if YoastSEO intends to save the indexable. + * @param Indexable $indexable The indexable to be saved. + */ + if ( ! \apply_filters( 'wpseo_should_save_indexable', $intend_to_save, $indexable ) ) { + return []; + } + global $post; if ( $indexable->object_type === 'post' ) { $post_backup = $post; diff --git a/tests/Unit/Builders/Indexable_Link_Builder/Abstract_Indexable_Link_Builder_TestCase.php b/tests/Unit/Builders/Indexable_Link_Builder/Abstract_Indexable_Link_Builder_TestCase.php index bfcb349831e..a1258c61dbf 100644 --- a/tests/Unit/Builders/Indexable_Link_Builder/Abstract_Indexable_Link_Builder_TestCase.php +++ b/tests/Unit/Builders/Indexable_Link_Builder/Abstract_Indexable_Link_Builder_TestCase.php @@ -7,6 +7,7 @@ use Yoast\WP\Lib\ORM; use Yoast\WP\SEO\Builders\Indexable_Link_Builder; use Yoast\WP\SEO\Helpers\Image_Helper; +use Yoast\WP\SEO\Helpers\Indexable_Helper; use Yoast\WP\SEO\Helpers\Options_Helper; use Yoast\WP\SEO\Helpers\Post_Helper; use Yoast\WP\SEO\Helpers\Url_Helper; @@ -41,6 +42,13 @@ abstract class Abstract_Indexable_Link_Builder_TestCase extends TestCase { */ protected $image_helper; + /** + * The indexable helper. + * + * @var Mockery\MockInterface|Indexable_Helper + */ + protected $indexable_helper; + /** * The post helper. * @@ -90,12 +98,14 @@ protected function set_up() { $this->image_helper = Mockery::mock( Image_Helper::class ); $this->post_helper = Mockery::mock( Post_Helper::class ); $this->options_helper = Mockery::mock( Options_Helper::class ); + $this->indexable_helper = Mockery::mock( Indexable_Helper::class ); $this->instance = new Indexable_Link_Builder( $this->seo_links_repository, $this->url_helper, $this->post_helper, - $this->options_helper + $this->options_helper, + $this->indexable_helper ); $this->instance->set_dependencies( $this->indexable_repository, $this->image_helper ); diff --git a/tests/Unit/Builders/Indexable_Link_Builder/Build_Test.php b/tests/Unit/Builders/Indexable_Link_Builder/Build_Test.php index 456239f1cad..43ab7da907e 100644 --- a/tests/Unit/Builders/Indexable_Link_Builder/Build_Test.php +++ b/tests/Unit/Builders/Indexable_Link_Builder/Build_Test.php @@ -114,6 +114,8 @@ public function test_build( $content, $link_type, $is_image, $ignore_content_sca $indexable->object_type = 'post'; $indexable->permalink = 'https://site.com/page'; + $this->indexable_helper->expects( 'should_index_indexables' )->once()->andReturn( true ); + Functions\expect( 'apply_filters' )->once()->with( 'wpseo_should_save_indexable', true, $indexable )->andReturn( true ); $this->post_helper->expects( 'get_post' )->once()->with( 2 )->andReturn( 'post' ); Functions\expect( 'setup_postdata' )->once()->with( 'post' ); Functions\expect( 'apply_filters' )->once()->with( 'the_content', $content )->andReturn( $content ); @@ -258,6 +260,8 @@ public function test_build_target_indexable_does_not_exist() { $target_indexable->language = 'nl'; $target_indexable->region = 'NL'; + $this->indexable_helper->expects( 'should_index_indexables' )->once()->andReturn( true ); + Filters\expectApplied( 'wpseo_should_save_indexable' )->with( true, $indexable )->andReturnFirstArg(); $this->post_helper->expects( 'get_post' )->once()->with( 2 )->andReturn( 'post' ); Functions\expect( 'setup_postdata' )->once()->with( 'post' ); Filters\expectApplied( 'the_content' )->with( $content )->andReturnFirstArg(); @@ -372,6 +376,9 @@ public function test_build_no_links() { $indexable->object_type = 'page'; $indexable->permalink = 'https://site.com/page'; + $this->indexable_helper->expects( 'should_index_indexables' )->once()->andReturn( true ); + Filters\expectApplied( 'wpseo_should_save_indexable' )->with( true, $indexable )->andReturnFirstArg(); + $this->seo_links_repository ->expects( 'find_all_by_indexable_id' ) ->with( $indexable->id ) @@ -400,6 +407,8 @@ public function test_build_ignore_content_scan( $input_content, $output_result ) $indexable->object_id = 2; $indexable->object_type = 'page'; $indexable->permalink = 'https://site.com/page'; + + $this->indexable_helper->expects( 'should_index_indexables' )->once()->andReturn( true ); Functions\expect( 'apply_filters' )->andReturn( true ); $this->seo_links_repository diff --git a/tests/Unit/Builders/Indexable_Link_Builder/Create_Internal_Link_Test.php b/tests/Unit/Builders/Indexable_Link_Builder/Create_Internal_Link_Test.php index 27c7b8c163a..cda92655ad6 100644 --- a/tests/Unit/Builders/Indexable_Link_Builder/Create_Internal_Link_Test.php +++ b/tests/Unit/Builders/Indexable_Link_Builder/Create_Internal_Link_Test.php @@ -2,6 +2,7 @@ namespace Yoast\WP\SEO\Tests\Unit\Builders\Indexable_Link_Builder; +use Brain\Monkey\Filters; use Brain\Monkey\Functions; use Mockery; use Yoast\WP\SEO\Models\SEO_Links; @@ -43,6 +44,9 @@ public function test_build_create_internal_link() { $model = new SEO_Links_Mock(); $model->type = SEO_Links::TYPE_INTERNAL_IMAGE; + $this->indexable_helper->expects( 'should_index_indexables' )->once()->andReturn( true ); + Filters\expectApplied( 'wpseo_should_save_indexable' )->with( true, $indexable )->andReturnFirstArg(); + Functions\stubs( [ // Executed in build->create_links->create_internal_link. @@ -119,6 +123,9 @@ public function test_build_create_internal_link_disable_attachment_true_file_doe $model->type = SEO_Links::TYPE_INTERNAL_IMAGE; $model->target_post_id = 2; + $this->indexable_helper->expects( 'should_index_indexables' )->once()->andReturn( true ); + Filters\expectApplied( 'wpseo_should_save_indexable' )->with( true, $indexable )->andReturnFirstArg(); + // Executed in build->create_links->create_internal_link. Functions\stubs( [ @@ -198,6 +205,9 @@ public function test_build_create_internal_link_disable_attachment_true_file_exi $model->height = null; $model->width = null; + $this->indexable_helper->expects( 'should_index_indexables' )->once()->andReturn( true ); + Filters\expectApplied( 'wpseo_should_save_indexable' )->with( true, $indexable )->andReturnFirstArg(); + // Executed in build->create_links->create_internal_link. Functions\stubs( [ @@ -261,6 +271,9 @@ public function test_build_create_internal_link_disable_attachment_true_file_not $model->type = SEO_Links::TYPE_INTERNAL_IMAGE; $model->target_post_id = 3; + $this->indexable_helper->expects( 'should_index_indexables' )->once()->andReturn( true ); + Filters\expectApplied( 'wpseo_should_save_indexable' )->with( true, $indexable )->andReturnFirstArg(); + Functions\stubs( [ // Executed in build->create_links->create_internal_link. @@ -339,6 +352,9 @@ public function test_build_create_internal_link_disable_attachment_true_get_atta $model->type = SEO_Links::TYPE_INTERNAL_IMAGE; $model->target_post_id = 2; + $this->indexable_helper->expects( 'should_index_indexables' )->once()->andReturn( true ); + Filters\expectApplied( 'wpseo_should_save_indexable' )->with( true, $indexable )->andReturnFirstArg(); + Functions\stubs( [ // Executed in build->create_links. diff --git a/tests/Unit/Builders/Indexable_Link_Builder/Get_Permalink_Test.php b/tests/Unit/Builders/Indexable_Link_Builder/Get_Permalink_Test.php index 4d62f218062..7495510984d 100644 --- a/tests/Unit/Builders/Indexable_Link_Builder/Get_Permalink_Test.php +++ b/tests/Unit/Builders/Indexable_Link_Builder/Get_Permalink_Test.php @@ -25,7 +25,8 @@ protected function set_up() { $this->seo_links_repository, $this->url_helper, $this->post_helper, - $this->options_helper + $this->options_helper, + $this->indexable_helper ); $this->instance->set_dependencies( $this->indexable_repository, $this->image_helper ); diff --git a/tests/Unit/Builders/Indexable_Link_Builder/Update_Incoming_Links_For_Related_Test.php b/tests/Unit/Builders/Indexable_Link_Builder/Update_Incoming_Links_For_Related_Test.php index 952b7aff76c..92a04c7b4c3 100644 --- a/tests/Unit/Builders/Indexable_Link_Builder/Update_Incoming_Links_For_Related_Test.php +++ b/tests/Unit/Builders/Indexable_Link_Builder/Update_Incoming_Links_For_Related_Test.php @@ -27,7 +27,8 @@ protected function set_up() { $this->seo_links_repository, $this->url_helper, $this->post_helper, - $this->options_helper + $this->options_helper, + $this->indexable_helper ); $this->instance->set_dependencies( $this->indexable_repository, $this->image_helper ); From f307d2cdf13b70b0b49f6c323fa56640aaa3ad29 Mon Sep 17 00:00:00 2001 From: Leonidas Milosis Date: Tue, 21 May 2024 11:56:05 +0300 Subject: [PATCH 16/35] Create a wrapper helper function to use when assessing whether an indexable should be created --- src/builders/indexable-link-builder.php | 13 +------ src/helpers/indexable-helper.php | 37 ++++++++++++------- .../Indexable_Link_Builder/Build_Test.php | 11 ++---- .../Create_Internal_Link_Test.php | 16 +++----- 4 files changed, 33 insertions(+), 44 deletions(-) diff --git a/src/builders/indexable-link-builder.php b/src/builders/indexable-link-builder.php index 8894a39bb9f..ca703c2cda7 100644 --- a/src/builders/indexable-link-builder.php +++ b/src/builders/indexable-link-builder.php @@ -119,18 +119,7 @@ public function set_dependencies( * @return SEO_Links[] The created SEO links. */ public function build( $indexable, $content ) { - $intend_to_save = $this->indexable_helper->should_index_indexables(); - - /** - * Filter: 'wpseo_should_save_indexable' - Allow developers to enable / disable - * saving the indexable when the indexable is updated. Warning: overriding - * the intended action may cause problems when moving from a staging to a - * production environment because indexable permalinks may get set incorrectly. - * - * @param bool $intend_to_save True if YoastSEO intends to save the indexable. - * @param Indexable $indexable The indexable to be saved. - */ - if ( ! \apply_filters( 'wpseo_should_save_indexable', $intend_to_save, $indexable ) ) { + if ( ! $this->indexable_helper->should_index_indexable( $indexable ) ) { return []; } diff --git a/src/helpers/indexable-helper.php b/src/helpers/indexable-helper.php index f5cd6ab2b7c..cd2f6bb0aab 100644 --- a/src/helpers/indexable-helper.php +++ b/src/helpers/indexable-helper.php @@ -178,6 +178,28 @@ public function reset_permalink_indexables( $type = null, $subtype = null, $reas } } + /** + * Determines whether indexing the specific indexable is appropriate at this time. + * + * @param Indexable $indexable The indexable. + * + * @return bool Whether indexing the specific indexable is appropriate at this time. + */ + public function should_index_indexable( $indexable ) { + $intend_to_save = $this->should_index_indexables(); + + /** + * Filter: 'wpseo_should_save_indexable' - Allow developers to enable / disable + * saving the indexable when the indexable is updated. Warning: overriding + * the intended action may cause problems when moving from a staging to a + * production environment because indexable permalinks may get set incorrectly. + * + * @param bool $intend_to_save True if YoastSEO intends to save the indexable. + * @param Indexable $indexable The indexable to be saved. + */ + return \apply_filters( 'wpseo_should_save_indexable', $intend_to_save, $indexable ); + } + /** * Determines whether indexing indexables is appropriate at this time. * @@ -274,20 +296,7 @@ public function check_if_default_field( $indexable, $field ) { * @return bool True if default value. */ public function save_indexable( $indexable, $indexable_before = null ) { - $intend_to_save = $this->should_index_indexables(); - - /** - * Filter: 'wpseo_should_save_indexable' - Allow developers to enable / disable - * saving the indexable when the indexable is updated. Warning: overriding - * the intended action may cause problems when moving from a staging to a - * production environment because indexable permalinks may get set incorrectly. - * - * @param bool $intend_to_save True if YoastSEO intends to save the indexable. - * @param Indexable $indexable The indexable to be saved. - */ - $intend_to_save = \apply_filters( 'wpseo_should_save_indexable', $intend_to_save, $indexable ); - - if ( ! $intend_to_save ) { + if ( ! $this->should_index_indexable( $indexable ) ) { return $indexable; } diff --git a/tests/Unit/Builders/Indexable_Link_Builder/Build_Test.php b/tests/Unit/Builders/Indexable_Link_Builder/Build_Test.php index 43ab7da907e..23de91e49f6 100644 --- a/tests/Unit/Builders/Indexable_Link_Builder/Build_Test.php +++ b/tests/Unit/Builders/Indexable_Link_Builder/Build_Test.php @@ -114,8 +114,7 @@ public function test_build( $content, $link_type, $is_image, $ignore_content_sca $indexable->object_type = 'post'; $indexable->permalink = 'https://site.com/page'; - $this->indexable_helper->expects( 'should_index_indexables' )->once()->andReturn( true ); - Functions\expect( 'apply_filters' )->once()->with( 'wpseo_should_save_indexable', true, $indexable )->andReturn( true ); + $this->indexable_helper->expects( 'should_index_indexable' )->once()->andReturn( true ); $this->post_helper->expects( 'get_post' )->once()->with( 2 )->andReturn( 'post' ); Functions\expect( 'setup_postdata' )->once()->with( 'post' ); Functions\expect( 'apply_filters' )->once()->with( 'the_content', $content )->andReturn( $content ); @@ -260,8 +259,7 @@ public function test_build_target_indexable_does_not_exist() { $target_indexable->language = 'nl'; $target_indexable->region = 'NL'; - $this->indexable_helper->expects( 'should_index_indexables' )->once()->andReturn( true ); - Filters\expectApplied( 'wpseo_should_save_indexable' )->with( true, $indexable )->andReturnFirstArg(); + $this->indexable_helper->expects( 'should_index_indexable' )->once()->andReturn( true ); $this->post_helper->expects( 'get_post' )->once()->with( 2 )->andReturn( 'post' ); Functions\expect( 'setup_postdata' )->once()->with( 'post' ); Filters\expectApplied( 'the_content' )->with( $content )->andReturnFirstArg(); @@ -376,8 +374,7 @@ public function test_build_no_links() { $indexable->object_type = 'page'; $indexable->permalink = 'https://site.com/page'; - $this->indexable_helper->expects( 'should_index_indexables' )->once()->andReturn( true ); - Filters\expectApplied( 'wpseo_should_save_indexable' )->with( true, $indexable )->andReturnFirstArg(); + $this->indexable_helper->expects( 'should_index_indexable' )->once()->andReturn( true ); $this->seo_links_repository ->expects( 'find_all_by_indexable_id' ) @@ -408,7 +405,7 @@ public function test_build_ignore_content_scan( $input_content, $output_result ) $indexable->object_type = 'page'; $indexable->permalink = 'https://site.com/page'; - $this->indexable_helper->expects( 'should_index_indexables' )->once()->andReturn( true ); + $this->indexable_helper->expects( 'should_index_indexable' )->once()->andReturn( true ); Functions\expect( 'apply_filters' )->andReturn( true ); $this->seo_links_repository diff --git a/tests/Unit/Builders/Indexable_Link_Builder/Create_Internal_Link_Test.php b/tests/Unit/Builders/Indexable_Link_Builder/Create_Internal_Link_Test.php index cda92655ad6..55dc508e7ba 100644 --- a/tests/Unit/Builders/Indexable_Link_Builder/Create_Internal_Link_Test.php +++ b/tests/Unit/Builders/Indexable_Link_Builder/Create_Internal_Link_Test.php @@ -2,7 +2,6 @@ namespace Yoast\WP\SEO\Tests\Unit\Builders\Indexable_Link_Builder; -use Brain\Monkey\Filters; use Brain\Monkey\Functions; use Mockery; use Yoast\WP\SEO\Models\SEO_Links; @@ -44,8 +43,7 @@ public function test_build_create_internal_link() { $model = new SEO_Links_Mock(); $model->type = SEO_Links::TYPE_INTERNAL_IMAGE; - $this->indexable_helper->expects( 'should_index_indexables' )->once()->andReturn( true ); - Filters\expectApplied( 'wpseo_should_save_indexable' )->with( true, $indexable )->andReturnFirstArg(); + $this->indexable_helper->expects( 'should_index_indexable' )->once()->andReturn( true ); Functions\stubs( [ @@ -123,8 +121,7 @@ public function test_build_create_internal_link_disable_attachment_true_file_doe $model->type = SEO_Links::TYPE_INTERNAL_IMAGE; $model->target_post_id = 2; - $this->indexable_helper->expects( 'should_index_indexables' )->once()->andReturn( true ); - Filters\expectApplied( 'wpseo_should_save_indexable' )->with( true, $indexable )->andReturnFirstArg(); + $this->indexable_helper->expects( 'should_index_indexable' )->once()->andReturn( true ); // Executed in build->create_links->create_internal_link. Functions\stubs( @@ -205,8 +202,7 @@ public function test_build_create_internal_link_disable_attachment_true_file_exi $model->height = null; $model->width = null; - $this->indexable_helper->expects( 'should_index_indexables' )->once()->andReturn( true ); - Filters\expectApplied( 'wpseo_should_save_indexable' )->with( true, $indexable )->andReturnFirstArg(); + $this->indexable_helper->expects( 'should_index_indexable' )->once()->andReturn( true ); // Executed in build->create_links->create_internal_link. Functions\stubs( @@ -271,8 +267,7 @@ public function test_build_create_internal_link_disable_attachment_true_file_not $model->type = SEO_Links::TYPE_INTERNAL_IMAGE; $model->target_post_id = 3; - $this->indexable_helper->expects( 'should_index_indexables' )->once()->andReturn( true ); - Filters\expectApplied( 'wpseo_should_save_indexable' )->with( true, $indexable )->andReturnFirstArg(); + $this->indexable_helper->expects( 'should_index_indexable' )->once()->andReturn( true ); Functions\stubs( [ @@ -352,8 +347,7 @@ public function test_build_create_internal_link_disable_attachment_true_get_atta $model->type = SEO_Links::TYPE_INTERNAL_IMAGE; $model->target_post_id = 2; - $this->indexable_helper->expects( 'should_index_indexables' )->once()->andReturn( true ); - Filters\expectApplied( 'wpseo_should_save_indexable' )->with( true, $indexable )->andReturnFirstArg(); + $this->indexable_helper->expects( 'should_index_indexable' )->once()->andReturn( true ); Functions\stubs( [ From 2d05d1239b396584aea59f62e4e452d424cb5365 Mon Sep 17 00:00:00 2001 From: Leonidas Milosis Date: Wed, 22 May 2024 12:58:43 +0300 Subject: [PATCH 17/35] Add unit tests --- .../Post_Link_Indexing_Action_Test.php | 2 + .../Term_Link_Indexing_Action_Test.php | 2 + tests/Unit/Helpers/Indexable_Helper_Test.php | 181 ++++++++++++++++++ .../Indexable_Ancestor_Watcher_Test.php | 3 + .../Indexable_Author_Watcher_Test.php | 2 + ...dexable_Post_Type_Archive_Watcher_Test.php | 12 +- .../Watchers/Indexable_Post_Watcher_Test.php | 5 + .../Watchers/Indexable_Term_Watcher_Test.php | 2 + .../Indexable_Hierarchy_Repository_Test.php | 1 + 9 files changed, 209 insertions(+), 1 deletion(-) diff --git a/tests/Unit/Actions/Indexing/Post_Link_Indexing_Action_Test.php b/tests/Unit/Actions/Indexing/Post_Link_Indexing_Action_Test.php index 35a5210cd84..b11d58b0e94 100644 --- a/tests/Unit/Actions/Indexing/Post_Link_Indexing_Action_Test.php +++ b/tests/Unit/Actions/Indexing/Post_Link_Indexing_Action_Test.php @@ -287,6 +287,7 @@ public function test_index() { $this->indexable_helper ->expects( 'save_indexable' ) + ->with( $indexable ) ->once(); $this->link_builder->expects( 'build' )->with( $indexable, $post->post_content ); @@ -364,6 +365,7 @@ public function test_index_without_link_count() { $this->indexable_helper ->expects( 'save_indexable' ) + ->with( $indexable ) ->times( 3 ); $this->repository->expects( 'find_by_id_and_type' )->once()->with( 1, 'post' )->andReturn( $indexable ); diff --git a/tests/Unit/Actions/Indexing/Term_Link_Indexing_Action_Test.php b/tests/Unit/Actions/Indexing/Term_Link_Indexing_Action_Test.php index a2985ea6786..be632fb931d 100644 --- a/tests/Unit/Actions/Indexing/Term_Link_Indexing_Action_Test.php +++ b/tests/Unit/Actions/Indexing/Term_Link_Indexing_Action_Test.php @@ -333,6 +333,7 @@ public function test_index() { $this->indexable_helper ->expects( 'save_indexable' ) + ->with( $indexable ) ->once(); $this->link_builder->expects( 'build' )->with( $indexable, $term->description ); @@ -403,6 +404,7 @@ public function test_index_without_link_count() { $this->indexable_helper ->expects( 'save_indexable' ) + ->with( $indexable ) ->times( 3 ); $this->repository->expects( 'find_by_id_and_type' )->once()->with( 1, 'term' )->andReturn( $indexable ); diff --git a/tests/Unit/Helpers/Indexable_Helper_Test.php b/tests/Unit/Helpers/Indexable_Helper_Test.php index 41b9c44498f..37eaa4655f4 100644 --- a/tests/Unit/Helpers/Indexable_Helper_Test.php +++ b/tests/Unit/Helpers/Indexable_Helper_Test.php @@ -337,4 +337,185 @@ public static function provider_check_if_default_field() { [ 'schema_article_type', 'irrelevant', false ], // Checking for fields that don't have an explicit default will always return false. ]; } + + /** + * Tests should_index_indexable method. + * + * @dataProvider provider_should_index_indexable + * @covers ::should_index_indexable + * @covers ::should_index_indexables + * + * @param bool $is_production_mode Whether is production mode. + * @param bool $should_index_indexables Whether indexables should be created. + * @param bool $should_save_indexable What the "wpseo_should_save_indexable" filter returns. + * @param bool $expected_result If indexable should be indexed. + * + * @return void + */ + public function test_should_index_indexable( $is_production_mode, $should_index_indexables, $should_save_indexable, $expected_result ) { + $indexable = Mockery::mock( Indexable_Mock::class ); + + $this->environment_helper + ->expects( 'is_production_mode' ) + ->andReturn( $is_production_mode ); + + // In order to test not having an overriding "Yoast\WP\SEO\should_index_indexables" filter. + if ( $should_index_indexables === null ) { + $should_index_indexables = $is_production_mode; + } + Monkey\Filters\expectApplied( 'Yoast\WP\SEO\should_index_indexables' ) + ->once() + ->with( $is_production_mode ) + ->andReturn( $should_index_indexables ); + + // In order to test not having an overriding "wpseo_should_save_indexable" filter. + if ( $should_save_indexable === null ) { + $should_save_indexable = $should_index_indexables; + } + Monkey\Filters\expectApplied( 'wpseo_should_save_indexable' ) + ->once() + ->with( $should_index_indexables, $indexable ) + ->andReturn( $should_save_indexable ); + + $result = $this->instance->should_index_indexable( $indexable ); + $this->assertSame( $expected_result, $result ); + } + + /** + * Data provider for test_should_index_indexable. + * + * @return array + */ + public static function provider_should_index_indexable() { + yield 'Not production and no overriding filters' => [ + 'is_production_mode' => false, + 'should_index_indexables' => null, + 'should_save_indexable' => null, + 'expected_result' => false, + ]; + yield 'Not production, but with an overriding filter allowing general indexing' => [ + 'is_production_mode' => false, + 'should_index_indexables' => true, + 'should_save_indexable' => null, + 'expected_result' => true, + ]; + yield 'Not production, but with an overriding filter disallowing general indexing' => [ + 'is_production_mode' => false, + 'should_index_indexables' => false, + 'should_save_indexable' => null, + 'expected_result' => false, + ]; + yield 'Not production, but with an overriding filter allowing indexing of that specific indexable' => [ + 'is_production_mode' => false, + 'should_index_indexables' => null, + 'should_save_indexable' => true, + 'expected_result' => true, + ]; + yield 'Not production, but with an overriding filter disallowing general indexing, but with an overriding filter allowing indexing of that specific indexable' => [ + 'is_production_mode' => false, + 'should_index_indexables' => false, + 'should_save_indexable' => true, + 'expected_result' => true, + ]; + yield 'Production and no overriding filters' => [ + 'is_production_mode' => true, + 'should_index_indexables' => null, + 'should_save_indexable' => null, + 'expected_result' => true, + ]; + yield 'Production, but with an overriding filter disallowing general indexing' => [ + 'is_production_mode' => true, + 'should_index_indexables' => false, + 'should_save_indexable' => null, + 'expected_result' => false, + ]; + yield 'Production, with an overriding filter disallowing general indexing, but with an overriding filter allowing indexing of that specific indexable' => [ + 'is_production_mode' => true, + 'should_index_indexables' => false, + 'should_save_indexable' => true, + 'expected_result' => true, + ]; + yield 'Production, with an overriding filter allowing general indexing, but with an overriding filter disallowing indexing of that specific indexable' => [ + 'is_production_mode' => true, + 'should_index_indexables' => true, + 'should_save_indexable' => false, + 'expected_result' => false, + ]; + } + + /** + * Tests save_indexable method. + * + * @dataProvider provider_save_indexable + * @covers ::save_indexable + * @covers ::should_index_indexable + * @covers ::should_index_indexables + * + * @param bool $should_save Whether the indexable should be saved. + * @param Indexable $indexable_before The indexable how it was before all this. + * @param int $save_times Times that indexable will be saved. + * @param int $action_times Times that the "wpseo_save_indexable" action will be run. + * + * @return void + */ + public function test_save_indexable( $should_save, $indexable_before, $save_times, $action_times ) { + $indexable = Mockery::mock( Indexable_Mock::class ); + + $this->environment_helper + ->expects( 'is_production_mode' ) + ->andReturn( true ); + + Monkey\Filters\expectApplied( 'Yoast\WP\SEO\should_index_indexables' ) + ->once() + ->andReturn( true ); + + Monkey\Filters\expectApplied( 'wpseo_should_save_indexable' ) + ->once( $action_times ) + ->andReturn( $should_save ); + + $indexable + ->expects( 'save' ) + ->times( $save_times ); + + Monkey\Actions\expectDone( 'wpseo_save_indexable' ) + ->times( $action_times ) + ->with( $indexable, $indexable_before ); + + $result = $this->instance->save_indexable( $indexable, $indexable_before ); + $this->assertSame( $result, $indexable ); + } + + /** + * Data provider for test_save_indexable. + * + * @return array + */ + public static function provider_save_indexable() { + $indexable_before = Mockery::mock( Indexable_Mock::class ); + + yield 'Indexable should be saved and there is no past iteration of the indexable passed' => [ + 'should_save' => true, + 'indexable_before' => null, + 'save_times' => 1, + 'action_times' => 0, + ]; + yield 'Indexable should not be saved and there is no past iteration of the indexable passed' => [ + 'should_save' => false, + 'indexable_before' => null, + 'save_times' => 0, + 'action_times' => 0, + ]; + yield 'Indexable should be saved and there is a past iteration of the indexable passed' => [ + 'should_save' => true, + 'indexable_before' => $indexable_before, + 'save_times' => 1, + 'action_times' => 1, + ]; + yield 'Indexable should not be saved and there is a past iteration of the indexable passed' => [ + 'should_save' => false, + 'indexable_before' => $indexable_before, + 'save_times' => 0, + 'action_times' => 0, + ]; + } } diff --git a/tests/Unit/Integrations/Watchers/Indexable_Ancestor_Watcher_Test.php b/tests/Unit/Integrations/Watchers/Indexable_Ancestor_Watcher_Test.php index 48eaa204dc8..30065b03a78 100644 --- a/tests/Unit/Integrations/Watchers/Indexable_Ancestor_Watcher_Test.php +++ b/tests/Unit/Integrations/Watchers/Indexable_Ancestor_Watcher_Test.php @@ -225,6 +225,7 @@ public function test_reset_children() { $this->indexable_helper ->expects( 'save_indexable' ) + ->with( $child_indexable ) ->once(); $this->assertTrue( $this->instance->reset_children( $indexable, $indexable_before ) ); @@ -312,6 +313,7 @@ public function test_reset_children_for_term() { $this->indexable_helper ->expects( 'save_indexable' ) + ->with( $child_indexable ) ->once(); $this->assertTrue( $this->instance->reset_children( $indexable, $indexable_before ) ); @@ -455,6 +457,7 @@ private function set_expectations_for_update_hierarchy_and_permalink( ...$indexa $this->indexable_helper ->expects( 'save_indexable' ) + ->with( $indexable ) ->once(); } } diff --git a/tests/Unit/Integrations/Watchers/Indexable_Author_Watcher_Test.php b/tests/Unit/Integrations/Watchers/Indexable_Author_Watcher_Test.php index b7d9a610b82..a831707de35 100644 --- a/tests/Unit/Integrations/Watchers/Indexable_Author_Watcher_Test.php +++ b/tests/Unit/Integrations/Watchers/Indexable_Author_Watcher_Test.php @@ -172,6 +172,7 @@ public function test_build_indexable() { $this->indexable_helper ->expects( 'save_indexable' ) + ->with( $indexable_mock ) ->once(); $this->instance->build_indexable( $id ); @@ -208,6 +209,7 @@ public function test_build_indexable_not_found() { $this->indexable_helper ->expects( 'save_indexable' ) + ->with( $indexable_mock ) ->once(); $this->instance->build_indexable( $id ); diff --git a/tests/Unit/Integrations/Watchers/Indexable_Post_Type_Archive_Watcher_Test.php b/tests/Unit/Integrations/Watchers/Indexable_Post_Type_Archive_Watcher_Test.php index 4a421416228..b1a65c97312 100644 --- a/tests/Unit/Integrations/Watchers/Indexable_Post_Type_Archive_Watcher_Test.php +++ b/tests/Unit/Integrations/Watchers/Indexable_Post_Type_Archive_Watcher_Test.php @@ -129,6 +129,7 @@ public function test_check_option_first_time_save() { $this->indexable_helper ->expects( 'save_indexable' ) + ->with( $indexable_mock ) ->once(); $this->assertTrue( $this->instance->check_option( false, [ 'title-ptarchive-my-post-type' => 'baz' ] ) ); @@ -177,6 +178,7 @@ public function test_update_wpseo_titles_value() { $this->indexable_helper ->expects( 'save_indexable' ) + ->with( $indexable_mock ) ->once(); $this->assertTrue( $this->instance->check_option( [ 'title-ptarchive-my-post-type' => 'bar' ], [ 'title-ptarchive-my-post-type' => 'baz' ] ) ); @@ -213,6 +215,7 @@ public function test_update_wpseo_titles_value_new() { $this->indexable_helper ->expects( 'save_indexable' ) + ->with( $indexable_mock ) ->once(); $this->assertTrue( $this->instance->check_option( [], [ 'title-ptarchive-my-post-type' => 'baz' ] ) ); @@ -266,7 +269,13 @@ public function test_update_wpseo_titles_value_switched() { $this->indexable_helper ->expects( 'save_indexable' ) - ->times( 2 ); + ->with( $indexable_mock ) + ->once(); + + $this->indexable_helper + ->expects( 'save_indexable' ) + ->with( $other_indexable_mock ) + ->once(); $this->assertTrue( $this->instance->check_option( [ 'title-ptarchive-my-post-type' => 'baz' ], [ 'title-ptarchive-other-post-type' => 'baz' ] ) ); } @@ -329,6 +338,7 @@ public function test_build_indexable_without_indexable() { $this->indexable_helper ->expects( 'save_indexable' ) + ->with( $indexable_mock ) ->once(); $this->instance->build_indexable( 'my-post-type' ); diff --git a/tests/Unit/Integrations/Watchers/Indexable_Post_Watcher_Test.php b/tests/Unit/Integrations/Watchers/Indexable_Post_Watcher_Test.php index ea5d8b69b07..29e3c11fe76 100644 --- a/tests/Unit/Integrations/Watchers/Indexable_Post_Watcher_Test.php +++ b/tests/Unit/Integrations/Watchers/Indexable_Post_Watcher_Test.php @@ -260,6 +260,7 @@ public function test_build_indexable() { $this->indexable_helper ->expects( 'save_indexable' ) + ->with( $indexable_mock ) ->once(); $this->instance @@ -360,6 +361,7 @@ public function test_build_indexable_does_not_exist() { $this->indexable_helper ->expects( 'save_indexable' ) + ->with( $indexable_mock ) ->once(); $this->instance @@ -420,6 +422,7 @@ public function test_update_has_public_posts_with_post() { $this->indexable_helper ->expects( 'save_indexable' ) + ->with( $author_indexable ) ->once(); $this->post->expects( 'update_has_public_posts_on_attachments' )->once()->with( 33, null )->andReturnTrue(); @@ -516,6 +519,7 @@ public function test_reschedule_cleanup_when_author_does_not_have_posts() { $this->indexable_helper ->expects( 'save_indexable' ) + ->with( $author_indexable ) ->once(); $this->post->expects( 'update_has_public_posts_on_attachments' ) @@ -559,6 +563,7 @@ public function test_update_relations() { $this->indexable_helper ->expects( 'save_indexable' ) + ->with( $indexable ) ->once(); $this->instance->update_relations( $post ); diff --git a/tests/Unit/Integrations/Watchers/Indexable_Term_Watcher_Test.php b/tests/Unit/Integrations/Watchers/Indexable_Term_Watcher_Test.php index dda79588f9f..c770d601568 100644 --- a/tests/Unit/Integrations/Watchers/Indexable_Term_Watcher_Test.php +++ b/tests/Unit/Integrations/Watchers/Indexable_Term_Watcher_Test.php @@ -217,6 +217,7 @@ public function test_build_indexable() { $this->indexable_helper ->expects( 'save_indexable' ) + ->with( $indexable ) ->once(); $this->instance->build_indexable( 1 ); @@ -381,6 +382,7 @@ public function test_build_does_not_exist() { $this->indexable_helper ->expects( 'save_indexable' ) + ->with( $indexable ) ->once(); $this->instance->build_indexable( 1 ); diff --git a/tests/Unit/Repositories/Indexable_Hierarchy_Repository_Test.php b/tests/Unit/Repositories/Indexable_Hierarchy_Repository_Test.php index 9f533c14096..118cbabc437 100644 --- a/tests/Unit/Repositories/Indexable_Hierarchy_Repository_Test.php +++ b/tests/Unit/Repositories/Indexable_Hierarchy_Repository_Test.php @@ -254,6 +254,7 @@ public function test_add_ancestor() { $this->indexable_helper ->expects( 'save_indexable' ) ->once() + ->with( $hierarchy ) ->andReturnTrue(); $this->assertTrue( $this->instance->add_ancestor( 1, 2, 1 ) ); From 50b5c162bd9eadae3c63792ff63971a49ef7d218 Mon Sep 17 00:00:00 2001 From: Leonidas Milosis Date: Wed, 22 May 2024 13:44:35 +0300 Subject: [PATCH 18/35] Add integration tests --- .../Primary_Term_Repository_Test.php | 106 ++++++++++++++++++ 1 file changed, 106 insertions(+) create mode 100644 tests/WP/Repositories/Primary_Term_Repository_Test.php diff --git a/tests/WP/Repositories/Primary_Term_Repository_Test.php b/tests/WP/Repositories/Primary_Term_Repository_Test.php new file mode 100644 index 00000000000..2ee61ce37f4 --- /dev/null +++ b/tests/WP/Repositories/Primary_Term_Repository_Test.php @@ -0,0 +1,106 @@ +insert( + $wpdb->prefix . 'yoast_primary_term', + [ + 'id' => '123', + 'post_id' => '110', + 'term_id' => '22', + 'taxonomy' => 'category', + 'created_at' => '2024-05-22 10:16:42', + 'updated_at' => '2024-05-22 10:16:42', + 'blog_id' => '1', + ] + ); + + $this->instance = new Primary_Term_Repository(); + } + + /** + * Tears down the test class. + * + * @return void + */ + public function tear_down(): void { + global $wpdb; + + $wpdb->query( "TRUNCATE TABLE {$wpdb->prefix}yoast_primary_term" ); + + parent::tear_down(); + } + + /** + * Tests the query method. + * + * @covers ::query + * + * @return void + */ + public function test_query() { + + $this->assertIsObject( $this->instance->query() ); + } + + /** + * Tests finding one entry by post id and taxonomy. + * + * @covers ::find_by_post_id_and_taxonomy + * + * @return void + */ + public function test_find_by_post_id_and_taxonomy_one_result() { + + $post_id = 110; + $taxonomy = 'category'; + + $result = $this->instance->find_by_post_id_and_taxonomy( $post_id, $taxonomy ); + + $this->assertInstanceOf( Primary_Term::class, $result, 'The result should be a Primary_term instance.' ); + $this->assertEquals( '123', $result->id, 'The id of the one result should be the one we expect.' ); + } + + /** + * Tests finding no entry by post id and taxonomy. + * + * @covers ::find_by_post_id_and_taxonomy + * + * @return void + */ + public function test_find_by_post_id_and_taxonomy_no_result() { + $post_id = 111; + $taxonomy = 'category'; + + $result = $this->instance->find_by_post_id_and_taxonomy( $post_id, $taxonomy, false ); + + $this->assertFalse( $result, 'The result should be false' ); + } +} From 522588efe88206e38aaaec6662407c22389f880c Mon Sep 17 00:00:00 2001 From: Leonidas Milosis Date: Tue, 28 May 2024 12:01:47 +0300 Subject: [PATCH 19/35] Return early when trying to save ancestor and indexing is disabled --- src/repositories/indexable-hierarchy-repository.php | 6 +++++- .../Indexable_Hierarchy_Repository_Test.php | 13 +++++++------ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/src/repositories/indexable-hierarchy-repository.php b/src/repositories/indexable-hierarchy-repository.php index 64f815b5922..1d83ed759b7 100644 --- a/src/repositories/indexable-hierarchy-repository.php +++ b/src/repositories/indexable-hierarchy-repository.php @@ -74,6 +74,10 @@ public function clear_ancestors( $indexable_id ) { * @return bool Whether or not the ancestor was added successfully. */ public function add_ancestor( $indexable_id, $ancestor_id, $depth ) { + if ( ! $this->indexable_helper->should_index_indexables() ) { + return false; + } + $hierarchy = $this->query()->create( [ 'indexable_id' => $indexable_id, @@ -83,7 +87,7 @@ public function add_ancestor( $indexable_id, $ancestor_id, $depth ) { ] ); - return $this->indexable_helper->save_indexable( $hierarchy ); + return $hierarchy->save(); } /** diff --git a/tests/Unit/Repositories/Indexable_Hierarchy_Repository_Test.php b/tests/Unit/Repositories/Indexable_Hierarchy_Repository_Test.php index 118cbabc437..3a06e429cbc 100644 --- a/tests/Unit/Repositories/Indexable_Hierarchy_Repository_Test.php +++ b/tests/Unit/Repositories/Indexable_Hierarchy_Repository_Test.php @@ -228,6 +228,11 @@ public function test_clear_ancestors() { public function test_add_ancestor() { $this->instance->set_helper( $this->indexable_helper ); + $this->indexable_helper + ->expects( 'should_index_indexables' ) + ->once() + ->andReturnTrue(); + $hierarchy = Mockery::mock( Indexable_Hierarchy_Mock::class ); $hierarchy->indexable_id = 1; $hierarchy->ancestor_id = 2; @@ -238,6 +243,8 @@ public function test_add_ancestor() { $orm_object = Mockery::mock( ORM::class )->makePartial(); + $hierarchy->expects( 'save' )->once()->andReturn( true ); + $orm_object ->expects( 'create' ) ->with( @@ -251,12 +258,6 @@ public function test_add_ancestor() { ->andReturn( $hierarchy ); $this->instance->expects( 'query' )->andReturn( $orm_object ); - $this->indexable_helper - ->expects( 'save_indexable' ) - ->once() - ->with( $hierarchy ) - ->andReturnTrue(); - $this->assertTrue( $this->instance->add_ancestor( 1, 2, 1 ) ); } From aef320b6c6373d558e2a75fb67c30cce20b1be7f Mon Sep 17 00:00:00 2001 From: Diede Exterkate <5352634+diedexx@users.noreply.github.com> Date: Fri, 5 Apr 2024 17:05:42 +0200 Subject: [PATCH 20/35] Improve sitemap performance and prevent all sitemap pages to be updated when an old post changes The performance improvement is mostly measured for individual sitemap pages for large sites on pages with a high page number. The old query forted by date, which required the full post table to be sorted before returning the right result. The new solution removes the need for sorting and instead keeps a map of post start end end ids of each page. This allows for fast lookups. The creation of the map is still a slow process and will be improved in a next commit. It used to be that, whenever an old post (on page 3) was updated, it would get bumped to the last page (page 40) because of its now recent last_mod date. This would result in the oldest post of the next page (page 4) to shift back one place, placing it on page 3, making its last_mod date more recent than the old value. This in turn causes bots to crawl the sitemap again for no reason, as the updated content is not on that sitemap page. --- .../class-post-type-sitemap-provider.php | 473 +++++++++++------- 1 file changed, 306 insertions(+), 167 deletions(-) diff --git a/inc/sitemaps/class-post-type-sitemap-provider.php b/inc/sitemaps/class-post-type-sitemap-provider.php index d018ad0b3ce..a83b50e286a 100644 --- a/inc/sitemaps/class-post-type-sitemap-provider.php +++ b/inc/sitemaps/class-post-type-sitemap-provider.php @@ -93,54 +93,15 @@ public function handles_type( $type ) { * @return array */ public function get_index_links( $max_entries ) { - global $wpdb; - $post_types = WPSEO_Post_Type::get_accessible_post_types(); - $post_types = array_filter( $post_types, [ $this, 'is_valid_post_type' ] ); - $last_modified_times = WPSEO_Sitemaps::get_last_modified_gmt( $post_types, true ); - $index = []; + $post_types = WPSEO_Post_Type::get_accessible_post_types(); + $post_types = array_filter( $post_types, [ $this, 'is_valid_post_type' ] ); + $index_links = []; foreach ( $post_types as $post_type ) { - - $total_count = $this->get_post_type_count( $post_type ); - - if ( $total_count === 0 ) { - continue; - } - - $max_pages = 1; - if ( $total_count > $max_entries ) { - $max_pages = (int) ceil( $total_count / $max_entries ); - } - - $all_dates = []; - - if ( $max_pages > 1 ) { - $all_dates = version_compare( $wpdb->db_version(), '8.0', '>=' ) ? $this->get_all_dates_using_with_clause( $post_type, $max_entries ) : $this->get_all_dates( $post_type, $max_entries ); - } - - for ( $page_counter = 0; $page_counter < $max_pages; $page_counter++ ) { - - $current_page = ( $page_counter === 0 ) ? '' : ( $page_counter + 1 ); - $date = false; - - if ( empty( $current_page ) || $current_page === $max_pages ) { - - if ( ! empty( $last_modified_times[ $post_type ] ) ) { - $date = $last_modified_times[ $post_type ]; - } - } - else { - $date = $all_dates[ $page_counter ]; - } - - $index[] = [ - 'loc' => WPSEO_Sitemaps_Router::get_base_url( $post_type . '-sitemap' . $current_page . '.xml' ), - 'lastmod' => $date, - ]; - } + $index_links = array_merge( $index_links, $this->get_index_links_for_post_type( $post_type, $max_entries ) ); } - return $index; + return $index_links; } /** @@ -155,7 +116,6 @@ public function get_index_links( $max_entries ) { * @throws OutOfBoundsException When an invalid page is requested. */ public function get_sitemap_links( $type, $max_entries, $current_page ) { - $links = []; $post_type = $type; @@ -163,43 +123,29 @@ public function get_sitemap_links( $type, $max_entries, $current_page ) { throw new OutOfBoundsException( 'Invalid sitemap page requested' ); } - $steps = min( 100, $max_entries ); - $offset = ( $current_page > 1 ) ? ( ( $current_page - 1 ) * $max_entries ) : 0; - $total = ( $offset + $max_entries ); - - $post_type_entries = $this->get_post_type_count( $post_type ); - - if ( $total > $post_type_entries ) { - $total = $post_type_entries; - } - - if ( $current_page === 1 ) { - $links = array_merge( $links, $this->get_first_links( $post_type ) ); - } + $page_boundaries = $this->get_page_boundaries( $post_type, $current_page, $max_entries ); // If total post type count is lower than the offset, an invalid page is requested. - if ( $post_type_entries < $offset ) { + if ( $page_boundaries['start'] > $page_boundaries['end'] ) { throw new OutOfBoundsException( 'Invalid sitemap page requested' ); } - if ( $post_type_entries === 0 ) { - return $links; + if ( $current_page === 1 ) { + $links = array_merge( $links, $this->get_first_links( $post_type ) ); } $posts_to_exclude = $this->get_excluded_posts( $type ); - - while ( $total > $offset ) { - - $posts = $this->get_posts( $post_type, $steps, $offset ); - - $offset += $steps; - + $start_post_id = $page_boundaries['start']; + while ( $start_post_id <= $page_boundaries['end'] ) { + $posts = $this->get_posts( $post_type, $max_entries, $start_post_id, $page_boundaries['end'] ); if ( empty( $posts ) ) { - continue; + break; } - foreach ( $posts as $post ) { + $last_id = (int) current( array_slice( $posts, -1 ) )->ID; + $start_post_id = ( $last_id + 1 ); + foreach ( $posts as $post ) { if ( in_array( $post->ID, $posts_to_exclude, true ) ) { continue; } @@ -227,8 +173,6 @@ public function get_sitemap_links( $type, $max_entries, $current_page ) { $links[] = $url; } } - - unset( $post, $url ); } return $links; @@ -499,18 +443,44 @@ protected function is_post_type_archive_indexable( $post_type, $archive_page_id /** * Retrieve set of posts with optimized query routine. * - * @param string $post_type Post type to retrieve. - * @param int $count Count of posts to retrieve. - * @param int $offset Starting offset. + * @param string $post_type Post type to retrieve. + * @param int $count Count of posts to retrieve. + * @param int $offset Starting offset. + * @param int|null $max_post_id The maximum post ID to retrieve. * - * @return object[] + * @return WP_Post[] */ - protected function get_posts( $post_type, $count, $offset ) { + protected function get_posts( $post_type, $count, $offset, $max_post_id = null ) { + $raw_post_data = $this->get_raw_post_data( $post_type, $count, $offset, $max_post_id ); + $posts = []; + $post_ids = []; + foreach ( $raw_post_data as $row_index => $post_data ) { + $post_data->post_type = $post_type; + $sanitized_post = sanitize_post( $post_data, 'raw' ); + $posts[ $row_index ] = new WP_Post( $sanitized_post ); + + $post_ids[] = $sanitized_post->ID; + } + + update_meta_cache( 'post', $post_ids ); + return $posts; + } + + /** + * Retrieve raw post data for a given post type within a range of post IDs. + * + * @param string $post_type The post type to retrieve posts for. + * @param int $count The maximum number of posts to retrieve. + * @param int $offset The starting post ID. + * @param int|null $max_post_id The maximum post ID to retrieve. + * + * @return stdClass[] + */ + protected function get_raw_post_data( $post_type, $count, $offset, $max_post_id = null ) { global $wpdb; static $filters = []; - if ( ! isset( $filters[ $post_type ] ) ) { // Make sure you're wpdb->preparing everything you throw into this!! $filters[ $post_type ] = [ @@ -536,39 +506,35 @@ protected function get_posts( $post_type, $count, $offset ) { $where_filter = $filters[ $post_type ]['where']; $where = $this->get_sql_where_clause( $post_type ); + $page_limit_clause = ( $max_post_id ) ? "AND {$wpdb->posts}.ID <= {$max_post_id}" : ''; + /* * Optimized query per this thread: * {@link http://wordpress.org/support/topic/plugin-wordpress-seo-by-yoast-performance-suggestion}. * Also see {@link http://explainextended.com/2009/10/23/mysql-order-by-limit-performance-late-row-lookups/}. */ + + $sql = " - SELECT l.ID, post_title, post_content, post_name, post_parent, post_author, post_status, post_modified_gmt, post_date, post_date_gmt + SELECT l.ID, post_title, post_content, post_name, post_parent, post_author, post_status, post_modified_gmt, post_date, post_date_gmt, post_password FROM ( SELECT {$wpdb->posts}.ID FROM {$wpdb->posts} {$join_filter} {$where} {$where_filter} - ORDER BY {$wpdb->posts}.post_modified ASC LIMIT %d OFFSET %d + AND {$wpdb->posts}.ID >= %d + {$page_limit_clause} + ORDER BY {$wpdb->posts}.ID ASC LIMIT %d ) o JOIN {$wpdb->posts} l ON l.ID = o.ID "; - $posts = $wpdb->get_results( $wpdb->prepare( $sql, $count, $offset ) ); - - $post_ids = []; - - foreach ( $posts as $post_index => $post ) { - $post->post_type = $post_type; - $sanitized_post = sanitize_post( $post, 'raw' ); - $posts[ $post_index ] = new WP_Post( $sanitized_post ); - - $post_ids[] = $sanitized_post->ID; - } + // phpcs:disable WordPress.DB.DirectDatabaseQuery.NoCaching,WordPress.DB.DirectDatabaseQuery.DirectQuery -- Can't do this with WP queries. Caching is done on the sitemap level. + $raw_post_data = $wpdb->get_results( $wpdb->prepare( $sql, $offset, $count ) ); + // phpcs:enable - update_meta_cache( 'post', $post_ids ); - - return $posts; + return array_values( $raw_post_data ); } /** @@ -668,99 +634,272 @@ protected function get_url( $post ) { } /** - * Get all dates for a post type by using the WITH clause for performance. + * Gets the boundaries of a single post sitemap page of a post type. * - * @param string $post_type Post type to retrieve dates for. - * @param int $max_entries Maximum number of entries to retrieve. + * @param string $post_type The post type to get the boundaries for. + * @param int $page_number The page number to get the boundaries for. Starting at 1. + * @param int $max_entries_per_page The maximum number of links that should be visible on a page. * - * @return array Array of dates. + * @return int[] + * @throws OutOfBoundsException When an invalid page is requested. */ - private function get_all_dates_using_with_clause( $post_type, $max_entries ) { - global $wpdb; + private function get_page_boundaries( $post_type, $page_number, $max_entries_per_page ) { + $map = $this->get_pagination_map( $post_type, $max_entries_per_page ); + if ( ! isset( $map[ $page_number ] ) ) { + throw new OutOfBoundsException( 'Invalid sitemap page requested' ); + } - $post_statuses = array_map( 'esc_sql', WPSEO_Sitemaps::get_post_statuses( $post_type ) ); + return $map[ $page_number ]; + } - $replacements = array_merge( - [ - 'ordering', - 'post_modified_gmt', - $wpdb->posts, - 'type_status_date', - 'post_status', - ], - $post_statuses, - [ - 'post_type', + /** + * Gets a full pagination map for a post type. + * The map is an array of page numbers with the starting and ending post id of the page. + * When the map is malformed or missing, it will be rebuilt. + * When the map is incomplete, it will be completed. + * + * @param string $post_type The post type to get the map for. + * @param int $max_entries_per_page The maximum number of links that should be visible on a page. + * + * @return array + */ + private function get_pagination_map( $post_type, $max_entries_per_page ) { + $pagination = get_option( "wpseo_{$post_type}_sitemap_pagination", [] ); + $aggregates = $this->get_aggregates( $post_type ); + + if ( $this->pagination_is_malformed( $pagination ) || $this->map_needs_to_be_rebuilt( $pagination, $aggregates, $max_entries_per_page ) ) { + $pagination_map = $this->create_map( $post_type, - 'post_modified_gmt', - 'post_modified_gmt', - 'ordering', - $max_entries, - ] - ); + $max_entries_per_page, + $aggregates['min_post_id'], + $aggregates['max_post_id'] + ); + $this->save_pagination( $post_type, $pagination_map, $aggregates, $max_entries_per_page ); - //phpcs:disable WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching -- We need to use a direct query here. - //phpcs:disable WordPress.DB.DirectDatabaseQuery.NoCaching -- Reason: No relevant caches. - return $wpdb->get_col( - //phpcs:disable WordPress.DB.PreparedSQLPlaceholders -- %i placeholder is still not recognized. - $wpdb->prepare( - ' - WITH %i AS (SELECT ROW_NUMBER() OVER (ORDER BY %i) AS n, post_modified_gmt - FROM %i USE INDEX ( %i ) - WHERE %i IN (' . implode( ', ', array_fill( 0, count( $post_statuses ), '%s' ) ) . ') - AND %i = %s - ORDER BY %i) - SELECT %i - FROM %i - WHERE MOD(n, %d) = 0; - ', - $replacements - ) - ); + return $pagination_map; + } + + // If the map is outdated, replace the last page with an updated version and complete the map. + if ( $this->map_is_missing_posts( $pagination, $aggregates ) ) { + $pagination_map = $pagination['map']; + // The existing map might be empty. E.g., when a post_type only has password protected posts. + if ( empty( $pagination_map ) ) { + $last_known_page = 1; + $first_id_on_last_page = $aggregates['min_post_id']; + } + else { + $last_known_page = max( array_keys( $pagination_map ) ); + $first_id_on_last_page = $pagination_map[ $last_known_page ]['start']; + } + array_pop( $pagination_map ); + + $final_pages = $this->create_map( + $post_type, + $max_entries_per_page, + $first_id_on_last_page, + $aggregates['max_post_id'], + $last_known_page + ); + + // Loop over pages instead of using array_merge to avoid losing array keys. + foreach ( $final_pages as $page_number => $page_boundaries ) { + $pagination_map[ $page_number ] = $page_boundaries; + } + + $this->save_pagination( $post_type, $pagination_map, $aggregates, $max_entries_per_page ); + + return $pagination_map; + } + + return $pagination['map']; } /** - * Get all dates for a post type. + * Gets a map of the starting and ending post id of sitemap pages for a post_type. * - * @param string $post_type Post type to retrieve dates for. - * @param int $max_entries Maximum number of entries to retrieve. + * @param string $post_type The post type to create the map for. + * @param int $max_entries_per_page The maximum number of links that should be visible on a page. + * @param int $min_post_id The minimum post id to start the map from. The lowest known post ID when creating a new map. + * @param int $max_post_id The highest known post ID. + * @param int $starting_page The page number to start the map from. Defaults to 1. * - * @return array Array of dates. + * @return array */ - private function get_all_dates( $post_type, $max_entries ) { - global $wpdb; + private function create_map( $post_type, $max_entries_per_page, $min_post_id, $max_post_id, $starting_page = 1 ) { + $map = []; + $current_page_id = max( $starting_page, 1 ); + $current_page_start_post_id = $min_post_id; - $post_statuses = array_map( 'esc_sql', WPSEO_Sitemaps::get_post_statuses( $post_type ) ); - $replacements = array_merge( - [ - 'post_modified_gmt', - $wpdb->posts, - 'type_status_date', - 'post_status', - ], - $post_statuses, + while ( $current_page_start_post_id <= $max_post_id ) { + $raw_post_data = $this->get_raw_post_data( $post_type, $max_entries_per_page, $current_page_start_post_id ); + + // No more posts left. + if ( count( $raw_post_data ) === 0 ) { + break; + } + + $last_post = $raw_post_data[ ( count( $raw_post_data ) - 1 ) ]; + $current_page_end_post_id = (int) $last_post->ID; + $map[ $current_page_id ] = [ + 'start' => $current_page_start_post_id, + 'end' => $current_page_end_post_id, + ]; + ++$current_page_id; + $current_page_start_post_id = ( $current_page_end_post_id + 1 ); + } + + return $map; + } + + /** + * Checks the structure of the pagination object. + * + * @param array $stored_pagination The stored pagination object to validate. + * + * @return bool + */ + private function pagination_is_malformed( $stored_pagination ): bool { + return ! isset( $stored_pagination['min_post_id'], $stored_pagination['max_post_id'], $stored_pagination['total_number_of_posts'], $stored_pagination['max_entries_per_page'] ); + } + + /** + * Checks if the map in a pagination object needs to be rebuilt completely. + * This is the case when: + * 1. posts exists with a lower ID than what the pagination object knows about; + * 2. the map has pages for posts with a higher ID than the highest ID in the database for this post type; + * 3. the total number of posts shrunk since creating the last pagination object. + * This can happen when posts are deleted, but could also hint at bigger site structure changes. + * 4. the site has been migrated to a different domain. + * + * This will trigger some false positives, causing some unnecessary rebuilds, + * but this will cover cases where bigger structural changes have been made that would otherwise go undetected. + * + * @param array $stored_pagination The stored pagination object. + * @param array{min_post_id: int, max_post_id: int, total_number_of_posts: int} $current_aggregates The current aggregates for the post type. + * @param int $max_entries_per_page The maximum number of links that should be visible on a page. + * + * @return bool + */ + private function map_needs_to_be_rebuilt( $stored_pagination, $current_aggregates, $max_entries_per_page ): bool { + return $stored_pagination['min_post_id'] !== $current_aggregates['min_post_id'] + || $stored_pagination['max_post_id'] > $current_aggregates['max_post_id'] + || $stored_pagination['total_number_of_posts'] > $current_aggregates['total_number_of_posts'] + || $stored_pagination['max_entries_per_page'] !== $max_entries_per_page + || $stored_pagination['site_url'] !== get_site_url(); + } + + /** + * Checks if the map in a pagination object is missing posts that have been published since the last update of the pagination object. + * + * @param array $stored_pagination The stored pagination object. + * @param array{min_post_id: int, max_post_id: int, total_number_of_posts: int} $current_aggregates The current aggregates for the post type. + * + * @return bool + */ + private function map_is_missing_posts( $stored_pagination, $current_aggregates ): bool { + return $stored_pagination['max_post_id'] < $current_aggregates['max_post_id']; + } + + /** + * Saves a pagination object to the database. + * + * @param string $post_type The post type to save the pagination object for. + * @param array $pagination_map The map to save as part of the pagination object. + * @param array{min_post_id: int, max_post_id: int, total_number_of_posts: int} $aggregates The aggregates that were used when creating the map. + * @param int $max_entries_per_page The maximum number of links that should be visible on a page. + * + * @return void + */ + private function save_pagination( $post_type, $pagination_map, $aggregates, $max_entries_per_page ): void { + update_option( + "wpseo_{$post_type}_sitemap_pagination", [ - 'post_type', - $post_type, - $max_entries, - 'post_modified_gmt', + 'map' => $pagination_map, + 'min_post_id' => $aggregates['min_post_id'], + 'max_post_id' => $aggregates['max_post_id'], + 'total_number_of_posts' => $aggregates['total_number_of_posts'], + 'max_entries_per_page' => $max_entries_per_page, + 'site_url' => get_site_url(), ] ); + } - return $wpdb->get_col( - //phpcs:disable WordPress.DB.PreparedSQLPlaceholders -- %i placeholder is still not recognized. + /** + * Gets links to show on the sitemap index/overview for a particular post type. + * + * @param string $post_type The post type to get index links for. + * @param int $max_entries The maximum number of links that should be visible on a page. + * + * @return array + */ + protected function get_index_links_for_post_type( string $post_type, int $max_entries ): array { + $index_links = []; + $pagination_map = $this->get_pagination_map( $post_type, $max_entries ); + + foreach ( $pagination_map as $page_number => $page_boundaries ) { + $raw_post_data = $this->get_raw_post_data( $post_type, $max_entries, $page_boundaries['start'], $page_boundaries['end'] ); + + $last_mod_gmt = array_reduce( + $raw_post_data, + static function ( $carry, $post ) { + return max( $post->post_modified_gmt, $carry ); + }, + $raw_post_data[0]->post_modified_gmt + ); + + $page_link_suffix = ( $page_number > 1 ) ? $page_number : ''; + + if ( $page_number === 1 ) { + $first_links = $this->get_first_links( $post_type ); + $last_mod_gmt = array_reduce( + $first_links, + static function ( $carry, $link ) { + if ( ! isset( $link['mod'] ) ) { + return $carry; + } + + return max( $link['mod'], $carry ); + }, + $last_mod_gmt + ); + } + + $index_links[] = [ + 'loc' => WPSEO_Sitemaps_Router::get_base_url( $post_type . '-sitemap' . $page_link_suffix . '.xml' ), + 'lastmod' => $last_mod_gmt, + ]; + } + + return $index_links; + } + + /** + * Gets current aggregates for a post type. + * + * @param string $post_type The post type to get aggregates for. + * + * @return array{min_post_id: int, max_post_id: int, total_number_of_posts: int} + */ + protected function get_aggregates( $post_type ): array { + global $wpdb; + // phpcs:disable WordPress.DB.DirectDatabaseQuery.NoCaching,WordPress.DB.DirectDatabaseQuery.DirectQuery -- Can't do this with WP queries. Caching is done on the sitemap level. + $aggregates = (array) $wpdb->get_row( $wpdb->prepare( - ' - SELECT %i - FROM ( SELECT @rownum:=0 ) init - JOIN %i USE INDEX( %i ) - WHERE %i IN (' . implode( ', ', array_fill( 0, count( $post_statuses ), '%s' ) ) . ') - AND %i = %s - AND ( @rownum:=@rownum+1 ) %% %d = 0 - ORDER BY %i ASC - ', - $replacements + "SELECT + MIN( ID ) AS min_post_id, + MAX( ID ) AS max_post_id, + COUNT( ID ) AS total_number_of_posts + FROM {$wpdb->posts} + WHERE post_type = %s", + $post_type ) ); + + // phpcs:enable + return [ + 'min_post_id' => (int) $aggregates['min_post_id'], + 'max_post_id' => (int) $aggregates['max_post_id'], + 'total_number_of_posts' => (int) $aggregates['total_number_of_posts'], + ]; } } From 4e5e3a5cf5cf29332bd022e78708dfe6c0f2a837 Mon Sep 17 00:00:00 2001 From: Diede Exterkate <5352634+diedexx@users.noreply.github.com> Date: Fri, 31 May 2024 15:07:26 +0200 Subject: [PATCH 21/35] Document existing behaviour --- inc/sitemaps/class-sitemaps.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/inc/sitemaps/class-sitemaps.php b/inc/sitemaps/class-sitemaps.php index 2fbb567c1fe..608622b3945 100644 --- a/inc/sitemaps/class-sitemaps.php +++ b/inc/sitemaps/class-sitemaps.php @@ -262,12 +262,15 @@ public function redirect( $query ) { return; } + // This may be the type of sitemap that's being requested or the value "1" for the root sitemap. $type = get_query_var( 'sitemap' ); + // example.com/sitemap-.xml is not a valid request. if ( empty( $type ) ) { return; } + // We don't use "page" numbers for the first (or 0th) page in a sitemap. if ( get_query_var( 'sitemap_n' ) === '1' || get_query_var( 'sitemap_n' ) === '0' ) { wp_safe_redirect( home_url( "/$type-sitemap.xml" ), 301, 'Yoast SEO' ); exit; From 238e25e97e6f1f9e6f5258462f9134c1fdc37464 Mon Sep 17 00:00:00 2001 From: Diede Exterkate <5352634+diedexx@users.noreply.github.com> Date: Mon, 10 Jun 2024 11:29:41 +0200 Subject: [PATCH 22/35] Test that the first page includes an archive link --- .../class-post-type-sitemap-provider.php | 6 +++--- .../Post_Type_Sitemap_Provider_Test.php | 21 ++++++++++++------- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/inc/sitemaps/class-post-type-sitemap-provider.php b/inc/sitemaps/class-post-type-sitemap-provider.php index a83b50e286a..ad6fc0d708b 100644 --- a/inc/sitemaps/class-post-type-sitemap-provider.php +++ b/inc/sitemaps/class-post-type-sitemap-provider.php @@ -116,8 +116,9 @@ public function get_index_links( $max_entries ) { * @throws OutOfBoundsException When an invalid page is requested. */ public function get_sitemap_links( $type, $max_entries, $current_page ) { - $links = []; - $post_type = $type; + $links = []; + $post_type = $type; + $current_page = max( $current_page, 1 ); if ( ! $this->is_valid_post_type( $post_type ) ) { throw new OutOfBoundsException( 'Invalid sitemap page requested' ); @@ -514,7 +515,6 @@ protected function get_raw_post_data( $post_type, $count, $offset, $max_post_id * Also see {@link http://explainextended.com/2009/10/23/mysql-order-by-limit-performance-late-row-lookups/}. */ - $sql = " SELECT l.ID, post_title, post_content, post_name, post_parent, post_author, post_status, post_modified_gmt, post_date, post_date_gmt, post_password FROM ( diff --git a/tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php b/tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php index 68668c8595c..7092eb4eec0 100644 --- a/tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php +++ b/tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php @@ -313,10 +313,15 @@ public function test_get_url() { * @return void */ public function test_regular_post() { - $this->factory->post->create(); + $post_id = $this->factory->post->create(); + + $expected_urls = [ + \trailingslashit( \home_url() ), + \get_permalink( $post_id ), + ]; + $actual_urls = \wp_list_pluck( self::$class_instance->get_sitemap_links( 'post', 100, 1 ), 'loc' ); - // Expect the created post to be in the sitemap list. - $this->assertCount( 1, self::$class_instance->get_sitemap_links( 'post', 100, 0 ) ); + $this->assertSame( $expected_urls, $actual_urls ); } /** @@ -333,11 +338,12 @@ public function test_password_protected_post() { 'post_password' => 'secret', ] ); + $this->factory->post->create(); // Expect the protected post should not be added. $this->assertCount( - 0, - self::$class_instance->get_sitemap_links( 'post', 100, 0 ), + 1, + self::$class_instance->get_sitemap_links( 'post', 100, 1 ), 'Password protected posts should not be in the sitemap' ); } @@ -369,7 +375,7 @@ public function test_regular_attachment() { ); // Expect the attchment to be in the list. - $this->assertCount( 1, self::$class_instance->get_sitemap_links( 'attachment', 100, 0 ) ); + $this->assertCount( 1, self::$class_instance->get_sitemap_links( 'attachment', 100, 1 ) ); } /** @@ -384,7 +390,6 @@ public function test_regular_attachment() { public function test_password_protected_post_parent_attachment() { // Enable attachments in the sitemap. WPSEO_Options::set( 'disable-attachment', false ); - // Create password protected post. $post_id = $this->factory->post->create( [ @@ -401,7 +406,7 @@ public function test_password_protected_post_parent_attachment() { ); // Expect the attachment not to be added to the list. - $this->assertCount( 0, self::$class_instance->get_sitemap_links( 'attachment', 100, 0 ) ); + $this->assertCount( 1, self::$class_instance->get_sitemap_links( 'attachment', 100, 1 ) ); } /** From 4dd55f40619d1e80ab39d84c60a87b1c4d679151 Mon Sep 17 00:00:00 2001 From: Diede Exterkate <5352634+diedexx@users.noreply.github.com> Date: Mon, 10 Jun 2024 12:02:25 +0200 Subject: [PATCH 23/35] Test that attachments from password protected posts are excluded from sitemaps The test had to be updated, since sitemaps without any posts now throw an out of bounds exception. This tests if only publicly viewable post's attachments end up in the sitemap. --- .../Post_Type_Sitemap_Provider_Test.php | 48 +++++++++++++++++-- 1 file changed, 45 insertions(+), 3 deletions(-) diff --git a/tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php b/tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php index 7092eb4eec0..15111cff5a8 100644 --- a/tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php +++ b/tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php @@ -2,6 +2,7 @@ namespace Yoast\WP\SEO\Tests\WP\Sitemaps; +use OutOfBoundsException; use WPSEO_Options; use WPSEO_Post_Type_Sitemap_Provider; use WPSEO_Sitemaps; @@ -390,8 +391,15 @@ public function test_regular_attachment() { public function test_password_protected_post_parent_attachment() { // Enable attachments in the sitemap. WPSEO_Options::set( 'disable-attachment', false ); + $public_post_id = $this->factory->post->create(); + $public_attachment_id = $this->factory->post->create( + [ + 'post_type' => 'attachment', + 'post_parent' => $public_post_id, + ] + ); // Create password protected post. - $post_id = $this->factory->post->create( + $password_protected_post_id = $this->factory->post->create( [ 'post_password' => 'secret', ] @@ -399,14 +407,48 @@ public function test_password_protected_post_parent_attachment() { $this->factory->post->create( [ - 'post_parent' => $post_id, + 'post_parent' => $password_protected_post_id, 'post_type' => 'attachment', 'post_status' => 'inherit', ] ); + $expected_urls = [ + \get_permalink( $public_attachment_id ), + ]; + $actual_urls = \wp_list_pluck( self::$class_instance->get_sitemap_links( 'attachment', 100, 1 ), 'loc' ); + // Expect the attachment not to be added to the list. - $this->assertCount( 1, self::$class_instance->get_sitemap_links( 'attachment', 100, 1 ) ); + $this->assertSame( $expected_urls, $actual_urls ); + } + + /** + * Tests to make sure that there is no attachment sitemap if all attachments are part of password protected posts. + * + * @covers ::get_sitemap_links + * + * @return void + */ + public function test_only_password_protected_post_parent_attachments() { + // Enable attachments in the sitemap. + WPSEO_Options::set( 'disable-attachment', false ); + // Create password protected post. + $password_protected_post_id = $this->factory->post->create( + [ + 'post_password' => 'secret', + ] + ); + + $this->factory->post->create( + [ + 'post_parent' => $password_protected_post_id, + 'post_type' => 'attachment', + 'post_status' => 'inherit', + ] + ); + + $this->expectException( OutOfBoundsException::class ); + self::$class_instance->get_sitemap_links( 'attachment', 100, 1 ); } /** From e836a2f1dc4cb7be08e5b1b9c6863cbb374050c9 Mon Sep 17 00:00:00 2001 From: Diede Exterkate <5352634+diedexx@users.noreply.github.com> Date: Mon, 10 Jun 2024 13:34:45 +0200 Subject: [PATCH 24/35] Test that password protected posts are excluded from sitemaps --- .../Post_Type_Sitemap_Provider_Test.php | 36 ++++++++++++------- 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php b/tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php index 15111cff5a8..13dcf4cb2e3 100644 --- a/tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php +++ b/tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php @@ -333,20 +333,32 @@ public function test_regular_post() { * @return void */ public function test_password_protected_post() { + // Create a public and password protected post. + $public_post_id = $this->factory->post->create(); + $this->factory->post->create( [ 'post_password' => 'secret' ] ); + + $expected_urls = [ + \trailingslashit( \home_url() ), + \get_permalink( $public_post_id ), + ]; + $actual_urls = \wp_list_pluck( self::$class_instance->get_sitemap_links( 'post', 100, 1 ), 'loc' ); + + $this->assertSame( $expected_urls, $actual_urls ); + } + + /** + * Tests to make sure that there is no sitemap if all posts are password protected. + * + * @covers ::get_sitemap_links + * + * @return void + */ + public function test_only_password_protected_posts() { // Create password protected post. - $this->factory->post->create( - [ - 'post_password' => 'secret', - ] - ); - $this->factory->post->create(); + $this->factory->post->create( [ 'post_password' => 'secret' ] ); - // Expect the protected post should not be added. - $this->assertCount( - 1, - self::$class_instance->get_sitemap_links( 'post', 100, 1 ), - 'Password protected posts should not be in the sitemap' - ); + $this->expectException( OutOfBoundsException::class ); + self::$class_instance->get_sitemap_links( 'post', 100, 1 ); } /** From 627205d64d7e0b187a11cbb32963de806aced6ac Mon Sep 17 00:00:00 2001 From: Diede Exterkate <5352634+diedexx@users.noreply.github.com> Date: Mon, 10 Jun 2024 14:45:50 +0200 Subject: [PATCH 25/35] Show the links on the first sitemap page even if there are no links to posts --- .../class-post-type-sitemap-provider.php | 49 ++++++++++++++++--- .../Post_Type_Sitemap_Provider_Test.php | 5 +- 2 files changed, 45 insertions(+), 9 deletions(-) diff --git a/inc/sitemaps/class-post-type-sitemap-provider.php b/inc/sitemaps/class-post-type-sitemap-provider.php index ad6fc0d708b..2b87af6f19c 100644 --- a/inc/sitemaps/class-post-type-sitemap-provider.php +++ b/inc/sitemaps/class-post-type-sitemap-provider.php @@ -124,15 +124,18 @@ public function get_sitemap_links( $type, $max_entries, $current_page ) { throw new OutOfBoundsException( 'Invalid sitemap page requested' ); } - $page_boundaries = $this->get_page_boundaries( $post_type, $current_page, $max_entries ); - - // If total post type count is lower than the offset, an invalid page is requested. - if ( $page_boundaries['start'] > $page_boundaries['end'] ) { - throw new OutOfBoundsException( 'Invalid sitemap page requested' ); + if ( $current_page === 1 ) { + $links = $this->get_first_links( $post_type ); } - if ( $current_page === 1 ) { - $links = array_merge( $links, $this->get_first_links( $post_type ) ); + // Even if we don't have posts for the page, we might have some first-page links to show. + try { + $page_boundaries = $this->get_page_boundaries( $post_type, $current_page, $max_entries ); + } catch ( OutOfBoundsException $e ) { + if ( count( $links ) > 0 ) { + return $links; + } + throw $e; } $posts_to_exclude = $this->get_excluded_posts( $type ); @@ -759,7 +762,37 @@ private function create_map( $post_type, $max_entries_per_page, $min_post_id, $m * @return bool */ private function pagination_is_malformed( $stored_pagination ): bool { - return ! isset( $stored_pagination['min_post_id'], $stored_pagination['max_post_id'], $stored_pagination['total_number_of_posts'], $stored_pagination['max_entries_per_page'] ); + return ( + ! isset( + $stored_pagination['min_post_id'], + $stored_pagination['max_post_id'], + $stored_pagination['total_number_of_posts'], + $stored_pagination['max_entries_per_page'], + $stored_pagination['map'] + ) + || $this->map_is_malformed( $stored_pagination['map'] ) + ); + } + + /** + * Checks the structure of the pagination map. + * A map is malformed when it is not an array or when it contains pages with a start ID higher than the end ID. + * + * @param array{start: int, end: int} $map The map. + * + * @return bool + */ + private function map_is_malformed( $map ): bool { + if ( ! is_array( $map ) ) { + return true; + } + foreach ( $map as $page ) { + if ( ! isset( $page['start'], $page['end'] ) || $page['end'] < $page['start'] ) { + return true; + } + } + + return false; } /** diff --git a/tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php b/tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php index 13dcf4cb2e3..7c2efc63464 100644 --- a/tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php +++ b/tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php @@ -356,9 +356,12 @@ public function test_password_protected_post() { public function test_only_password_protected_posts() { // Create password protected post. $this->factory->post->create( [ 'post_password' => 'secret' ] ); + $this->factory->post->create( [ 'post_password' => 'secret' ] ); + $this->factory->post->create( [ 'post_password' => 'secret' ] ); $this->expectException( OutOfBoundsException::class ); - self::$class_instance->get_sitemap_links( 'post', 100, 1 ); + + self::$class_instance->get_sitemap_links( 'post', 1, 2 ); } /** From af6f1072efda1a1073337d576d214e463ee14bc5 Mon Sep 17 00:00:00 2001 From: Diede Exterkate <5352634+diedexx@users.noreply.github.com> Date: Tue, 11 Jun 2024 09:26:47 +0200 Subject: [PATCH 26/35] Speed up generation of sitemap pagination maps --- .../class-post-type-sitemap-provider.php | 64 +++++++++++++------ 1 file changed, 46 insertions(+), 18 deletions(-) diff --git a/inc/sitemaps/class-post-type-sitemap-provider.php b/inc/sitemaps/class-post-type-sitemap-provider.php index 2b87af6f19c..8f938afd2c7 100644 --- a/inc/sitemaps/class-post-type-sitemap-provider.php +++ b/inc/sitemaps/class-post-type-sitemap-provider.php @@ -540,6 +540,43 @@ protected function get_raw_post_data( $post_type, $count, $offset, $max_post_id return array_values( $raw_post_data ); } + /** + * Gets the first id of the post of all pages. + * + * @param string $post_type The post type to retrieve posts for. + * @param int $entries_per_page The maximum number of posts per page. + * @param int $starting_post_id The lowest post ID to get the pages for. + * + * @return string[] + */ + protected function get_raw_page_data( $post_type, $entries_per_page, $starting_post_id ) { + global $wpdb; + + $where = $this->get_sql_where_clause( $post_type ); + + /* + * Optimized query per this thread: + * {@link http://wordpress.org/support/topic/plugin-wordpress-seo-by-yoast-performance-suggestion}. + * Also see {@link http://explainextended.com/2009/10/23/mysql-order-by-limit-performance-late-row-lookups/}. + */ + $sql = " + SELECT l.ID as first_id_on_page + FROM ( + SELECT {$wpdb->posts}.ID + FROM ( SELECT @rownum:=-1 ) as init + JOIN {$wpdb->posts} USE INDEX (`type_status_date`) + {$where} + AND {$wpdb->posts}.ID >= %d + ORDER BY {$wpdb->posts}.ID ASC + ) + o JOIN {$wpdb->posts} l ON l.ID = o.ID + WHERE (@rownum:=@rownum+1) %% %d = 0 + "; + // phpcs:disable WordPress.DB.DirectDatabaseQuery.NoCaching,WordPress.DB.DirectDatabaseQuery.DirectQuery -- Can't do this with WP queries. Caching is done on the sitemap level. + return $wpdb->get_col( $wpdb->prepare( $sql, $starting_post_id, $entries_per_page ) ); + // phpcs:enable + } + /** * Constructs an SQL where clause for a given post type. * @@ -729,26 +766,17 @@ private function get_pagination_map( $post_type, $max_entries_per_page ) { * @return array */ private function create_map( $post_type, $max_entries_per_page, $min_post_id, $max_post_id, $starting_page = 1 ) { - $map = []; - $current_page_id = max( $starting_page, 1 ); - $current_page_start_post_id = $min_post_id; - - while ( $current_page_start_post_id <= $max_post_id ) { - $raw_post_data = $this->get_raw_post_data( $post_type, $max_entries_per_page, $current_page_start_post_id ); - - // No more posts left. - if ( count( $raw_post_data ) === 0 ) { - break; - } - - $last_post = $raw_post_data[ ( count( $raw_post_data ) - 1 ) ]; - $current_page_end_post_id = (int) $last_post->ID; - $map[ $current_page_id ] = [ - 'start' => $current_page_start_post_id, - 'end' => $current_page_end_post_id, + $map = []; + $current_page_id = max( $starting_page, 1 ); + + $page_starting_ids = $this->get_raw_page_data( $post_type, $max_entries_per_page, $min_post_id ); + foreach ( $page_starting_ids as $index => $page_starting_id ) { + $next_page_starting_id = isset( $page_starting_ids[ ( $index + 1 ) ] ) ? ( $page_starting_ids[ ( $index + 1 ) ] ) : null; + $map[ $current_page_id ] = [ + 'start' => (int) $page_starting_id, + 'end' => $next_page_starting_id ? ( $next_page_starting_id - 1 ) : $max_post_id, ]; ++$current_page_id; - $current_page_start_post_id = ( $current_page_end_post_id + 1 ); } return $map; From 638c6731622c6e0ffc6fe15478015865d9af9d26 Mon Sep 17 00:00:00 2001 From: Diede Exterkate <5352634+diedexx@users.noreply.github.com> Date: Tue, 11 Jun 2024 09:31:17 +0200 Subject: [PATCH 27/35] Reduce complexity of get_first_links method --- .../class-post-type-sitemap-provider.php | 30 +++++++++---------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/inc/sitemaps/class-post-type-sitemap-provider.php b/inc/sitemaps/class-post-type-sitemap-provider.php index 8f938afd2c7..56801d91227 100644 --- a/inc/sitemaps/class-post-type-sitemap-provider.php +++ b/inc/sitemaps/class-post-type-sitemap-provider.php @@ -304,9 +304,7 @@ protected function get_post_type_count( $post_type ) { * @return array */ protected function get_first_links( $post_type ) { - - $links = []; - $archive_url = false; + $links = []; if ( $post_type === 'page' ) { @@ -332,7 +330,8 @@ protected function get_first_links( $post_type ) { /** * Filter images to be included for the term in XML sitemap. * - * @param array $images Array of image items. + * @param array $images Array of image items. + * * @return array $image_list Array of image items. */ $image_list = apply_filters( 'wpseo_sitemap_urlimages_front_page', $images ); @@ -342,30 +341,29 @@ protected function get_first_links( $post_type ) { $links[] = $front_page; } - elseif ( $post_type !== 'page' ) { + else { /** * Filter the URL Yoast SEO uses in the XML sitemap for this post type archive. * * @param string $archive_url The URL of this archive * @param string $post_type The post type this archive is for. */ - $archive_url = apply_filters( + $archive_url = (string) apply_filters( 'wpseo_sitemap_post_type_archive_link', $this->get_post_type_archive_link( $post_type ), $post_type ); - } - - if ( $archive_url ) { - $links[] = [ - 'loc' => $archive_url, - 'mod' => WPSEO_Sitemaps::get_last_modified_gmt( $post_type ), + if ( $archive_url !== '' ) { + $links[] = [ + 'loc' => $archive_url, + 'mod' => WPSEO_Sitemaps::get_last_modified_gmt( $post_type ), - // Deprecated, kept for backwards data compat. R. - 'chf' => 'daily', - 'pri' => 1, - ]; + // Deprecated, kept for backwards data compat. R. + 'chf' => 'daily', + 'pri' => 1, + ]; + } } /** From e1a7f3774901e44510fc13571d2b453c1becfb91 Mon Sep 17 00:00:00 2001 From: Diede Exterkate <5352634+diedexx@users.noreply.github.com> Date: Tue, 11 Jun 2024 15:33:47 +0200 Subject: [PATCH 28/35] Cleanup unused method --- .../class-post-type-sitemap-provider.php | 40 ------------------- 1 file changed, 40 deletions(-) diff --git a/inc/sitemaps/class-post-type-sitemap-provider.php b/inc/sitemaps/class-post-type-sitemap-provider.php index 56801d91227..79b58b55d3b 100644 --- a/inc/sitemaps/class-post-type-sitemap-provider.php +++ b/inc/sitemaps/class-post-type-sitemap-provider.php @@ -256,46 +256,6 @@ protected function get_excluded_posts( $post_type ) { return array_unique( $excluded_posts_ids ); } - /** - * Get count of posts for post type. - * - * @param string $post_type Post type to retrieve count for. - * - * @return int - */ - protected function get_post_type_count( $post_type ) { - - global $wpdb; - - /** - * Filter JOIN query part for type count of post type. - * - * @param string $join SQL part, defaults to empty string. - * @param string $post_type Post type name. - */ - $join_filter = apply_filters( 'wpseo_typecount_join', '', $post_type ); - - /** - * Filter WHERE query part for type count of post type. - * - * @param string $where SQL part, defaults to empty string. - * @param string $post_type Post type name. - */ - $where_filter = apply_filters( 'wpseo_typecount_where', '', $post_type ); - - $where = $this->get_sql_where_clause( $post_type ); - - $sql = " - SELECT COUNT({$wpdb->posts}.ID) - FROM {$wpdb->posts} - {$join_filter} - {$where} - {$where_filter} - "; - - return (int) $wpdb->get_var( $sql ); - } - /** * Produces set of links to prepend at start of first sitemap page. * From 1aac1dc4561c0258f7e69ed7a3bfb2fbf38a070f Mon Sep 17 00:00:00 2001 From: Diede Exterkate <5352634+diedexx@users.noreply.github.com> Date: Tue, 11 Jun 2024 15:35:30 +0200 Subject: [PATCH 29/35] Ensure that sitemap with links on the first page are shown on the sitemap index --- .../class-post-type-sitemap-provider.php | 37 ++++++++++++------- ...dexable_Post_Type_Archive_Builder_Test.php | 13 +++++++ .../Generators/Schema/Organization_Test.php | 12 ++++++ tests/WP/Generators/Schema/Person_Test.php | 12 ++++++ .../Post_Type_Sitemap_Provider_Test.php | 21 +++++++++++ tests/WP/Sitemaps/Sitemaps_Test.php | 1 + 6 files changed, 83 insertions(+), 13 deletions(-) diff --git a/inc/sitemaps/class-post-type-sitemap-provider.php b/inc/sitemaps/class-post-type-sitemap-provider.php index 79b58b55d3b..c855d3063d9 100644 --- a/inc/sitemaps/class-post-type-sitemap-provider.php +++ b/inc/sitemaps/class-post-type-sitemap-provider.php @@ -849,27 +849,38 @@ private function save_pagination( $post_type, $pagination_map, $aggregates, $max * @param string $post_type The post type to get index links for. * @param int $max_entries The maximum number of links that should be visible on a page. * - * @return array + * @return array{loc: string, lastmod: string} */ protected function get_index_links_for_post_type( string $post_type, int $max_entries ): array { $index_links = []; $pagination_map = $this->get_pagination_map( $post_type, $max_entries ); - foreach ( $pagination_map as $page_number => $page_boundaries ) { - $raw_post_data = $this->get_raw_post_data( $post_type, $max_entries, $page_boundaries['start'], $page_boundaries['end'] ); - - $last_mod_gmt = array_reduce( - $raw_post_data, - static function ( $carry, $post ) { - return max( $post->post_modified_gmt, $carry ); - }, - $raw_post_data[0]->post_modified_gmt - ); - + // Ensure a first page, so we can check for first links. + $pages = array_unique( array_merge( [ 1 ], array_keys( $pagination_map ) ) ); + foreach ( $pages as $page_number ) { $page_link_suffix = ( $page_number > 1 ) ? $page_number : ''; + $last_mod_gmt = null; + $raw_post_data = null; + if ( isset( $pagination_map[ $page_number ] ) ) { + $page_boundaries = $pagination_map[ $page_number ]; + $raw_post_data = $this->get_raw_post_data( $post_type, $max_entries, $page_boundaries['start'], $page_boundaries['end'] ); + + $last_mod_gmt = array_reduce( + $raw_post_data, + static function ( $carry, $post ) { + return max( $post->post_modified_gmt, $carry ); + }, + $raw_post_data[0]->post_modified_gmt + ); + } if ( $page_number === 1 ) { - $first_links = $this->get_first_links( $post_type ); + $first_links = $this->get_first_links( $post_type ); + + if ( ! $first_links && ! $raw_post_data ) { + continue; + } + $last_mod_gmt = array_reduce( $first_links, static function ( $carry, $link ) { diff --git a/tests/WP/Builders/Indexable_Post_Type_Archive_Builder_Test.php b/tests/WP/Builders/Indexable_Post_Type_Archive_Builder_Test.php index 60fda177480..1b9c7ad3685 100644 --- a/tests/WP/Builders/Indexable_Post_Type_Archive_Builder_Test.php +++ b/tests/WP/Builders/Indexable_Post_Type_Archive_Builder_Test.php @@ -38,6 +38,19 @@ public function set_up(): void { ); } + /** + * Remove the custom post type after each test. + * + * @return void + */ + public function tear_down() { + // Remove possibly set post type. + \unregister_post_type( 'my-custom-post-type' ); + \unregister_post_type( 'my-private-post-type' ); + + parent::tear_down(); + } + /** * Tests the build method's happy path. * diff --git a/tests/WP/Generators/Schema/Organization_Test.php b/tests/WP/Generators/Schema/Organization_Test.php index ec06d78e4ce..51f9220952e 100644 --- a/tests/WP/Generators/Schema/Organization_Test.php +++ b/tests/WP/Generators/Schema/Organization_Test.php @@ -87,6 +87,18 @@ public function set_up() { $this->context = $meta_tags_context_memoizer->get( $built_indexable, 'page' ); } + /** + * Remove the custom post type after each test. + * + * @return void + */ + public function tear_down() { + // Remove possibly set post type. + \unregister_post_type( 'my-custom-post-type' ); + + parent::tear_down(); + } + /** * Tests that the type is properly set if everything is in the default setting. * diff --git a/tests/WP/Generators/Schema/Person_Test.php b/tests/WP/Generators/Schema/Person_Test.php index fe99d947980..c2479cf0c75 100644 --- a/tests/WP/Generators/Schema/Person_Test.php +++ b/tests/WP/Generators/Schema/Person_Test.php @@ -87,6 +87,18 @@ public function set_up() { $this->context = $meta_tags_context_memoizer->get( $built_indexable, 'page' ); } + /** + * Remove the custom post type after each test. + * + * @return void + */ + public function tear_down() { + // Remove possibly set post type. + \unregister_post_type( 'my-custom-post-type' ); + + parent::tear_down(); + } + /** * Tests that the type is properly set if everything is in the default setting. * diff --git a/tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php b/tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php index 7c2efc63464..d9d246f9f9d 100644 --- a/tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php +++ b/tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php @@ -51,10 +51,31 @@ public function set_up() { * @return void */ public function test_get_index_links_no_entries() { + \add_filter( 'wpseo_sitemap_exclude_post_type', '__return_true' ); $index_links = self::$class_instance->get_index_links( 1 ); $this->assertEquals( [], $index_links ); } + /** + * Any post type that has links on the first page of the sitemap should be included in the index. + * + * @covers ::get_index_links + * + * @return void + */ + public function test_get_index_links_no_posts() { + $index_links = \wp_list_pluck( self::$class_instance->get_index_links( 1 ), 'loc' ); + $this->assertSame( \wp_count_posts( 'post' )->publish, 0 ); + $this->assertSame( \wp_count_posts( 'page' )->publish, 0 ); + $this->assertEquals( + [ + 'http://example.org/post-sitemap.xml', + 'http://example.org/page-sitemap.xml', + ], + $index_links + ); + } + /** * Multiple pages of a post-type should result in multiple index entries. * diff --git a/tests/WP/Sitemaps/Sitemaps_Test.php b/tests/WP/Sitemaps/Sitemaps_Test.php index 532f936d55b..8c6e48dadd9 100644 --- a/tests/WP/Sitemaps/Sitemaps_Test.php +++ b/tests/WP/Sitemaps/Sitemaps_Test.php @@ -162,6 +162,7 @@ public function test_last_modified_post_type() { $this->factory->post->create( $post_args ); $this->assertEquals( $newest_date, WPSEO_Sitemaps::get_last_modified_gmt( [ 'yoast' ] ) ); + \unregister_post_type( 'yoast' ); } /** From 7cb8abcf69392689e8aac1912cf4e6364cf737d5 Mon Sep 17 00:00:00 2001 From: Diede Exterkate <5352634+diedexx@users.noreply.github.com> Date: Tue, 11 Jun 2024 16:10:03 +0200 Subject: [PATCH 30/35] Document why we update_meta_cache for all posts --- inc/sitemaps/class-post-type-sitemap-provider.php | 1 + 1 file changed, 1 insertion(+) diff --git a/inc/sitemaps/class-post-type-sitemap-provider.php b/inc/sitemaps/class-post-type-sitemap-provider.php index c855d3063d9..c929af73d01 100644 --- a/inc/sitemaps/class-post-type-sitemap-provider.php +++ b/inc/sitemaps/class-post-type-sitemap-provider.php @@ -424,6 +424,7 @@ protected function get_posts( $post_type, $count, $offset, $max_post_id = null ) $post_ids[] = $sanitized_post->ID; } + // We'll need meta values for individual posts, so let's prime the cache with a single query instead of one per post. update_meta_cache( 'post', $post_ids ); return $posts; From f127aa7bdae8264cbbd10dfedb95f5def035923e Mon Sep 17 00:00:00 2001 From: Diede Exterkate <5352634+diedexx@users.noreply.github.com> Date: Wed, 19 Jun 2024 11:47:19 +0200 Subject: [PATCH 31/35] Prevent excluded posts to affect the sitemap index lastmod date --- inc/sitemaps/class-post-type-sitemap-provider.php | 14 +++++++------- .../Sitemaps/Post_Type_Sitemap_Provider_Test.php | 10 +++++++--- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/inc/sitemaps/class-post-type-sitemap-provider.php b/inc/sitemaps/class-post-type-sitemap-provider.php index c929af73d01..61721e77c58 100644 --- a/inc/sitemaps/class-post-type-sitemap-provider.php +++ b/inc/sitemaps/class-post-type-sitemap-provider.php @@ -138,8 +138,7 @@ public function get_sitemap_links( $type, $max_entries, $current_page ) { throw $e; } - $posts_to_exclude = $this->get_excluded_posts( $type ); - $start_post_id = $page_boundaries['start']; + $start_post_id = $page_boundaries['start']; while ( $start_post_id <= $page_boundaries['end'] ) { $posts = $this->get_posts( $post_type, $max_entries, $start_post_id, $page_boundaries['end'] ); if ( empty( $posts ) ) { @@ -150,10 +149,6 @@ public function get_sitemap_links( $type, $max_entries, $current_page ) { $start_post_id = ( $last_id + 1 ); foreach ( $posts as $post ) { - if ( in_array( $post->ID, $posts_to_exclude, true ) ) { - continue; - } - if ( WPSEO_Meta::get_value( 'meta-robots-noindex', $post->ID ) === '1' ) { continue; } @@ -546,6 +541,7 @@ protected function get_raw_page_data( $post_type, $entries_per_page, $starting_p protected function get_sql_where_clause( $post_type ) { global $wpdb; + $posts_to_exclude = $this->get_excluded_posts( $post_type ); $join = ''; $post_statuses = array_map( 'esc_sql', WPSEO_Sitemaps::get_post_statuses( $post_type ) ); @@ -566,7 +562,11 @@ protected function get_sql_where_clause( $post_type ) { AND {$wpdb->posts}.post_date != '0000-00-00 00:00:00' "; - return $wpdb->prepare( $where_clause, $post_type ); + if ( count( $posts_to_exclude ) > 0 ) { + $where_clause .= "AND {$wpdb->posts}.ID NOT IN (" . implode( ', ', array_fill( 0, count( $posts_to_exclude ), '%d' ) ) . ')'; + } + + return $wpdb->prepare( $where_clause, $post_type, ...$posts_to_exclude ); } /** diff --git a/tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php b/tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php index d9d246f9f9d..6d55aa80e9a 100644 --- a/tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php +++ b/tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php @@ -123,12 +123,10 @@ public function test_get_index_links_multiple_entries_non_paged() { * @return void */ public function test_get_index_links_empty_bucket() { - $this->factory->post->create(); $this->excluded_posts = [ $this->factory->post->create() ]; // Remove this post. $this->factory->post->create(); - \add_filter( 'wpseo_exclude_from_sitemap_by_post_ids', [ $this, 'exclude_post' ] ); \add_filter( 'wpseo_sitemap_entries_per_page', [ $this, 'return_one' ] ); // Fetch the global sitemap. @@ -137,10 +135,16 @@ public function test_get_index_links_empty_bucket() { // Set the page to the second one, which should not contain an entry, but should exist. \set_query_var( 'sitemap_n', '2' ); - // Load the sitemap. + // Load the sitemap to generate and store the pagination map. $sitemaps = new Sitemaps_Double(); $sitemaps->redirect( $GLOBALS['wp_the_query'] ); + $this->expectOutputContains( \get_permalink( $this->excluded_posts[0] ) ); + + // Now exclude the only post on page 2. + \add_filter( 'wpseo_exclude_from_sitemap_by_post_ids', [ $this, 'exclude_post' ] ); + $sitemaps->redirect( $GLOBALS['wp_the_query'] ); + // Expect an empty list to be output. $this->expectOutputContains( 'xmlns="http://www.sitemaps.org/schemas/sitemap/0.9">' . "\r\n" . '' From 2902960d395a30e28c65cd32f149a42244f7d773 Mon Sep 17 00:00:00 2001 From: Diede Exterkate <5352634+diedexx@users.noreply.github.com> Date: Wed, 19 Jun 2024 11:54:08 +0200 Subject: [PATCH 32/35] Speed up sitemap index by using a single query instead of one per page --- .../class-post-type-sitemap-provider.php | 106 +++++++++++++----- 1 file changed, 76 insertions(+), 30 deletions(-) diff --git a/inc/sitemaps/class-post-type-sitemap-provider.php b/inc/sitemaps/class-post-type-sitemap-provider.php index 61721e77c58..b138bbd04f8 100644 --- a/inc/sitemaps/class-post-type-sitemap-provider.php +++ b/inc/sitemaps/class-post-type-sitemap-provider.php @@ -494,6 +494,46 @@ protected function get_raw_post_data( $post_type, $count, $offset, $max_post_id return array_values( $raw_post_data ); } + /** + * Gets aggregated data for all pages in a pagination map for a post type. + * Empty pages won't show up in the results. + * Posts with IDs that aren't covered by the pagination map will be added to page 1. + * + * @param array $pagination_map The pagination map to use for grouping results. + * @param string $post_type The post type to get the aggregates for. + * + * @return array{page: int, last_mod_gmt: string}[] An array of aggregates with page. Page numbers start at 1. + */ + private function get_page_aggregates( $pagination_map, string $post_type ) { + global $wpdb; + + if ( empty( $pagination_map ) ) { + return []; + } + + $where_clause = $this->get_sql_where_clause( $post_type ); + $cases = []; + $replacements = []; + foreach ( $pagination_map as $page_number => $page ) { + $cases[] = 'WHEN ID >= %d AND ID <= %d THEN %d'; + array_push( $replacements, $page['start'], $page['end'], $page_number ); + } + + $query = $wpdb->prepare( + 'SELECT + CASE ' . implode( ' ', $cases ) . " ELSE 1 END as page, + MAX(post_modified_gmt) as last_mod_gmt, + COUNT(ID) as count + FROM {$wpdb->posts} + {$where_clause} + GROUP by page + ORDER BY page ASC", + ...$replacements + ); + + return (array) $wpdb->get_results( $query, OBJECT_K ); + } + /** * Gets the first id of the post of all pages. * @@ -855,44 +895,33 @@ private function save_pagination( $post_type, $pagination_map, $aggregates, $max protected function get_index_links_for_post_type( string $post_type, int $max_entries ): array { $index_links = []; $pagination_map = $this->get_pagination_map( $post_type, $max_entries ); + $pages = $this->get_page_aggregates( $pagination_map, $post_type ); - // Ensure a first page, so we can check for first links. - $pages = array_unique( array_merge( [ 1 ], array_keys( $pagination_map ) ) ); - foreach ( $pages as $page_number ) { - $page_link_suffix = ( $page_number > 1 ) ? $page_number : ''; - $last_mod_gmt = null; - $raw_post_data = null; - if ( isset( $pagination_map[ $page_number ] ) ) { - $page_boundaries = $pagination_map[ $page_number ]; - $raw_post_data = $this->get_raw_post_data( $post_type, $max_entries, $page_boundaries['start'], $page_boundaries['end'] ); - - $last_mod_gmt = array_reduce( - $raw_post_data, - static function ( $carry, $post ) { - return max( $post->post_modified_gmt, $carry ); - }, - $raw_post_data[0]->post_modified_gmt - ); + // If there's no pages with posts, check for any "first links" to create a page 1 with. + if ( empty( $pages ) ) { + $first_links = $this->get_first_links( $post_type ); + if ( ! $first_links ) { + return $index_links; } + $last_mod_gmt = array_reduce( $first_links, [ $this, 'reduce_most_recent_last_mod_for_links' ], ( $first_links[0]['mod'] ?? null ) ); + + $index_links[] = [ + 'loc' => WPSEO_Sitemaps_Router::get_base_url( $post_type . '-sitemap.xml' ), + 'lastmod' => $last_mod_gmt, + ]; + } + + foreach ( $pages as $page_number => $page ) { + $page_link_suffix = ( $page_number > 1 ) ? $page_number : ''; + $last_mod_gmt = $page->last_mod_gmt; if ( $page_number === 1 ) { $first_links = $this->get_first_links( $post_type ); - - if ( ! $first_links && ! $raw_post_data ) { + if ( ! $first_links && ! $last_mod_gmt ) { continue; } - $last_mod_gmt = array_reduce( - $first_links, - static function ( $carry, $link ) { - if ( ! isset( $link['mod'] ) ) { - return $carry; - } - - return max( $link['mod'], $carry ); - }, - $last_mod_gmt - ); + $last_mod_gmt = array_reduce( $first_links, [ $this, 'reduce_most_recent_last_mod_for_links' ], $last_mod_gmt ); } $index_links[] = [ @@ -904,6 +933,23 @@ static function ( $carry, $link ) { return $index_links; } + /** + * Returns the most recent last modified date for a set of links. + * Intended to be used with array_reduce. + * + * @param string $carry The string representation most recent last modified date. e.g. '2021-02-03 01:02:03'. + * @param array{loc: string, lastmod: string} $link The link to compare to the carry. + * + * @return string The most recent last modified date. + */ + private function reduce_most_recent_last_mod_for_links( $carry, $link ) { + if ( ! isset( $link['mod'] ) ) { + return $carry; + } + + return max( $link['mod'], $carry ); + } + /** * Gets current aggregates for a post type. * From 2337ffb78ba03396398e9d745cfae4e34831fe8e Mon Sep 17 00:00:00 2001 From: Diede Exterkate <5352634+diedexx@users.noreply.github.com> Date: Wed, 19 Jun 2024 11:56:26 +0200 Subject: [PATCH 33/35] Restore the 8.0+ query variant According to #20688 the 5.7 variant triggers warning ons 8.0+ --- .../class-post-type-sitemap-provider.php | 28 ++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/inc/sitemaps/class-post-type-sitemap-provider.php b/inc/sitemaps/class-post-type-sitemap-provider.php index b138bbd04f8..115d8b8071a 100644 --- a/inc/sitemaps/class-post-type-sitemap-provider.php +++ b/inc/sitemaps/class-post-type-sitemap-provider.php @@ -545,9 +545,34 @@ private function get_page_aggregates( $pagination_map, string $post_type ) { */ protected function get_raw_page_data( $post_type, $entries_per_page, $starting_post_id ) { global $wpdb; - $where = $this->get_sql_where_clause( $post_type ); + // Run a different query for MySQL 8.0 and up. It's ever so slightly faster than the 5.7 alternative and doesn't produce a warning. + // It is, however, not compatible with mysql < 8.0. + if ( version_compare( $wpdb->db_version(), '8.0', '>=' ) ) { + return $wpdb->get_col( + $wpdb->prepare( + " + WITH pages AS (SELECT ROW_NUMBER() OVER (ORDER BY %i.ID) AS n, %i.ID as first_id_on_page + FROM %i USE INDEX ( `type_status_date` ) + {$where} + AND %i.ID >= %d + ORDER BY %i.ID ASC) + SELECT first_id_on_page + FROM `pages` + WHERE MOD(n-1, %d) = 0 + ", + $wpdb->posts, + $wpdb->posts, + $wpdb->posts, + $wpdb->posts, + $starting_post_id, + $wpdb->posts, + $entries_per_page + ) + ); + } + /* * Optimized query per this thread: * {@link http://wordpress.org/support/topic/plugin-wordpress-seo-by-yoast-performance-suggestion}. @@ -566,6 +591,7 @@ protected function get_raw_page_data( $post_type, $entries_per_page, $starting_p o JOIN {$wpdb->posts} l ON l.ID = o.ID WHERE (@rownum:=@rownum+1) %% %d = 0 "; + // phpcs:disable WordPress.DB.DirectDatabaseQuery.NoCaching,WordPress.DB.DirectDatabaseQuery.DirectQuery -- Can't do this with WP queries. Caching is done on the sitemap level. return $wpdb->get_col( $wpdb->prepare( $sql, $starting_post_id, $entries_per_page ) ); // phpcs:enable From e21117ebbb4cd6a5dad52b021eaea3ae3c713e7c Mon Sep 17 00:00:00 2001 From: Diede Exterkate <5352634+diedexx@users.noreply.github.com> Date: Wed, 19 Jun 2024 14:15:18 +0200 Subject: [PATCH 34/35] Ensure that hidden posts will be in the sitemap as soon as they're no longer hidden e.g. when you publish a post as password protected and later remove the password, this change will ensure that the now public post will be added to the sitemap. --- inc/sitemaps/class-post-type-sitemap-provider.php | 10 ++++++++++ tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php | 6 ++++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/inc/sitemaps/class-post-type-sitemap-provider.php b/inc/sitemaps/class-post-type-sitemap-provider.php index 115d8b8071a..bd12fc29a42 100644 --- a/inc/sitemaps/class-post-type-sitemap-provider.php +++ b/inc/sitemaps/class-post-type-sitemap-provider.php @@ -795,6 +795,16 @@ private function create_map( $post_type, $max_entries_per_page, $min_post_id, $m $current_page_id = max( $starting_page, 1 ); $page_starting_ids = $this->get_raw_page_data( $post_type, $max_entries_per_page, $min_post_id ); + // If there's no data, we'll create a single page that would encompass all posts in case they would become visible in the sitemap. + if ( empty( $page_starting_ids ) ) { + return [ + $current_page_id => [ + 'start' => $min_post_id, + 'end' => $max_post_id, + ], + ]; + } + foreach ( $page_starting_ids as $index => $page_starting_id ) { $next_page_starting_id = isset( $page_starting_ids[ ( $index + 1 ) ] ) ? ( $page_starting_ids[ ( $index + 1 ) ] ) : null; $map[ $current_page_id ] = [ diff --git a/tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php b/tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php index 6d55aa80e9a..9ebe5a59308 100644 --- a/tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php +++ b/tests/WP/Sitemaps/Post_Type_Sitemap_Provider_Test.php @@ -487,8 +487,10 @@ public function test_only_password_protected_post_parent_attachments() { ] ); - $this->expectException( OutOfBoundsException::class ); - self::$class_instance->get_sitemap_links( 'attachment', 100, 1 ); + $actual_urls = \wp_list_pluck( self::$class_instance->get_sitemap_links( 'attachment', 100, 1 ), 'loc' ); + + // Expect the attachment not to be added to the list. + $this->assertCount( 0, $actual_urls ); } /** From 485dce7ada59da2bbdd0df519589cce1d7111df2 Mon Sep 17 00:00:00 2001 From: Diede Exterkate <5352634+diedexx@users.noreply.github.com> Date: Wed, 19 Jun 2024 14:18:44 +0200 Subject: [PATCH 35/35] Ensure the first post on a page can regain its spot on the same page after hiding and republishing e.g. when the first link/post of a sitemap page gets marked as private/password protected/no-index, the link gets hidden from the page. When you would then publish a new post that would end up on the same page, the password-protected post would get bumped off the page never to be seen again. This change ensures that that password protected page will keep its place and that it will re-appear on the first place of that page whenever the post is made public again. This does mean that that sitemap page _could_ go over its max_entries limit. This isn't an issue as long as the page doesn't exceed 50.000 links. --- inc/sitemaps/class-post-type-sitemap-provider.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/inc/sitemaps/class-post-type-sitemap-provider.php b/inc/sitemaps/class-post-type-sitemap-provider.php index bd12fc29a42..df948701978 100644 --- a/inc/sitemaps/class-post-type-sitemap-provider.php +++ b/inc/sitemaps/class-post-type-sitemap-provider.php @@ -808,8 +808,8 @@ private function create_map( $post_type, $max_entries_per_page, $min_post_id, $m foreach ( $page_starting_ids as $index => $page_starting_id ) { $next_page_starting_id = isset( $page_starting_ids[ ( $index + 1 ) ] ) ? ( $page_starting_ids[ ( $index + 1 ) ] ) : null; $map[ $current_page_id ] = [ - 'start' => (int) $page_starting_id, - 'end' => $next_page_starting_id ? ( $next_page_starting_id - 1 ) : $max_post_id, + 'start' => ( $current_page_id === $page_starting_id ) ? $min_post_id : (int) $page_starting_id, + 'end' => ( $next_page_starting_id ) ? ( $next_page_starting_id - 1 ) : $max_post_id, ]; ++$current_page_id; }