FSE: Fall back to next best template in hierarchy when querying through REST API #37258
Conversation
|
Size Change: -58 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
ockham
force-pushed
the
try/implicit-template-resolution-for-rest-api
branch
from
689d713
to
1b176c0
Compare
ockham
requested review from
youknowriad,
jeyip,
kevin940726,
Addison-Stavlo and
mcsf
| } | ||
|
|
||
| $block_template = get_block_file_template( $id, $template_type ); |
There was a problem hiding this comment.
Personally I feel the logic change here doesn't make sense, at least not from the gutenberg_get_block_template perspective, this is a low level API to retrieve a template given its ID. Potentially it can be moved to the endpoint to create a template but not while quering. WDYT
There was a problem hiding this comment.
Yeah, that's the big question here. I think that there's good arguments for each side:
- It is indeed a fairly low-level API, and it seems like when given a template ID, it should just return that template (or nothing, if it can't find it).
- OTOH, I think a point can be made that we've now identified one significant use case where that doesn't cut it, and we need the fallback. My hypothesis was that this could be the case pretty much anywhere where we'd call
gutenberg_get_block_template(or "require a template given its ID") -- not just for the endpoint -- since it replicates what happens on the frontend, and it's likely that we always want to replicate that.
I'll do a quick audit of all the other occurrences of gutenberg_get_block_template. If indeed I find that the fallback makes sense in all current cases, I'll probably opt to change it to that effect. (The argument then being YAGNI: If we currently only need it with the fallback, we should only provide it that way.)
Another option would be to add a bool arg to the function signature to indicate whether or not to look for fallback templates or not, but I'd only do that if we find that we really need both versions of the function already.
There was a problem hiding this comment.
I'll do a quick audit of all the other occurrences of
gutenberg_get_block_template.
|
Audit of existing
|
|
I'm not sure I agree with this reasoning personally, For me, the fallback doesn't really make sense in |
|
Ah, missed one:
|
Just to make sure we're on the same page here 🤔 -- having the fallback in We open the site editor to edit a template which doesn't exist yet, e.g. "Search". So instead, we load the fallback template -- "Index", in this case. We do so since that's also the template that the frontend falls back to, so it's reasonable for the user to assume that that's what they'd see when attempting to edit the corresponding template. Doing this at LMK if I'm reading you correctly and you'd rather solve this altogether differently 🙂 |
|
yeah, I think it's a difference in terms of user expectation that we need to clarify. For me, if I'm on a listing of templates or I want to open the site editor for a given template and that template doesn't exist, I don't really care about hierarchy, I'd like for the site editor to tell me that it doesn't exist but when I want to create a template I think it's fine to inherit the frontend behavior (hierarchy) |
I should point out the templates listing indeed doesn't include non-existent templates (since we're not changing the pluralized functions, i.e. |
I think that's where we should ask ourselves: do we want for this to be a template resolution instead of a template lookup? Don't we already have template resolution functions resolve_template and the like? Aren't there any use cases for simple template lookup. I fear we're breaking the abstraction personally by transforming to template resolution (a specific use-case) |
Yeah, this, to me, is the biggest question. What I was trying to say throughout our convo is that I couldn't really think of any 🤷♂️ |
|
@ockham Here's a quick usecase Basically I'm listing the available templates for a given theme, you could argue that you should pass the whole object to |
|
Not a bad example, but still not totally convinced 😬 On a practical level: We already have a template list implemented in our UI that's using the "pluralized" endpoint, and it's working well (even with this PR applied) -- no non-existent templates are listed:
Importantly, your example would be unaffected in most practical cases IMO: If the So the only case where this would be different are rather pathological queries whose responses would include non-existing templates; and once again, I'm struggling to come up with an example 😅 I get that we're bending the concept of IDs, and maybe both Gutenberg's entity concept, and WP's CPT endpoint concept quite a bit. My takeaway is that templates might have deserved a distinct concept (and endpoint) after all, but I guess it's a bit late for that now. Still, I'd like to make the relevant tools as ergonomic as possible. This, to me, means that our tools should replicate the frontend as much as possible (whereas we have more freedom for things that don't exist on the frontend -- such as a list of existing templates). As a thought experiment, I think that this PR solves #36648 much more cleanly and consistently (also in terms of mental models) than would be otherwise possible; we'd have to make changes in many more different places. I don't think that the "resolved" template query at this point deserves a totally distinct endpoint; if anything, then maybe a bool flag (whether or not to resolve/fall back). If we do go that way, I'd still like to make the resolution the default behavior 😬 |
|
First of all, I think all your arguments on both sides are pertinent, and I've been looking at this issue for a while trying to make up my mind. :) The more I re-read things, though, the more I lean towards this: The user feature is really compelling: when adding a missing template, it should be possible to bootstrap from the fallback template. That doesn't mean that the feature should be implicit. It might, but I can also imagine that the editor would ask the user whether to start from the fallback or start empty. I also quite like that, in contrast with its predecessor, this PR avoids unnecessary (and possibly harmful) saving upon starting a template. Regardless of whether the user-facing bits are implicit, I don't think the mechanics under the hood should. If one of the initial points of the PR was consistency (between front end and editor), then we shouldn't introduce (direct or indirect) inconsistencies elsewhere, which is what seems to be happening:
As for the internal semantics, I'd beware of changing Finally, I should ask this: aside from the |
ockham
changed the title
|
Thanks a lot for your feedback, Miguel! ❤️
Just to clarify, this wouldn't be on the caller though; what I meant is an extra check in the controller. I've now pushed one in 4b09790 -- that should be pretty much it.
I wasn't aware of this -- thanks for the pointer, I'll look into this!
I've struggled to come up with other use cases. My overall reading is: Templates are CRUD, and for the Create and Read parts, it seemed to me that we'd want to replicate the frontend for most conceivable use cases. A short digressionAnother way of looking at it is that we barely have any API (REST or even just PHP functions) at all ATM to "directly" query what would get rendered for a given route -- partly since it's hard to formulate a language for those queries (that takes into account not just simple entities such as a page with a given slug or a category with a given ID, but also post archives for a given month etc). Famously, this is why originally had the site editor simply query the actual route for the entity, with a magic?_wp_find_template querystring appended (see here and here).
I digress; the current solution obviously also only works for entities with an ID, so I don't claim it's the end-all. Anyway, one more-or-less trivial example I can think of is replicating the frontend over the REST API. I.e. you wanna render your template, but not straight from PHP, but something else that's connected to ("headless"?) WordPress/GB only through the REST API. (Aren't the Frontity folks looking into something like that? cc/ our liaison @gziolo plus @michalczaplinski who I still owe a reply on a P2 👋 ) |
I don't know the full context of this PR and how it's related to the work that the Frontity team is doing. At the moment the focus is to replicate the comments list with the Comments Query Loop block that contains inner blocks. The next project in the pipeline is to replicate the comment form with inner blocks, too. On top of that, the plan is to look at finding ways to replicate the frontend view with a JS code to bring interactivity without a full page refresh (like pagination, replying to the comment). However, the obvious challenge is where it is even possible to use REST API and JavaScript to mirror what PHP renders knowing that there are WP filters available on the server that might completely change how the comments are presented. |
|
Apologies for letting this stall for a bit. I'm happy to move the implicit template resolution into a new For the endpoint, I'll add a bool flag to the |
Done. Note that the default behavior is to use the fallback. I briefly tried the other way round, but that got messy quickly, since the calls to the endpoint are all over the place and not exactly easy to track down. |
noisysocks
added
the
Backport to WP Minor Release
|
👋 @ockham I noticed this PR is on the WordPress 5.9.x board. Do you this could still land until Wednesday to be included in WordPress 6.0 release? |
Hey @adamziel, thanks for reaching out! I'm afraid it's rather unlikely for this PR to get merged by Wednesday since there was some disagreement about some of the underlying choices 😕 |
|
Can we move forwards with this one? |
The basic idea of this PR -- using template resolution pretty much whenever a template was requested, even by ID -- was a bit controversial (it would return a fallback template if the one specified by the given ID didn't exist). I was asked to change the PR so that there would be two versions of template lookup by ID, one without, and one with fallback. However, in practice, I ran into a problem when trying to use the non-fallback version for a case where it was requested to be used (e.g. REST API
This means that the PR currently implements the suggested changes only half-way (where they probably don't make a lot of sense; it's really just cosmetics on top of the original PR). If people are okay with this state (or with rolling back those cosmetic changes) and give approval, I'm happy to merge. Otherwise, we should probably close this PR and either pursue #37054, or wait for someone to figure out if it's possible to implement the requested changes while retaining the underlying idea of this PR. |
|
There's a flow being created for adding slug specific templates that asks a few steps, it stands to reason that you should be able to:
We are not building all of this at once, but the option to do blank or existing template makes sense asap. cc @jameskoster |
This is now merged (#41189), which imo elevates the priority on this PR. It is especially strange to intentionally create a template for a specific entity only to be met with a totally blank canvas. |
Just connecting a dot here... a concept was shared to do virtually the same thing from the 'add template' flow in the post editor: #41060 (comment). We can probably do the same thing in the site editor, with some minor copy tweaks. Obviously out-of-scope for this PR, but I find it helps to try and keep these things connected :D |
|
Since this PR's branch is pretty badly outdated, I've decided to start fresh: #41848 |
adamziel
removed
the
Backport to WP Minor Release
Description
Purports to fix #36648, as an alternative approach to #37054. For more background, see #37054 (comment)
I think that this PR is in good enough shape for a first round of reviews. I'm especially curious if people think that there are any risks in changing the
gutenberg_get_block_templatefunction (which was supposed to return a template specified by its ID) to fall back to the "next best" template if the one queried isn't available. My personal impression is that that's fine, since that better represents how the template hierarchy works, but I might be missing some cases.Note that this doesn't yet support "full" template hierarchy fallbacks: We only currently fall back within a template type, e.g.
page-${slug}topage-${id}topage, and eventually to the catch-allindextemplate, but not between template types, e.g. frompagetosingular, or fromsingle-posttosingletosingular. I'd implement those as part of a separate PR if we agree on the direction.We also need to figure out if this should be backported to WP 5.9 or not.
How has this been tested?
Apply the following patch on top of this branch to ensure you're using GB's compat layer template resolution code (rather than Core's).
Patch
Ideally, give it some more smoke testing -- whatever you can think of makes sense: delete the template again; find a way to test it on the frontend; etc.
Screenshots
Before adding the 'search' template:
After adding the 'search' template:
Types of changes
Somewhere in between a bug fix and new feature I guess?