-
Notifications
You must be signed in to change notification settings - Fork 894
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
Add post type and taxonomy filters in the script data #21814
base: feature/dash-phase-1
Are you sure you want to change the base?
Add post type and taxonomy filters in the script data #21814
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Just some minor 🧅 suggestions and some WP decouple suggestions. Also missing any intergration/unit tests. But lets talk about the need for those at this stage.
src/general/application/content-types/content-types-repository.php
Outdated
Show resolved
Hide resolved
src/general/application/content-types/content-types-repository.php
Outdated
Show resolved
Hide resolved
src/general/application/taxonomy-filters/taxonomy-filters-repository.php
Outdated
Show resolved
Hide resolved
I was hoping you wouldnt notice that 😄 In all seriousness though, I wanted to nail the layout first, and after this round of CR, I think I can now add those (or add them in a separate subtask). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some tiny style thingies. The rest looks great!
/** | ||
* Returns the content types array. | ||
* | ||
* @return array<array<string,array<string, array<string, array<string, string|null>>>>> The content types array. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if @return array<Content_Types> makes more sense here 🤔 It is an array representation due to the ->to_array call. But for developer comprehension knowing that its a list of content types might make more sense.
* | ||
* @param string $name The name of the content type. | ||
* @param string $label The label of the content type. | ||
* @param Taxonomy $taxonomy The taxonomy that filters the content type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @param Taxonomy $taxonomy The taxonomy that filters the content type. | |
* @param Taxonomy|null $taxonomy The taxonomy that filters the content type. |
/** | ||
* Gets the taxonomy that filters the content type. | ||
* | ||
* @return string The taxonomy that filters the content type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @return string The taxonomy that filters the content type. | |
* @return Taxonomy|null The taxonomy that filters the content type. |
* | ||
* @return bool Whether the taxonomy in question is valid. | ||
*/ | ||
public function is_valid_taxonomy( $taxonomy, $content_type ): bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public function is_valid_taxonomy( $taxonomy, $content_type ): bool { | |
public function is_valid_taxonomy( $taxonomy, string $content_type ): bool { |
Context
Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
wpseo_{$content_type_name}_filtering_taxonomy
filter, that allows users to set the filtering taxonomy for each post typedoing_it_wrong
notice will be displayedPost_Type_Helper::get_indexable_post_types()
is used. Unfortunately, it's configured to retrieve only post type names and not object, which makes our code a bit more complex than needed. A@todo
was added along with a subtask, to take care of that outside of this PR.Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
Default post content type:
wpseoScriptData.contentTypes
in the console.category
taxonomy inside the post object:Default post content type with categories de-registered:
category
taxonomy from thepost
content type:post
object became:Added filter for products:
Added filter for products, but product category is de-registered:
product_cat
taxonomy from theproduct
content type:product
object became:Added filter via hook:
wpseo_{$content_type}_filtering_taxonomy
$content_type
with the post type you want to enable the filter for, for example for enabling themovie-genre
taxonomy for themovie
CPT:wpseoScriptData.contentTypes
:movie-genre
, youcategory
) ornot-taxonomy
) or, you'll get a
null
instead in thetaxonomy
attribute and aPHP notice:
Added filter via hook, for a taxonomy with custom REST API URL:
wpseo_{$content_type}_filtering_taxonomy
again by replacing the$content_type
with the post type you want to enable the filter for, in this example:wpseoScriptData.contentTypes
:Added category filter for custom post type:
Like described in the technical choices, if a CPT has the default WP
category
taxonomy associated with it, and there's no other hook involved, we usecategory
for filtering that CPT. To test that:Blog posts
CPT with thecategory
taxonomy associated with it with this code:wpseoScriptData.contentTypes
:category
taxonomy as the filter. So, addAlternative genre
the filtering taxonomy for Blog PostswpseoScriptData.contentTypes
:No public (aka, indexable) post types:
return ['movie','book','product','post','page','blog-post',]
covers ALL content typeswpseoScriptData.contentTypes
and confirm that it returns an empty arrayRelevant test scenarios
Test instructions for QA when the code is in the RC
QA can test this PR by following these steps:
Impact check
This PR affects the following parts of the plugin, which may require extra testing:
UI changes
Other environments
[shopify-seo]
, added test instructions for Shopify and attached theShopify
label to this PR.Documentation
Quality assurance
Innovation
innovation
label.Fixes https://github.com/Yoast/reserved-tasks/issues/320