Terms Context: Handle custom taxonomies and consolidate logic#72549
Open
Terms Context: Handle custom taxonomies and consolidate logic#72549
Conversation
mikachan
added
[Type] Code Quality
|
Size Change: -29 B (0%) Total Size: 2.47 MB
ℹ️ View Unchanged
|
Contributor
|
Is the goal of this PR to also handle: #72378. Specifically for taxonomies there are some cases that weren't handled properly (check this PR and the different commits - they are not big). I skimmed through your PR and didn't see the regex that was used before. Does that mean that it also fixes the issue above and is not just about consolidating in a single util function? |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Member
Author
I started this before I saw this issue, but yes, I was planning on attempting to address that issue as well. I have something in progress that I'll commit soon that I'm hoping handles custom taxonomies better.
Currently, in this PR I've just refactored the regex into something more readable and explicit. I'll push what I've been working on over the last day soon. I hadn't pinged anyone here yet for review as I started addressing #72378 after I'd already created the PR 😄 |
mikachan
changed the title
|
Flaky tests detected in 38c8bbe. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/19176031987
|
Member
Author
|
@cr0ybot Tagging you for review as well, if you have time 🙇 |
cr0ybot
reviewed
Oct 30, 2025
Comment on lines
92
to
105
| if ( dashIndices.length > 0 ) { | ||
| // If there's only one dash, treat it as taxonomy. | ||
| // If there are multiple dashes, use the last one as the split point. | ||
| if ( dashIndices.length === 1 ) { | ||
| // Single dash | ||
| // e.g. taxonomy-product-category -> taxonomy: "product-category", term: null | ||
| return createContext( taxonomyPart ); | ||
| } | ||
|
|
||
| // Multiple dashes | ||
| // e.g. taxonomy-product-category-electronics -> taxonomy: "product-category", term: "electronics" | ||
| const lastDashIndex = dashIndices[ dashIndices.length - 1 ]; | ||
| const taxonomy = taxonomyPart.substring( 0, lastDashIndex ); | ||
| const termSlug = taxonomyPart.substring( lastDashIndex + 1 ); |
Contributor
There was a problem hiding this comment.
I don't think it would be the end of the world to push forward with this approach since it is new functionality, but this breaks down if the term itself includes a dash. For example: taxonomy-product-category-computer-parts would assume the taxonomy is product-category-computer and the term is parts.
I wonder if there would be a way to hint at the taxonomy, either by fetching all public taxonomies and comparing to the list or accepting a taxonomy slug as the "expected" slug. I think in most (all?) cases right now you must select the taxonomy you want to display data from in the various blocks, but it sure would be nice to be able to just inherit the taxonomy from context like this.
Edit: I see now there is a function for parsing with validation below. Should have read through everything first before commenting.
cr0ybot
reviewed
Oct 30, 2025
cr0ybot
reviewed
Oct 30, 2025
ntsekouras
reviewed
Nov 3, 2025
| * | ||
| * @return {string|null} The current template slug or null if not available. | ||
| */ | ||
| export function useTemplateSlug() { |
Contributor
There was a problem hiding this comment.
A small optimization would be to pass templateSlug from block context, if available and avoid the queries in that case.
Member
Author
There was a problem hiding this comment.
Could you explain a little bit more about how this would work? Do you mean that the templateSlug should be passed to useTemplateSlug (via useTermContext) where it's available? We'd need to add templateSlug to the usesContext attribute for each of these blocks. I think this is probably fine, just double checking before I try this out.
Contributor
There was a problem hiding this comment.
We'd need to add templateSlug to the usesContext attribute for each of these blocks.
That's what I had in mind, yes. It should be fine since they need this.
Member
Author
There was a problem hiding this comment.
Thanks! In b6b6d31, I've passed templateSlug from the block context of these blocks so it can be used in useTemplateSlug if available.
ntsekouras
reviewed
Nov 3, 2025
ntsekouras
reviewed
Nov 3, 2025
|
|
||
| // Check if this taxonomy exists | ||
| const taxonomyRecord = getTaxonomy( potentialTaxonomy ); | ||
| if ( taxonomyRecord ) { |
Contributor
There was a problem hiding this comment.
This fails when we have taxonomies like taxonomy-one and taxonomy-one-more. So if we're in a template for the second example it will match the first one because it also exists. It will fail later on though to fetch the terms etc.. as it will search for a term more as part of taxonomy-one.
Contributor
There was a problem hiding this comment.
@mikachan I want to say this is handled by the "parse with validation" functionality, can you clarify if that is correct?
Member
Author
There was a problem hiding this comment.
I think there was a case where this would fail if there were overlapping taxonomy slugs. I should have a fix for this in 3e6b61c, and I've added a couple of test cases as well.
ntsekouras
reviewed
Nov 3, 2025
ntsekouras
reviewed
Nov 3, 2025
| templateSlug?.startsWith( 'tag-' ); | ||
|
|
||
| // Auto-set inherit mode and taxonomy when we have a valid template context taxonomy. | ||
| useEffect( () => { |
Contributor
There was a problem hiding this comment.
I believe this should be explored separately from this PR. Personally I don't think we should have this logic in this block. We can fall in undo traps and make it impossible for such a template to have two instances of this block, if they want an inherit one and a custom one too.
Similar thing had been explored for Query Loop in the past and dismissed for the above reasons.
Member
Author
There was a problem hiding this comment.
That makes sense. I've removed inherit from the setQuery call in 2dd55ae. Is this enough to avoid the undo traps?
Contributor
There was a problem hiding this comment.
Still, I don't think we should have this auto set taxonomy, but we could discuss it separately. My main point is that we should focus on the consolidation here and not add extra block functionality - it's out of scope of this PR.
Member
Author
There was a problem hiding this comment.
Ah I see, in that case I've removed them in 202fc6a. I can open a separate PR.
ntsekouras
reviewed
Nov 7, 2025
| const taxonomyRecord = getTaxonomy( potentialTaxonomy ); | ||
| if ( taxonomyRecord ) { | ||
| return { | ||
| ...DEFAULT_CONTEXT, |
Contributor
There was a problem hiding this comment.
I don't think we need to spread DEFAULT_CONTEXT at all when returning values, do we? We could even delete it and return an empty object when we return just that, since all the values are falsy.
Member
Author
There was a problem hiding this comment.
Deleting it works well, thanks! Done in 6d7e447.
# Conflicts: # packages/block-library/src/term-name/edit.js
ntsekouras
reviewed
Nov 19, 2025
| * @param {string|null} [templateSlug] The template slug from block context (optional). | ||
| * @return {string|null} The current template slug or null if not available. | ||
| */ | ||
| export function useTemplateSlug( templateSlug = null ) { |
Contributor
There was a problem hiding this comment.
Nit: I guess we don't need to export this one, since we don't have tests and is used here. I haven't checked all the other exports, but there might be more similar cases.
ntsekouras
reviewed
Nov 19, 2025
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { store as coreStore } from '@wordpress/core-data'; |
Contributor
There was a problem hiding this comment.
Should we rename the file to match the main function that is useTermContext?
ntsekouras
reviewed
Nov 19, 2025
| } | ||
|
|
||
| if ( isAuthor ) { | ||
| archiveTypeLabel = 'Author'; |
Contributor
There was a problem hiding this comment.
Shouldn't we localize this with __()?
ntsekouras
reviewed
Nov 19, 2025
| }; | ||
| } | ||
|
|
||
| if ( taxonomyParts.length === 2 ) { |
Contributor
There was a problem hiding this comment.
Is there a case here that taxonomy-product-category could be:
taxonomy: product, term: category?
ntsekouras
reviewed
Nov 19, 2025
| } | ||
|
|
||
| // For non-taxonomy prefixed slugs, use parseTemplateSlug. | ||
| const parsedTemplateSlug = parseTemplateSlug( templateSlug ); |
Contributor
There was a problem hiding this comment.
I'm a bit confused here. It seems parseTemplateSlug is only called when the template doesn't startsWith( 'taxonomy-' ) and only in this function (parseTemplateSlugWithValidation).
In parseTemplateSlug though there is the same check startsWith( 'taxonomy-' ) with some logic. I guess we don't need that right?
Do we really need both parseTemplateSlug and parseTemplateSlugWithValidation functions? Can we consolidate into one parse function or there is a specific reason for having two similarly named functions?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
Closes #72378
This attempts to fix the above issue by correctly detecting the current taxonomy and also consolidates the logic from a couple of hooks that have similar functionality:
useArchiveLabeluseTermDescriptionuseTermNameuseTermCountWhy?
Allows these hooks to handle custom taxonomies, reduces duplication and makes this logic easier to maintain.
How?
Adds logic to handle custom taxonomies with a dash (e.g.
taxonomy-product,taxonomy-product-category), and custom taxonomies with the slugtag.Removed any duplicated logic and moved the rest of the functionality into a single file in
utils.Also refactors the way the taxonomy templates are matched by breaking up the old regex pattern (
/^(category|tag|taxonomy-([^-]+))$|^(((category|tag)|taxonomy-([^-]+))-(.+))$/) into something more readable and explicit.Testing Instructions
Please try the steps below with several different taxonomies: category, tag, custom taxonomy and custom taxonomy with "tag" in the slug.
For
useArchiveLabel:For
useTermDescription:For
useTermName:For
useTermCount: