Add Indexable visibility columns #17757
Conversation
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
diedexx
added
the
changelog: non-user-facing
There was a problem hiding this comment.
When the
has_public_postsproperty is used, the code shows a deprecation warning and returns the value fornumber_of_publicly_viewable_postsposts 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.
| $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 ); |
There was a problem hiding this comment.
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 );
|
|
||
| $replacements = \array_merge( $post_statuses, [ $author_id ] ); | ||
| $replacements = \array_merge( $post_statuses, [ $author_id ], $post_types ); |
There was a problem hiding this comment.
Maybe a check if $post_types is array here, before merging it to the $replacement?
There was a problem hiding this comment.
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.
| $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; |
There was a problem hiding this comment.
This block is repeated 4 times throughout the builders.
Maybe create a separate function to avoid Code Duplication?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yeap, for sure it's ok 👍 :)
| @@ -18,13 +18,13 @@ class Indexable_Builder_Versions { | |||
| */ | |||
| protected $indexable_builder_versions_by_type = [ | |||
| 'date-archive' => self::DEFAULT_INDEXABLE_BUILDER_VERSION, | |||
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
You're right! I missed that one. Thanks for noticing!
…IX-22-number-of-posts
There was a problem hiding this comment.
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.
|
|
||
| $sql = " | ||
| SELECT MAX(p.post_modified_gmt) AS last_modified, MIN(p.post_date_gmt) AS published_at | ||
| SELECT |
There was a problem hiding this comment.
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
|
During acceptance together with @increddibelly we found a bug: |
|
@diedexx Is this still relevant, or can it be closed? :) |
|
Yes, this is still relevant for indexable sitemaps |
Context
Summary
This PR can be summarized in the following changelog entry:
number_of_publicly_viewable_postsandis_publicly_viewableis_publicandhas_public_postson the Indexable model.Relevant technical choices:
is_publicly_viewableto be accurate for term, post-type and post Indexables.NULLvalues as possible for the new columns. Sometimes we have to:is_publicinstead of removing. The model is exposed through our surfaces, so removing the is_public property is a BC break.is_publicis still actively being set when (re)indexing. But all related code has been deprecated.has_public_postsproperty is used, the code shows a deprecation warning and returns the value fornumber_of_publicly_viewable_postsposts instead. This should yield the same result.is_post_publicly_viewablehas been copied to the post-builder because it was introduced in WP 5.7 and we still support 5.6.set_dreprecated_propertyand marked it with@internalso 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
_yoast_indexables.Database migration
number_of_publicly_viewable_postsandis_publicly_viewable.is_publicly_viewableisNULLfor all indexables.number_of_publicly_viewable_postsis populated and has the correct value. It should be eitherNULL,0or1:0for 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)0for author indexables that authored no posts or only posts that are not published or have robots set tonoindexfor that post or are password protected.1for author indexables that authored at least one post that is published and has robots set toindexfor that post and isn't password protected.NULLfor author indexables that authored at posts that are published and have robots set todefaultfor that posttype and aren't password protected.NULLfor post, term and post-type archive indexables0for attachment posts that don't have a parent; the attachment isn't linked to a post.0for attachment posts that are linked to a post that is not published or have robots set tonoindexfor that post or is password protected.1for attachment posts that are linked to a post that is published and has robots set toindexfor that post and isn't password protected.NULLfor attachment posts that are linked to a post that is published and have robots set todefaultfor that posttype and isn't password protected.Posts
is_publicly_viewableis1is_publicly_viewableis0is_publicly_viewableis0number_of_public_postsis0for your new posts.Date archives
01System pages
is_publicly_viewableis set to1Home page
number_of_publicly_viewablevalue now matches the number of posts that is visible in your home page for not logged in users (use an incognito window)number_of_publicly_viewablegoes down and theobject_last_modifiedreflects the moment you made the post private.Authors
number_of_publicly_viewablevalue now matches the number of posts that is visible in your author archive for not logged in users (use an incognito window)number_of_publicly_viewablegoes down and theobject_last_modifiedreflects the moment you made the post private.Post types
number_of_publicly_viewablevalue now matches the number of posts that is visible in your post type archive for not logged in users (use an incognito window)number_of_publicly_viewablegoes down and theobject_last_modifiedreflects the moment you made the post private.Terms
number_of_publicly_viewableis 0.number_of_publicly_viewablevalue now matches the number of posts that is visible in your term archive for not logged in users (use an incognito window). The value is1.number_of_publicly_viewablegoes down and theobject_last_modifiedreflects the moment you made the post private.Reindexing
Test instructions for QA when the code is in the RC
Impact check
This PR affects the following parts of the plugin, which may require extra testing:
UI changes
Other environments
Documentation
Quality assurance
Fixes FIX-21
Fixes FIX-22
Related to