Terms Query: Fix check for selected taxonomy and current template.#72300
Terms Query: Fix check for selected taxonomy and current template.#72300ntsekouras wants to merge 3 commits intotrunkfrom
Conversation
ntsekouras
added
[Type] Bug
|
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. |
|
Size Change: +21 B (0%) Total Size: 2.13 MB
ℹ️ View Unchanged
|
| _taxonomyFromTemplate = | ||
| // `Tags` are a special case in WP template hierarchy. | ||
| _taxonomyFromTemplate === 'tag' | ||
| ? 'post_tag' | ||
| : _taxonomyFromTemplate; |
There was a problem hiding this comment.
I'm concerned that if someone creates a custom taxonomy with the slug "tag" that this would always switch to the built in post_tag.
I still think that there's an argument to be made for moving getQueryContextFromTemplate() out from the query block into a shared util: https://github.com/WordPress/gutenberg/pull/71916/files#diff-e1114d9d6c8644b6b5ebbb3ebf9965691070d0eb60d8597b58259e7997972ce5R8
You can see proposed usage of the modified util here: https://github.com/WordPress/gutenberg/pull/71916/files#diff-8cac49f7201ede824425efd643d4a8162d2a42ca6ab6fdcf3a6570c2f856d9a3R199
There was a problem hiding this comment.
I'm concerned that if someone creates a custom taxonomy with the slug "tag" that this would always switch to the built in post_tag.
Wouldn't then the _taxonomyFromTemplate be taxonomy-tag? 🤔
Not sure about the specific Query util (I'd have to check the code more) but a util like useArchiveLabel, useTemplateBasedTermData, etc.. probably makes sense. This should be done separately though.
There was a problem hiding this comment.
Wouldn't then the
_taxonomyFromTemplatebetaxonomy-tag?
No, it would just be "tag", see line 59.
There was a problem hiding this comment.
Hm.. You're right. That's a great catch and when we explore to extract in a util, we should take this into account, as the other functions do not.
|
Flaky tests detected in f434fb0. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/18486219897
|
t-hamano
left a comment
t-hamano
left a comment
There was a problem hiding this comment.
Question: Will this logic work correctly if the taxonomy itself contains hyphens?
Example: taxonomy-book-genre
Wow.. It seems not and the original code would not catch the cases we discussed here and need to be revisited. For now I'll simplify here with I'll test to remove this altogether here in a follow up, but we need to keep this in mind for the original implementation in the other places. |
|
Not being able to derive the exact taxonomy and term from the template slug is a wild situation to get ourselves into... while we can at least detect if the taxonomy is in the template slug, assuming from there that anything left over is the term, it's still a bit fraught. In the We could do a request for the derived "term" to find out if it exists, but the existence of a term isn't necessarily implied by the existence of a template. WordPress looks for a specific template based on the query and not the other way around. I don't think there is a good solution unless hyphens were banned from post type and term names. |
I share your thoughts! I'm wondering how common that would be in the real world applications. I don't think we had an issue in the repo for that, for other places we have that logic. Iit's a problem we need to solve eventually though. Having said that, for Terms Query block, we might not need to solve it right now if we land this PR:#72313 that simplifies the check without needing to know the taxonomy. |
@ntsekouras Oh, do you mean like just checking if the template slug starts with |
Yes.
You can already do that with registerBlockType filter. |
hi, I saw your comment at #49094 (comment) and just popping in here since we have some interest in implementing term query blocks. At my org; nearly all of our post types and taxonomies contain hyphens: we prefix our custom post types, with our org's prefix, (e.g. our location post type is named 'cpl-location') and some services agencies do this as well (10up immediately comes to mind) to avoid conflicts with other plugins. I was going to test this; but it seems unnecessary to because of #72313 is approved to be merged. |
|
@skorasaurus I would like to clarify that I think banning hyphens in taxonomy names is a non-starter, that was just the only option I could see for detecting the taxonomy cleanly from the template slug. I just don't think WordPress templates are set up for this, they are only used on output when the taxonomy and term id/slug is known ahead of time. |
|
I've been looking for a way to accurately parse the taxonomy and terms from the template slug, but it doesn't seem like there's a way to do this at the moment.... Perhaps we need to make a fix at a higher layer and provide the taxonomy and terms separately to the block as a context. That said, doesn't the current issue potentially exist in other blocks as well? Will this issue hinder us from stabilizing the Terms Query block? |
|
Thanks for the reviews everyone. Since we merged the |
4 participants
What?
Follow up of: #72295
I oversimplified in a wrong way with the check whether the selected taxonomy matches the current template.
This PR uses the same regex we have in other places like
useArchiveLabel, but with more comments.Testing Instructions
The inherit control and functionality from a template should work as expected.
--cc @cr0ybot