Skip to content
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

FSE: Pre-populate template with fallback upon creation #42007

Closed

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Jun 28, 2022

What?

Alternative to #41848, based on some ideas shared by @ntsekouras in DM.

Purports to fix #36648, as an alternative approach to #37054 and #41848. For more background, see #37054 (comment)

(More description to come)

Why?

See e.g. #37258 (comment) and #37258 (comment).

How?

By using the "next best" template per template hierarchy resolution to pre-populate a newly created template's content.

Testing Instructions

This testing routine will verify a number of things:

  • That if a new template is created, it will pre-populate the editor with the "next best", less specific, fallback template.
    • This can be either a manually created template, or a theme-supplied one.
  • That the template titles and descriptions are correct.

First, we create a "Single Post" template and verify that it comes pre-populated with the contents of the theme-supplied "Single" template:

  1. Enable the Twenty Twenty-Two theme.
  2. Go to the Site Editor.
  3. Click the "W" logo menu in the top left corner.
  4. In the sidebar that opens, select "Templates".
  5. Click the "Add New" button in the top right corner.
  6. In the dropdown that opens, click on the "Single item: Post" template.
  7. In the modal that opens, click on "All Posts -- For all items".
  8. Verify that the site editor opens, pre-populated with the generic "single" template. (This is to ensure parity with the frontend, which would also use the "single" template as a fallback when there's no dedicated single-post template.)
  9. Verify that the title and description are still as prior to this PR ("Single item: Post" -- "Displays a single item: Post.")
  10. Make a change to the template. Make sure it's not to the header or footer template parts, nor to the post content. Ideally, insert a Paragraph block with some text right above the footer (and below the content). (Verify that it's a direct child of the template by checking in the bottom breadcrumbs bar that it shows up as "Template > Paragraph".)
  11. Save the template.
  12. Once saved, navigate back to the "Templates" screen (via the top left "W" button that opens the sidebar).
  13. Verify that there's now a "Single item: Post" template at the top of the list of the theme's templates.
  14. Verify that it's distinct from the "Single" template at the bottom of the list.

Second, we create a "Single Post" template for a specific post and verify that it comes pre-populated with the generic "Single Post" template that we just created: This is still buggy, reload every time you go back to the template list

  1. In the template list, click the "Add New" button in the top right corner.
  2. Since you've already created the generic "Single item: Post" template, the editor will directly open a modal to ask you for which post you'd like to create a template.
  3. Select a post from that list, e.g. the standard WordPress "Hello World" post.
  4. Verify that the site editor opens, with the "Single item: Post" template contents loaded that you just created. (This is again to reflect parity with the frontend).
  5. Verify that the template content has the change that you just made earlier to the "Single item: Post" template.
  6. Verify that the title and description are still as prior to this PR ("Post: Hello world!" -- "Template for Post: Hello world!").
  7. Make another change to the template, ideally to the part that you changed to distinguish the "Single item: Post" template.
  8. Save the template.
  9. Once saved, navigate back to the "Templates" screen (via the top left 'W' button that opens the sidebar).
  10. Verify that there's now a "Post: Hello World!" template at the top of the list of the theme's templates.
  11. Verify that it's distinct from the "Single item: Post" template right below.

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.

@ntsekouras
Copy link
Contributor

Thanks for exploring this Bernie and for all your efforts to solve this problem! From my initial tests this seems to work really well!

For context this is part of the DM I sent to Bernie:

I'm more inclined to a solution that would involve a new separate REST endpoint for that. Or even better if it works, to have the logic in the create_item endpoint and if the content is empty fill it with next best available template.

Even though I suggested the modification of create_item, I feel that having a completely separate endpoint for fetching the content would be better. This endpoint would be called before the creation of the template and the logic of gutenberg_get_template_slug is simple enough to be implemented client side. When this endpoint is resolved and the content is fetched, it will be passed in the creation here. I think this is better because we leave the basic endpoints intact and if someone for whatever reason needs to create empty templates through REST API, should be able to do so without any surprises. What do you think? -cc @TimothyBJacobs

@ockham
Copy link
Contributor Author

ockham commented Jun 29, 2022

TBH I’d rather avoid introducing a new endpoint. The functional overlap with the current one would be fairly large, so I’d be wary of fragmenting the REST API surface for templates too much. Since both would allow reading the template content for a given slug, it might be confusing for developers to know what each endpoint is for, let alone to reason about where any changes should go to.

This endpoint would be called before the creation of the template and the logic of gutenberg_get_template_slug is simple enough to be implemented client side. 

It’s not complete yet, though: It doesn’t yet support the full template hierarchy (https://wphierarchy.com/); among other things, the fallback from frontend to home template is missing (which is currently implemented elsewhere https://github.com/WordPress/wordpress-develop/blob/ae9cff730f54f2401ce3271477b1ba63fed81f83/src/wp-includes/block-template.php#L335-L365 but would be good to absorb into the logic here).

IMO, extending gutenberg_get_template_slug can be done iteratively (and with proper test coverage) to account for more and more exotic cases. Now that could of course also apply to the client side, but I still feel that we’d fragment the template hierarchy resolution logic. After all, template lookup is still performed server-side.

I think this is better because we leave the basic endpoints intact and if someone for whatever reason needs to create empty templates through REST API, should be able to do so without any surprises.

One reason why I think that using the fallback to create a new template (vs. starting blank) makes sense is the following:

  • If you start blank (and don’t modify the template), and on the frontend load a page or post that uses that template, you get a blank page 😬
  • If you start with the fallback content (and don’t modify the template) and load a page or post on the frontend that uses that template, you get the fallback — which is exactly what you would’ve gotten before adding the new template.

@ockham
Copy link
Contributor Author

ockham commented Jun 29, 2022

@ntsekouras and I had a call earlier today. While neither of us managed to convince the other as to whether modifying the existing endpoint’s behavior or adding a new endpoint is the better solution 😬 (see above discussion), we decided it made sense to open this PR for review already, in order to solicit others’ feedback.

Some notes:

The full template hierarchy isn’t covered by this PR yet, but we agreed it would be easy enough to extend it iteratively (and with proper test coverage) to account for e.g. the frontpage -> home fallback, and more.

Further note that as of now, all templates eventually fall back to the index template. While this is consistent with the template hierarchy (as implemented on the frontend), it was brought up that this can be counter-intuitive in some cases, e.g. for 404 or single templates. I’m happy to add logic for these special cases. My question would be what should be the fallback there? Just start blank?

Finally, as an alternative to adding a new endpoint, I suggested adding a field to indicate using fallback mode during template creation (akin to this).

Curious to hear y’all’s thoughts!

@ockham ockham marked this pull request as ready for review June 29, 2022 14:48
@ockham ockham requested a review from mtias June 29, 2022 14:48
@TimothyBJacobs
Copy link
Member

I think this is better because we leave the basic endpoints intact and if someone for whatever reason needs to create empty templates through REST API, should be able to do so without any surprises. What do you think? -cc @TimothyBJacobs

I personally agree, I prefer less "magical" behavior in REST API endpoints.

After all, template lookup is still performed server-side.

Yes, I don't think the client would be the one to follow the template hierarchy, IMO that would still be done server side. So I would see a REST API endpoint that could be used to resolve an arbitrary template hierarchy. Then the client would grab the content for the matched post, and pass it to the existing create endpoint.

Those are my initial thoughts, but I don't think I have a full understanding of what is in play here. So let me know if I'm missing something.

@jameskoster
Copy link
Contributor

I tested the flows outlined in the OP plus a few more and everything seems to be working as expected! :)

it was #41848 (comment) that this can be counter-intuitive in some cases, e.g. for 404 or single templates. I’m happy to add logic for these special cases. My question would be what should be the fallback there? Just start blank?

For single templates perhaps core can provide a minimal fallback that consists of something like:

<!-- wp:template-part {"area":"header"} /-->

<!-- wp:post-title /-->

<!-- wp:post-content /-->

<!-- wp:template-part {"area":"footer"} /-->

Obviously this should only be used after looping through all available templates and finding no singular template.

For 404 we can do something similar:

<!-- wp:template-part {"area":"header"} /-->

<!-- wp:heading -->
<h2>404</h2>
<!-- /wp:heading -->

<!-- wp:paragraph -->
<p>Page not found.</p>
<!-- /wp:paragraph -->

<!-- wp:template-part {"area":"footer"} /-->

What do you think?

@ockham
Copy link
Contributor Author

ockham commented Jul 8, 2022

@jameskoster I think that makes sense! In practice, I'd probably implement it by including those templates as files with GB (and -- once backported -- with Core). This would mark a bit of a departure from how themes used to work in absence of a given template, but that's probably okay.

@ockham
Copy link
Contributor Author

ockham commented Jul 8, 2022

Thanks @TimothyBJacobs for your thoughts!

Yes, I don't think the client would be the one to follow the template hierarchy, IMO that would still be done server side. So I would see a REST API endpoint that could be used to resolve an arbitrary template hierarchy. Then the client would grab the content for the matched post, and pass it to the existing create endpoint.

It's maybe relevant to point out that template resolution can mean two different things:

  • Originally, it was basically just WordPress' way of routing, i.e. mapping a URL such as example.com/category/mycat to a template file that's in charge of rendering that URL. It would do so by checking for the existence of progressively less specific templates (in this case, starting at category-mycat and continuing via category-123 to category, and eventually index). We're all well aware of this, but now for the "new" meaning:
  • When we create a new template from the Site Editor, the user has already stated their intent which template that should be. We don't ask them to specify the URL; instead, they state something like "A generic category template" or "A specific category template for the mycat category". This is, among other things, readily translated to the slug format that's also used to identify template files (here: category-mycat). What remains to be done is checking for the existence of that template, and if it's not there, return the "next best" match according to the template hierarchy. We can do so by either providing that template's content (which is what this PR currently does), or its slug.

Personally, I don't like the idea too much to have:

  1. one endpoint solely for that latter template resolution based on a slug (something like GET /wp/v2/template-resolution/category-mycat -- which returns the next-best existing fallback template, e.g. category)
  2. whose response we then use to query the existing templates endpoint (GET /wp/v2/templates/category) to get the fallback template's content
  3. which we then use to pre-populate the new template (POST /wp/v2/templates/category-mycat).

It's two more network roundtrips than with this PR, and I'm not sure it's worth having a template-resolution endpoint if all it does is given a template slug, provide the slug of the template that matches most closely, according to the template resolution algorithm -- when that functionality is likely only ever needed upon template creation, where it could be easily absorbed into the existing templates endpoint (as seen in this PR) 😬 (I'm also not too worried about changing that endpoint's behavior, since it'd be very much consistent with how the template resolution works on the frontend.)

(Sidenote: Maybe it'd make eventually sense to have a template resolution endpoint that emulates the actual routing as done by WordPress on the frontend, i.e. given a route, returns the template that's in charge of rendering that. E.g. GET /wp/v2/template-resolution?route=/category/mycat returning category. This might be relevant for "headless" WordPress installations that still want to keep the route structure and template hierarchy. It'd also be significantly more complex to implement, since it's basically have to mimic WP_Query.)


Well, that probably sounded a bit defensive when I really just wanted to clarify my reasoning for this PR 😅
However, if people generally feel that there should be a dedicated endpoint for the resolution, I'm not going to resist that. Since I've spent a bunch of time on this subject (see #37054 and #41848) -- is there anyone who'd volunteer to work on that? 😬

@ntsekouras
Copy link
Contributor

Personally, I don't like the idea too much to have:
one endpoint solely for that latter template resolution based on a slug (something like GET /wp/v2/template-resolution/category-mycat -- which returns the next-best existing fallback template, e.g. category)

whose response we then use to query the existing templates endpoint (GET /wp/v2/templates/category) to get the fallback template's content

I think one endpoint for the above two is enough. Making just an extra request for the content and keeping the basic create requests intact seems good enough for me, with no big overhead. After all it's not a hot path..

@ockham
Copy link
Contributor Author

ockham commented Jul 11, 2022

I think one endpoint for the above two is enough. Making just an extra request for the content and keeping the basic create requests intact seems good enough for me, with no big overhead. After all it's not a hot path..

But that would mean that we'd have two endpoints that accept a template slug and return the template's content -- only that one would use the fallback lookup whereas the other one wouldn't (unless I'm misreading you). But that's the kind of duplication of functionality that I thought would confuse people (and likely be problematic with regard to maintenance -- i.e. if a developer wants to change something and does it to one endpoint but not the other)...

@TimothyBJacobs
Copy link
Member

Thanks for your detailed thoughts @ockham. I think we can avoid the pitfalls you describe.

As mentioned by @ntsekouras we can consolidate the first two requests via a couple of different methods.

  1. If additional meta information about the resolution is needed, for example potentially other templates that were considered (?), we can utilize linking & embedding to link to the template object that is the "best match". This will save the client a roundtrip.
  2. Alternatively, we can utilize a 302 redirect response which would redirect to the found template.
  3. Lastly, we can use an internal rest_do_request to return the response for the found template without us needing to duplicate REST API serialization logic.

@ockham
Copy link
Contributor Author

ockham commented Aug 4, 2022

Closing as obsolete, now that #42520 has been merged.

@ockham ockham closed this Aug 4, 2022
@ockham ockham deleted the try/prepopulating-template-with-fallback-upon-creation branch August 4, 2022 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Block Template creation is using empty content instead of the appropiate fallback
4 participants