Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the filter that disables indexable creation, cover also the creation when a post is created and updated #21335

Open
wants to merge 23 commits into
base: feature/plugin-fixes
Choose a base branch
from

Conversation

leonidasmi
Copy link
Contributor

@leonidasmi leonidasmi commented Apr 24, 2024

Warning

Not to be merged before consulting with QA

Note

Easiest to review commit-by-commit

Context

  • We want to ensure that using the Yoast\WP\SEO\should_index_indexables filter (or being on a non-production environment) disables totally the indexable creation. Without this fix, indexables were still getting created, eg. when posts/terms were updated .

Summary

This PR can be summarized in the following changelog entry:

  • Fixes a bug where unnecessary data was written in the Yoast database when on non-production sites (or when the relevant Yoast\WP\SEO\should_index_indexables filter was used to disable such a behavior)
  • [wordpress-seo enhancement] Removes a redundant database write query, when saving a post.

Relevant technical choices:

  • A redundant db write query was removed when a post is saved, because the indexable has already been saved coming into that function and the only thing the second write did was to update the updated_at column of the indexable (that had already been updated before coming into the function)
  • Decided to keep a couple of $indexable->save()s intact, in the indexable homepage watcher and the indexable static homepage watcher, because they are only triggered if there's an indexable already there, which then gets updated. The reasoning is that:
    • it doesn't create a new indexable
    • it actually makes sense that if there is an existing indexable, to update its values with the new stuff, to avoid having stale data, which might be a problem when/if the filter gets removed at some point.
    • for the indexable homepage watcher specifically, I had to some logic to make sure that the $indexable->save() does only updates and not creates.
  • Renamed a bunch of variables in primary term functionality, to indicate that they carry an indexable instance, to improve readability.

Test instructions

Test instructions for the acceptance test before the PR gets merged

This PR can be acceptance tested by following these steps:

Note: Wherever add_filter( 'Yoast\WP\SEO\should_index_indexables', '__return_false' ); is mentioned, it can also mean that you are on a non-production site instead (aka, have the WP_ENVIRONMENT_TYPE constant indicate a staging or local or development site).

For created/updated post (or term) (or user):

  • Have the add_filter( 'Yoast\WP\SEO\should_index_indexables', '__return_false' ); filter active somewhere on your site.
  • Have indexables cleaned out in your site.
  • Go to create or update a post (or a term) (or a user).
    • For the post, make sure that you assign it to a category
  • Confirm that you don't get any indexables in the wp_yoast_indexable, wp_yoast_indexable_hierarchy, wp_yoast_primary_term and wp_yoast_seo_links tables
  • Visit that post/term/user and again confirm that you don't get indexables in those tables.
  • Add a link in the post/term content, clean indexables and repeat the test (not applicable for users).
  • Create/update a page with a parent page, clean indexables and repeat the test (or create a category with a parent category) (not applicable to users).

For regression test:

  • Remove the filter.
  • Repeat the above tests twice (both for post and term and user). One with this PR/RC and one with main/released version.
  • Between switching to the other plugin version, clean indexables
  • Once you save the post/page/term/user, take note of the wp_yoast_indexable, wp_yoast_indexable_hierarchy, wp_yoast_primary_term and wp_yoast_seo_links tables.
  • Compare the tables between this PR/RC and the released version and confirm that they have the same data (aside from maybe ID differences and of course the object_last_modified/updated_at columns.
    Also:
  • Cleanup again the indexable tables
  • Visit the post/page/term/user in the frontend
  • Take note of the indexable tables
  • Switch to the released version, cleanup indexable tables and visit the post/page/term/user again.
  • Compare the tables between this PR/RC and the released version and confirm that they have the same data (aside from maybe ID differences and of course the object_last_modified/updated_at columns.
    Also:
  • Cleanup again the indexable tables
  • Run the SEOO
  • Take note of the indexable tables
  • Switch to the released version, cleanup indexable tables and run the SEOO again.
  • Compare the tables between this PR/RC and the released version and confirm that they have the same data (aside from maybe ID differences and of course the object_last_modified/updated_at columns.
    Also:
  • Cleanup again the indexable tables
  • Create a post and assign it to a category. Confirm you see an entry in the wp_yoast_primary_term table.
  • Remove the category from that post and save it again.
  • Now confirm that you see that entry in the wp_yoast_primary_term table removed.

For hierarchy builder:

  • Don't have the add_filter( 'Yoast\WP\SEO\should_index_indexables', '__return_false' ); filter active somewhere on your site.
  • Have indexables cleaned out in your site.
  • Go to create or update a post.
  • You'll have a bunch of entries in the wp_yoast_indexable table and one in the wp_yoast_indexable_hierarchy one. Manually delete all entries from the hierarchy table, via the DB tool of your choice.
  • Now, add the filter in your site and re-save the post
  • Confirm that you don't get any indexables in the wp_yoast_indexable_hierarchy table.
  • Visit that post in the frontend and again confirm that you don't get any indexables in the wp_yoast_indexable_hierarchy table.
  • Repeat the test, but this time with creating a term instead.

For the ancestor watcher:

  • Have the add_filter( 'Yoast\WP\SEO\should_index_indexables', '__return_false' ); filter active somewhere on your site.
  • Have indexables cleaned out in your site.
  • Create a page and then create a subpage.
  • Now, go and edit the parent page to change its slug to slug-parent-new.
  • Confirm that there's no entry created in the wp_yoast_indexable or the wp_yoast_indexable_hierarchy table.

For regression test:

  • Remove any filter and cleanup indexables
  • Create a page and then create a subpage. Lets say the slugs are slug-parent and slug-child
  • Check the indexable for the subpage. It should have a permalink link http://domain.com/slug-parent/slug-child
  • Now, go and edit the parent page to change its slug to slug-parent-new.
  • Confirm that the indexable for the subpage now has the updated permalink, to something like http://domain.com/slug-parent-new/slug-child.
  • Also confirm that the wp_yoast_indexable_hierarchy table has entries created both for the parent page and the children page (you can understand which entry is for what page, when you check the indexable_id column and compare it with the id column of the indexable table)
  • Also, reset indexable and try something more:
  • In settings->permalinks, select the custom structure and put /%category%/%postname%/ in the field
  • Create a category, with slug cat1
  • Go to a post and assign it to the above category
  • Edit the category and make the slug cat1-new
  • Go to the indexable table, find the post's indexable and confirm that the permalink now is the updated one, eg. http://domain.com/cat1-new/post-title

For the quick edit watcher:

  • Set the breadcrumbs of Yoast for posts, to use categories.
  • Have the add_filter( 'Yoast\WP\SEO\should_index_indexables', '__return_false' ); filter active somewhere on your site.
  • Create 2 categories for posts
  • Create a post and assign it to category A
  • Go to the posts overview and use "quick-edit" on the created post. Set the category to B and update
  • Confirm that there's no entry created in the wp_yoast_indexable or the wp_yoast_indexable_hierarchy table.

For regression test:

  • Repeat the above test but without the filter
  • Every time you change the category to the post via the "quick-edit", check the wp_yoast_indexable_hierarchy table
  • Find the entries for that post (which is by finding the indexable_id that is equal to the id of the post in the indexable table) and confirm that the ancestor_id is pointing to the right category (again by comparing the ancestor_id to the id in the indexable table)

For homepage:

  • Have the add_filter( 'Yoast\WP\SEO\should_index_indexables', '__return_false' ); filter active somewhere on your site.
  • Have indexables cleaned out in your site.
  • Change one of the following settings:
    • Title or meta description of the homepage via the Yoast SEO settings->Homepage->Search appearance
    • Title, description or image of the Yoast SEO settings->Homepage->Social media appearance
    • The Search engine visibility in Settings->Reading
    • The Tagline in Settings->General.
  • Confirm that you don't get any indexables in the wp_yoast_indexable table.
  • Repeat the test, but this time change the following setting:
    • the Your homepage displays setting in Settings->General to a static page, pick a page, save and then change the page and save again
    • confirm that you don't get any indexables in the wp_yoast_indexable table
  • Visit the homepage again and confirm that you don't get any indexables in the wp_yoast_indexable table.

For regression test:

  • Remove the filter.
  • Repeat the above tests twice. One with this PR/RC and one with main/released version.
  • Between switching to the other plugin version, clean indexables
  • Once you save the setting, take note of the wp_yoast_indexable table.
  • Compare the table between this PR/RC and the released version and confirm that they have the exact same data (aside from the created_at/updated_at columns.
  • Make sure you change all settings mentioned above, because the control different columns in the indexable row:
    • title and description are controlled by: Title or meta description of the homepage via the Yoast SEO settings->Homepage->Search appearance.
    • open_graph_title, open_graph_description and open_graph_image are controlled by: Title, description or image of the Yoast SEO settings->Homepage->Social media appearance
    • Nothing is controlled by the Search engine visibility in Settings->Reading, so you can ignore that one.
    • description is also controlled by the Tagline in Settings->General, but only if there's no meta description in Yoast SEO settings
  • Now, re-add the filter and with this PR/RC repeat the regression test above. You should confirm that the columns change their values as expected, depending on the setting you changed.
    • This is explained by the fact that if there is already an indexable for the homepage, we intend to update it even with the filter enabled.

For post type archives:

  • Have the add_filter( 'Yoast\WP\SEO\should_index_indexables', '__return_false' ); filter active somewhere on your site.
  • Have indexables cleaned out in your site.
  • Activate the book CPT via the test helper.
  • Change one of the following settings, in the Books Archives in Yoast SEO settings:
    • SEO title (controls the title column in the indexable)
    • Meta description (controls the description column in the indexable)
    • Show the archive for books in search results (controls the is_robots_noindex column in the indexable)
    • Breadcrumbs title (controls the breadcrumb_title column in the indexable)
  • Confirm that you don't get any indexables in the wp_yoast_indexable table.
  • Visit the book archive page (/my-books/) and confirm that you don't get any indexables in the wp_yoast_indexable table.

For regression test:

  • Remove the filter.
  • Repeat the above tests twice. One with this PR/RC and one with main/released version.
  • Between switching to the other plugin version, clean indexables
  • Once you save the setting, take note of the wp_yoast_indexable table.
  • Compare the table between this PR/RC and the released version and confirm that they have the exact same data (aside from the created_at/updated_at columns).
  • Make sure you change all settings mentioned above, because the control different columns in the indexable row, like mentioned above.

For the link builder:

  • Have the add_filter( 'Yoast\WP\SEO\should_index_indexables', '__return_false' ); filter active somewhere on your site.
  • Go to create a post (or a term) and make sure you add an internal link in the content. Save the post (or term)
  • Confirm that there's no entry created in the wp_yoast_seo_links table.

For regression test:

  • Remove the filter.
  • Repeat the above tests twice. One with this PR/RC and one with main/released version.
  • Between switching to the other plugin version, clean indexables
  • Once you save the post (or term), take note of the wp_yoast_seo_links table.
  • Compare the table between this PR/RC and the released version and confirm that they have the exact same data.

For AIOSEO import:

  • Have the add_filter( 'Yoast\WP\SEO\should_index_indexables', '__return_false' ); filter active somewhere on your site.
  • Have indexables cleaned out in your site.
  • Install the AIOSEO plugin, deactivate Yoast SEO and create a post
  • Re-activate Yoast SEO and go to Yoast SEO->Tools->Import and Export ->Import from other plugins
  • Select AIOSEO to import from and perform the import
  • Go to Yoast's indexable table and confirm that you dont have any indexables created/imported there.

For regression test:

  • Remove the filter.
  • Repeat the above tests twice. One with this PR/RC and one with main/released version.
  • Between switching to the other plugin version, clean indexables and options from the test helper.
  • Once you import, take note of the indexable that was imported (a way to understand which one is that, is to look for the object_id that's equal to the post_id of the post you created when Yoast was inactive).
  • Compare the indexable between this PR/RC and the released version and confirm that they have the exact same data (aside from the created_at/updated_at columns).

For linking actions:

  • Have the add_filter( 'Yoast\WP\SEO\should_index_indexables', '__return_false' ); filter active somewhere on your site.
  • Have indexables cleaned out in your site.
  • Add the following code in your site:
function run_link_action() {
    $link_action = YoastSEO()->classes->get( Yoast\WP\SEO\Actions\Indexing\Post_Link_Indexing_Action::class );
    $link_action->index();
}
add_action( 'shutdown', 'run_link_action' );
  • Load the homepage of the site
  • Go to Yoast's indexable table and confirm that you dont have any indexables created/imported there.
  • Repeat the same with this code instead:
function run_link_action() {
    $link_action = YoastSEO()->classes->get( Yoast\WP\SEO\Actions\Indexing\Term_Link_Indexing_Action::class );
    $link_action->index();
}
add_action( 'shutdown', 'run_link_action' );
  • Remember to remove the custom code, because it will affect other tests you might run

For regression test:

  • Have at least a post and a term with links to another post or term
  • Remove the filter and cleanup indexables
  • Run the SEOO
  • Confirm that you see in the wp_yoast_seo_links table all the links you have added in your posts/terms and that the indexable_id and target_indexable_id columns are populated properly. (If you're unsure of what you'd expect there, you can do the same test with trunk/production version and compare the wp_yoast_seo_links table after the SEOO.

Relevant test scenarios

  • Changes should be tested with the browser console open
  • Changes should be tested on different posts/pages/taxonomies/custom post types/custom taxonomies
  • Changes should be tested on different editors (Default Block/Gutenberg/Classic/Elementor/other)
  • Changes should be tested on different browsers
  • Changes should be tested on multisite

Test instructions for QA when the code is in the RC

  • QA should use the same steps as above.

Impact check

This PR affects the following parts of the plugin, which may require extra testing:

  • Non-creation of indexables via the SEOO or the background indexation, or the WP CLI command or when a post/term is visited in the frontend, when the filter is enabled.
  • Usage of the wpseo_should_save_indexable filter
    • For example, a user might want to have all indexables disabled, except the one for the homepage.
    • So, they might add these two filters together:
add_filter( 'Yoast\WP\SEO\should_index_indexables', '__return_false' );

add_filter( 'wpseo_should_save_indexable', function ( $intend_to_save, $indexable ) {
    if ( in_array( $indexable->object_type, [ 'post' ] ) ) {
        return true;
    }

    return false;
}, 10, 2 );
  • After adding those, go and create a post
  • Confirm that the indexable for the post was created.

UI changes

  • This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Other environments

  • This PR also affects Shopify. I have added a changelog entry starting with [shopify-seo], added test instructions for Shopify and attached the Shopify label to this PR.

Documentation

  • I have written documentation for this change. For example, comments in the Relevant technical choices, comments in the code, documentation on Confluence / shared Google Drive / Yoast developer portal, or other.

Quality assurance

  • I have tested this code to the best of my abilities.
  • During testing, I had activated all plugins that Yoast SEO provides integrations for.
  • I have added unit tests to verify the code works as intended.
  • If any part of the code is behind a feature flag, my test instructions also cover cases where the feature flag is switched off.
  • I have written this PR in accordance with my team's definition of done.
  • I have checked that the base branch is correctly set.

Innovation

  • No innovation project is applicable for this PR.
  • This PR falls under an innovation project. I have attached the innovation label.
  • I have added my hours to the WBSO document.

Fixes #20335

@leonidasmi leonidasmi force-pushed the 20335-yoastwpseoshould_index_indexables-filter-doesnt-disable-indexables branch from 9808746 to 51ef489 Compare April 29, 2024 11:51
@leonidasmi leonidasmi force-pushed the 20335-yoastwpseoshould_index_indexables-filter-doesnt-disable-indexables branch from 51ef489 to b46a7c0 Compare April 29, 2024 14:59
@coveralls
Copy link

coveralls commented Apr 29, 2024

Pull Request Test Coverage Report for Build 57158dc9f51a708c44a4fcd2369b403aaecd37ee

Details

  • 73 of 78 (93.59%) changed or added relevant lines in 15 files are covered.
  • 841 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.01%) to 52.644%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/actions/importing/aioseo/aioseo-posts-importing-action.php 0 1 0.0%
src/builders/indexable-link-builder.php 2 3 66.67%
src/integrations/watchers/indexable-home-page-watcher.php 6 7 85.71%
src/repositories/indexable-hierarchy-repository.php 4 5 80.0%
src/repositories/primary-term-repository.php 3 4 75.0%
Files with Coverage Reduction New Missed Lines %
src/generated/container.php 841 0.0%
Totals Coverage Status
Change from base Build c82b15818b2003a9e7244269952635fc221ad1bf: 0.01%
Covered Lines: 28346
Relevant Lines: 54444

💛 - Coveralls

@leonidasmi leonidasmi force-pushed the 20335-yoastwpseoshould_index_indexables-filter-doesnt-disable-indexables branch from 76154d2 to 611d85a Compare April 30, 2024 12:39
@leonidasmi leonidasmi changed the base branch from trunk to feature/plugin-fixes May 23, 2024 12:01
@leonidasmi leonidasmi changed the base branch from feature/plugin-fixes to trunk May 23, 2024 12:01
@leonidasmi leonidasmi marked this pull request as ready for review May 23, 2024 12:04
@leonidasmi leonidasmi changed the base branch from trunk to feature/plugin-fixes May 23, 2024 13:06
@leonidasmi leonidasmi changed the base branch from feature/plugin-fixes to trunk May 23, 2024 13:09
@leonidasmi leonidasmi changed the base branch from trunk to feature/plugin-fixes May 23, 2024 13:09
@leonidasmi leonidasmi added the changelog: bugfix Needs to be included in the 'Bugfixes' category in the changelog label May 23, 2024
Copy link
Contributor

@thijsoo thijsoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CR 🚧 Some small comments, and once I noticed that we were using the hierarchy with the var name $indexable, so we should probably make tripple sure this is not done elsewhere.

@leonidasmi
Copy link
Contributor Author

leonidasmi commented May 24, 2024

once I noticed that we were using the hierarchy with the var name $indexable, so we should probably make tripple sure this is not done elsewhere.

I'm actually not sure what that means, so let's talk if we need to do anything else, other than the above points :)

Copy link
Contributor

@thijsoo thijsoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CR + ACC 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: bugfix Needs to be included in the 'Bugfixes' category in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Yoast\WP\SEO\should_index_indexables filter doesn't disable indexables
3 participants