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

Always include core/query block correctly in Twenty Twenty-Three theme #4341

Conversation

felixarntz
Copy link
Member

This PR fixes the 3 templates in TT3 which had problems due to incorrect / missing usage of the core/query block.

Trac ticket: https://core.trac.wordpress.org/ticket/58154


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@felixarntz
Copy link
Member Author

#4342 is another PR using the same branch as this one just for testing purposes, which also includes the changes from WordPress/gutenberg#49904.

@gziolo
Copy link
Member

gziolo commented Apr 19, 2023

Does it mean that with all the changes applied, all themes must use the Query Loop also on the single page/post pages?

@felixarntz
Copy link
Member Author

felixarntz commented Apr 19, 2023

@gziolo Yes, that's correct.

I understand that may seem odd, but it is critical that the query loop runs in those templates as well. To be fair, maybe there is a simpler way, in which we can solve it automatically without requiring use of the core/query block - but the query loop itself has to be used. Currently, the way the page and single templates of TT3 are written, with no query loop, it breaks the way WordPress core is expected to work, and has been expected to work since basically forever.

See any of the other classic default themes for reference - they all have the query loop in page.php and single.php as well, even though it's clear that only a single post/page will be rendered there.

To clarify: We don't necessarily have to use the core/query block on those templates, but we need to somehow ensure that the while ( have_posts() ) { the_post(); ... } code is run around where the core/post-* blocks are rendered. That code is currently in the core/post-template block, but that block can only be used within a core/query block.

@carolinan left a related comment in https://core.trac.wordpress.org/ticket/58154#comment:5. I'd be happy to think about other ways to handle this - maybe we can modify Gutenberg to allow using only the core/post-template block by itself (without core/query around it), or we can introduce a new block type intended for singular post content that ensures the query loop is used.

But the current behavior is causing several bugs that break backward compatibility, so fixing that in some way is critical.

@ntsekouras
Copy link

Does it mean that with all the changes applied, all themes must use the Query Loop also on the single page/post pages?

That was my first thought too and it seems quite hard to reinforce such a restriction, since with the site editor you can edit visually a template and can start with an empty one. Also what happens to other existing block themes that do not have these updates?

I'm all for removing the call to the_post if(and where) we can in the blocks, but if the problem exists in classic themes now, could we do some conditional calling based on that info? I mean block themes are new and while we want back compat, we don't aim to preserve all the 'old' ways of doing things.

Calling the_post started in an attempt to accommodate some third party template usages, that had checks about internal flags of WP_Query. After that there have been some changes in that logic regarding some other nuances(I'm not aware of all of them, sorry). Do we still need it at all or in all places? 🤔

From a different comment:

... some places in code check if they are in the loop and this is bad to start with. Ideally templates/blocks shouldn't rely on the loop(that goes for post content) and maybe there could be another way to handle lazy loading of images(post featured image)?
I'm not sure how we could find plugins or blocks that rely on the loop and start a deprecation process.. Normally using block context could make the trick, but I'm not sure of the nuances of every use case.

@felixarntz
Copy link
Member Author

@ntsekouras

That was my first thought too and it seems quite hard to reinforce such a restriction, since with the site editor you can edit visually a template and can start with an empty one.

That's a great point. It suggests to me that we should probably find a low-level solution in which we ensure the loop is always properly started regardless of using the core/query or core/post-template blocks.

I mean block themes are new and while we want back compat, we don't aim to preserve all the 'old' ways of doing things.

This I find a confusing statement. Either we care about back compat, or we don't :) Not having the while ( have_posts() ) { the_post(); ... } run in every template is a critical backward compatibility break - it isn't just to satisfy some old convention, but it breaks expected behavior of WordPress.

Maybe we can add some intelligent logic where we in block templates without a core/query block, we detect the usage of the first and last core/post-* blocks and run the logic to start the loop before the first and close it after the last?

To conclude, I see the point of not putting this problem on block theme developers, so maybe this PR is not the right way to go. But the problem still needs to be fixed. Probably better to continue this discussion in WordPress/gutenberg#49904, which is more about the root problem.

@felixarntz
Copy link
Member Author

Closing in favor of #5104.

@felixarntz felixarntz closed this Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants