Add Indexable visibility columns #17757

diedexx · 2021-12-02T16:02:04Z

Context

We lack an easy way to determine whether archives have posts while querying Indexables. We need to know this when we generate XML Sitemaps with Indexables or when altering the noindex value for empty archives (empty user and term archives are now soft-404s, which are set to noindex)

Summary

This PR can be summarized in the following changelog entry:

Add new columns to the Indexable model: number_of_publicly_viewable_posts and is_publicly_viewable
Deprecate the use of is_public and has_public_posts on the Indexable model.

Relevant technical choices:

The return value of the adapter's select_one function is cast, because it returned an stdClass while all surrounding code depends on it being an array. (it wasn't used before)
The version number of most indexable builders has been increased to ensure that all relevant Indexables will be rebuilt and that they will receive values for the new columns.
I don't watch for changes in visibility for posttypes and taxonomies as there are no hooks for this and rarely happens. If this happens, a reindex is required for the is_publicly_viewable to be accurate for term, post-type and post Indexables.
I use as little NULL values as possible for the new columns. Sometimes we have to:
- The date-archive indexable can't have a reasonable value for number_of_publicly_viewable_posts, as the number of posts varies based on which date query is used.
- The author is_publicly_viewable changes depending on a global Yoast SEO setting (which disables author pages). If we were to include that in author indexables, we'd have to rebuild all authors when that setting is changed which may is a huge performance issue for larger sites.
I started including password protected posts from the aggregate queries that are used for getting the object_last_modified and object_published_at. The reason being that password protected posts do show up in the archives and do get indexed just like any other post.
I deprecate is_public instead of removing. The model is exposed through our surfaces, so removing the is_public property is a BC break. is_public is still actively being set when (re)indexing. But all related code has been deprecated.
When the has_public_posts property is used, the code shows a deprecation warning and returns the value for number_of_publicly_viewable_posts posts instead. This should yield the same result.
Add a new function in the Indexable_Repository: query_where_noindex. This is meant to be used for sitemaps later.
is_post_publicly_viewable has been copied to the post-builder because it was introduced in WP 5.7 and we still support 5.6.
I've introducedset_dreprecated_property and marked it with @internal so we can keep deprecated properties up to date during the deprecation period without throwing any deprecation warnings.

Test instructions

Test instructions for the acceptance test before the PR gets merged

This PR can be acceptance tested by following these steps:

Prep

Login as a user who can author posts (author, editor or administrator).
Use a database client like TablePlus, SequelPro or MySQL Workbench to connect to the database.
Open the table that ends with _yoast_indexables.
Keep it open; you'll be referencing this a lot during testing.

Database migration

Check if the indexables table now has two new columns: number_of_publicly_viewable_posts and is_publicly_viewable.
Check that is_publicly_viewable is NULL for all indexables.
Check that number_of_publicly_viewable_posts is populated and has the correct value. It should be either NULL, 0 or 1:
- 0 for the home page and system-page indexables (OR the acutal number of publicly viewable posts in the home page. This value is updated rather quickly)
- 0 for author indexables that authored no posts or only posts that are not published or have robots set to noindex for that post or are password protected.
- 1 for author indexables that authored at least one post that is published and has robots set to index for that post and isn't password protected.
- NULL for author indexables that authored at posts that are published and have robots set to default for that posttype and aren't password protected.
- NULL for post, term and post-type archive indexables
- 0 for attachment posts that don't have a parent; the attachment isn't linked to a post.
- 0 for attachment posts that are linked to a post that is not published or have robots set to noindex for that post or is password protected.
- 1 for attachment posts that are linked to a post that is published and has robots set to index for that post and isn't password protected.
- NULL for attachment posts that are linked to a post that is published and have robots set to default for that posttype and isn't password protected.

Posts

Publish a new post.
in the database, find the post indexable row.
Verify that the is_publicly_viewable is 1
Make the post private.
Verify that the is_publicly_viewable is 0
Find a posttype that doesn't isn't accessible from the frontend (e.g. WooCommerce orders). You can create your own with a plugin like CPT UI by selecting "false" for the "public" value.
Publish a new post of that private posttype.
Verify that the is_publicly_viewable is 0
Verify that number_of_public_posts is 0 for your new posts.

Date archives

Go to the Search appearance settings > Archives
Disable date archves and save
In the database, find the date-archive indexable row.
see that the is_publicly_viewable is set to 0
Reinable the date archives and see that the value is back to 1

System pages

Go to the Search appearance settings > Archives
Under "Special pages", adjust the titles for the 404 page.
In the database, find the system-page indexable row.
see that is_publicly_viewable is set to 1

Home page

Go to wp-admin/options-reading.php (Settings > Reading) and select "Your latest posts" next to "Your homepage displays" if that hasn't been the case already.
Publish a new post
in the database, find the home-page indexable row.
Verify that the number_of_publicly_viewable value now matches the number of posts that is visible in your home page for not logged in users (use an incognito window)
Make that post private
Verify that the number_of_publicly_viewable goes down and the object_last_modified reflects the moment you made the post private.

Authors

Publish a new post.
in the database, find the user you are logged in to.
Verify that the number_of_publicly_viewable value now matches the number of posts that is visible in your author archive for not logged in users (use an incognito window)
Make that post private
Verify that the number_of_publicly_viewable goes down and the object_last_modified reflects the moment you made the post private.

Post types

Publish a new post
in the database, find the post-type-archive indexable row.
Verify that the number_of_publicly_viewable value now matches the number of posts that is visible in your post type archive for not logged in users (use an incognito window)
Make that post private
Verify that the number_of_publicly_viewable goes down and the object_last_modified reflects the moment you made the post private.

Terms

Create a new category
in the database, find the term indexable row.
Verify that the number_of_publicly_viewable is 0.
Add your new category to a published post.
Verify that the number_of_publicly_viewable value now matches the number of posts that is visible in your term archive for not logged in users (use an incognito window). The value is 1.
Make the post private.
Verify that the number_of_publicly_viewable goes down and the object_last_modified reflects the moment you made the post private.

Reindexing

In the SEO > Tools admin screen, hit "Start SEO data optimization".
In the database, all indexables should be updated and match the same conditions as the test cases above.

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:

N/A - covered in test instructions.

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 other environments and needs to be tested there.

Documentation

I have written documentation for this change.

Quality assurance

I have tested this code to the best of my abilities
I have added unittests 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.

Fixes FIX-21
Fixes FIX-22

Related to

This caused issues with the rename_column, as column_info expects an array

If a replacement is given the ORM uses the replacement automatically.

…o number_of_publicly_viewable_posts

is_public is replaced by a different approach in querying data and by is_publicly_viewable. If we would immediately replace is_public with is_publicly_viewable, that would be a BC break.

…icly_viewable value

Password protected posts still end up in the post-type- and termarchives and are publicly queryable. The difference is that the content is password protected. Because if this detail, these posts should be noindexed, but the archives should still update their data because the protected posts are still shown there

This prevents unsetting the last modified date on archive indexables when the last post of the post-type or taxonomy is removed. Also prevents moving the date back in time when performing a reindex action.

…IX-22-number-of-posts

leonidasmi

When the has_public_posts property is used, the code shows a deprecation warning and returns the value for number_of_publicly_viewable_posts posts instead. This should yield the same result.

If that's what we want, then it's not currently happening, since has_public_posts was a boolean value and number_of_publicly_viewable_posts is an integer.

Also, if we want a match between the old has_public_posts value and the new number_of_publicly_viewable_posts one, I can imagine a case where these won't match and that is when we add images/attachments in a post.

Doing so, we will create an indexable for that attachment with number_of_publicly_viewable_posts = 0 as per the new code
but if I read the old code right, we would create the indexable for that attachment with has_public_posts = 1 as long as that post where the attachment was added is public.

leonidasmi · 2021-12-06T13:11:10Z

src/builders/indexable-post-builder.php

+		$post_type   = get_post_type( $post );
+		$post_status = get_post_status( $post );
+
+		return is_post_type_viewable( $post_type ) && $this->is_post_status_viewable( $post_status );


I would prefix those functions with \ likeso:

$post_type = \get_post_type( $post ); $post_status = \get_post_status( $post ); return \is_post_type_viewable( $post_type ) && $this->is_post_status_viewable( $post_status );

leonidasmi · 2021-12-06T13:28:12Z

src/builders/indexable-author-builder.php

+		';

-		$replacements = \array_merge( $post_statuses, [ $author_id ] );
+		$replacements = \array_merge( $post_statuses, [ $author_id ], $post_types );


Maybe a check if $post_types is array here, before merging it to the $replacement?

Yes! that could've been a nasty bug. I added validation higher up in the get_author_post_types so we're sure that we get an array.

leonidasmi · 2021-12-06T13:59:36Z

src/builders/indexable-author-builder.php

+		$indexable->object_published_at               = $aggregates->first_published_at;
+		$indexable->object_last_modified              = max( $indexable->object_last_modified, $aggregates->most_recent_last_modified );
+		$indexable->number_of_publicly_viewable_posts = $aggregates->number_of_public_posts;


This block is repeated 4 times throughout the builders.

Maybe create a separate function to avoid Code Duplication?

I did consider it, but decided to not change more than I had to. I want to better abstract this when we revise the builders entirely. Do you think that's ok?

Yeap, for sure it's ok 👍 :)

leonidasmi · 2021-12-06T14:50:03Z

src/values/indexables/indexable-builder-versions.php

 	 * @var array
 	 */
 	protected $indexable_builder_versions_by_type = [
 		'date-archive'      => self::DEFAULT_INDEXABLE_BUILDER_VERSION,


I'm probably missing something here, but why don't we up the builder version for dates?

My thinking is that since we add the is_publicly_viewable property, we should up this too, but maybe I don't get it :)

You're right! I missed that one. Thanks for noticing!

… properties

…IX-22-number-of-posts

leonidasmi

CR: 👍

With these new changes, we also take care of this remark, except the last part about attachments' has_public_posts BC break, but as per our conversation with @diedexx that's ok.

lib/model.php

increddibelly · 2021-12-17T08:27:36Z

src/builders/indexable-author-builder.php


 		$sql = "
-			SELECT MAX(p.post_modified_gmt) AS last_modified, MIN(p.post_date_gmt) AS published_at
+			SELECT 


idea: how about getting post ID's for an author first and iterating over those - using primary keys seems more efficient than where-ing over a set of fields.

If we use the same version number for two separate model changes in two separate releases, the later release won't receive automatic model updates because the version hasn't changed as of the previous release. Adding a comment of the version number, will cause a git merge conflict, which forces you to look at what the correct version number should be.

…IX-22-number-of-posts

diedexx · 2021-12-20T08:54:18Z

During acceptance together with @increddibelly we found a bug:
When you remove a term from a post, the number_of_publicly_viewable_posts of that term doesn't get updated.

…mber-of-posts

IreneStr · 2022-09-23T08:27:00Z

@diedexx Is this still relevant, or can it be closed? :)

diedexx · 2022-09-23T09:00:13Z

Yes, this is still relevant for indexable sitemaps

github-actions · 2024-12-24T02:55:47Z

A merge conflict has been detected for the proposed code changes in this PR. Please resolve the conflict by either rebasing the PR or merging in changes from the base branch.

diedexx added 28 commits November 12, 2021 13:38

Ensure array return type

b3fd2cd

This caused issues with the rename_column, as column_info expects an array

Allow for deprecating columns

87c11c3

If a replacement is given the ORM uses the replacement automatically.

Add method for creating a noindex query for indexables

2eef6e2

Rename is_public to is_publicly_viewable and number_of_public_posts t…

b5bc8db

…o number_of_publicly_viewable_posts

Use PHP 5.6 syntax

6c015ba

Add migrations

3d96968

Deprecate is_public instead of immediately replacing it

acdf97a

is_public is replaced by a different approach in querying data and by is_publicly_viewable. If we would immediately replace is_public with is_publicly_viewable, that would be a BC break.

Keep the method name the same and deprecate

e18c9ac

Access properties that were renamed earlier

58d6a71

Make the homepage get a number_of_publicly_viewable_posts and is_publ…

b7cfa04

…icly_viewable value

Use new column name

c2d121e

Add deprecation tags

38b552d

Fix cs

2fdce2f

Mark enabled date archives as publicly viewable

3a18f72

Delegate logic to central helper

5e89640

Make global namespace usage explicit

0a38f49

Flip argument order

81e7421

Allow for considering empty archives as indexed

1564f53

Update deperatation based on the expected release version

ecae28e

Prevent new integrators from accessing is_public

ea0a46e

Update tests

6ccacd5

Remove resolved todo comment

ccf7f9b

Rebuild date archive indexable when disabling date archives

af7b219

Fix cs

510f9de

Prevent deprecation warnings when (re)indexing

ed46abb

Keep the most recent last modified date.

ca85a07

This prevents unsetting the last modified date on archive indexables when the last post of the post-type or taxonomy is removed. Also prevents moving the date back in time when performing a reindex action.

Merge branch 'trunk' of https://github.com/Yoast/wordpress-seo into F…

faf1180

…IX-22-number-of-posts

diedexx added the changelog: non-user-facing Needs to be included in the 'Non-userfacing' category in the changelog label Dec 2, 2021

diedexx added this to the 17.9 milestone Dec 2, 2021

leonidasmi reviewed Dec 6, 2021

View reviewed changes

leonidasmi requested changes Dec 6, 2021

View reviewed changes

diedexx added 7 commits December 7, 2021 11:50

Make global namespace explicit

9eb9263

Ensure that post_types is an array

0c9ac1a

Automatically update date-archive indexables

2059186

Use the type of the originally requested property in case of replaced…

b95492e

… properties

Merge branch 'trunk' of https://github.com/Yoast/wordpress-seo into F…

101edcf

…IX-22-number-of-posts

Fix CS warnings and errors

5d095a5

Fix tests

d8f754f

diedexx requested a review from leonidasmi December 10, 2021 13:31

leonidasmi approved these changes Dec 13, 2021

View reviewed changes

increddibelly reviewed Dec 17, 2021

View reviewed changes

lib/model.php Outdated Show resolved Hide resolved

Clarify that the comment is about a function

f613dfc

increddibelly reviewed Dec 17, 2021

View reviewed changes

diedexx added 5 commits December 17, 2021 09:43

Add instructions to remove related code

3e7e755

Update post relation indexables when a post is set to private

4ed8b38

Fix cs

275c423

Fix precedence issues in query

08c1f5f

diedexx mentioned this pull request Dec 20, 2021

Build XML Sitemaps with indexables #17869

Closed

7 tasks

diedexx added 2 commits December 20, 2021 09:30

Merge branch 'trunk' of https://github.com/Yoast/wordpress-seo into F…

e1c5055

…IX-22-number-of-posts

Increase threhshold

6cf15bf

Exclude password protected posts from the publicly_viewable_count

bad2929

diedexx removed this from the 17.9 milestone Dec 21, 2021

Merge branch 'trunk' of github.com:Yoast/wordpress-seo into FIX-22-nu…

148d47c

…mber-of-posts

github-actions bot added the merge conflict label Dec 24, 2024

Add Indexable visibility columns #17757

Are you sure you want to change the base?

Add Indexable visibility columns #17757

Conversation

diedexx commented Dec 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Summary

Relevant technical choices:

Test instructions

Test instructions for the acceptance test before the PR gets merged

Prep

Database migration

Posts

Date archives

System pages

Home page

Authors

Post types

Terms

Reindexing

Test instructions for QA when the code is in the RC

Impact check

UI changes

Other environments

Documentation

Quality assurance

Related to

Uh oh!

leonidasmi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leonidasmi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diedexx commented Dec 20, 2021

Uh oh!

IreneStr commented Sep 23, 2022

Uh oh!

diedexx commented Sep 23, 2022

Uh oh!

github-actions bot commented Dec 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

diedexx commented Dec 2, 2021 •

edited

Loading

leonidasmi left a comment •

edited

Loading