Fix query loop bugs by correctly relying on the main query and removing problematic workaround

[felixarntz]

This PR will fix https://core.trac.wordpress.org/ticket/58027 and https://core.trac.wordpress.org/ticket/59225 once merged into WP core. The bug can be replicated with Gutenberg as well though.

What?

Currently, block themes do not handle "the loop" correctly. Since their existence, WordPress themes have been expected to have the main WP_Query object go through the loop of posts when rendering posts, which is commonly done with code such as the following:

while ( have_posts() ) {
	the_post();
	// Render post etc.
}

The above logic ensures the WordPress query loop is correctly handled, e.g. calling in_the_loop() will return true as expected.

In block themes, this doesn't happen correctly though, the loop is not started when rendering posts which breaks the long-standing expectation that is relied on in both WP core and the plugin ecosystem. This is because the core/post-template block, when configured to use the global query (via the inherit attribute), clones the global query object rather than using it directly. This leads to the main query loop never being started since it is only started on the cloned instance.

Before that problem was introduced even, based on an older version of the query block which at the time also did not trigger the loop correctly, the lack of proper loop handling was "addressed" with a workaround in the core/post-content (and later core/post-featured-image) block which effectively forces the main query loop to start if it was not started before. That workaround is not a proper fix and itself causes bugs, such as the one outlined in https://core.trac.wordpress.org/ticket/58027.

Looking at the history led to the following findings:

• Query loop infinite loop when inheriting query from template #43198 (via Prevent query block from looping in classic themes #43221) introduced the latest underlying query loop bug, in an attempt to fix another (more edge-case) bug originally reported in [Bug]: Query Loop block causes infinite loop with older themes and inherit template Automattic/wp-calypso#66270.
• Block Library - Query: Use a WordPress loop for the query block #30405 introduced the problematic workaround in the core/post-content block which is a hack to force the loop to start if it hasn't started yet.
• Ensure post-featured-image block is in_the_loop() for BC with core and plugins, and to fix lazy-loading #45534 then added the same hack to the core/post-featured-image block.

Why?

WordPress has always expected the query loop to be used when rendering posts, even when a template is only expected to render a single post (such as the single or page templates). Classic themes therefore start the loop even on those templates, but it looks like this was missed in block themes, which has led to the aforementioned problems.

Workarounds like enforcing the loop in certain blocks is not a proper solution for that and as mentioned above has led to its own set of problems.

How?

This PR ensures that the loop is properly handled in block themes, in a way that fixes the bug in https://core.trac.wordpress.org/ticket/58027 while also maintaining a fix for Automattic/wp-calypso#66270 (comment).

• In block templates where the global main query is used, the core/post-template block is updated to actually use the main query directly which will fix the loop to work correctly with any in_the_loop() checks and address https://core.trac.wordpress.org/ticket/59225.
  • Also in the core/post-template block, if there is already a loop (which would only happen when using the block in a classic theme, or an even more edge-case scenario where the block is used nested within another core/post-template block), the cloning of the main query is maintained, however with the addition of a WP_Query::rewind_posts() call on the cloned instance, to ensure that, even if the loop on the main query was already started, the cloned instance will render all relevant posts. This is effectively a more appropriate fix to [Bug]: Query Loop block causes infinite loop with older themes and inherit template Automattic/wp-calypso#66270 / Query loop infinite loop when inheriting query from template #43198.
• The workaround to enforce the loop in core/post-content and core/post-featured-image is removed as it's no longer necessary following the above fix, which effectively will fix the bug reported in https://core.trac.wordpress.org/ticket/58027.

Testing Instructions

Before testing, add the following code snippet to your WP installation (could be added anywhere, in a plugin, or hacked into any core or theme functions file):

add_filter(
	'render_block_core/null',
	function( $block_content ) {
		return $block_content . '<p>in the loop: ' . ( in_the_loop() ? 'true' : 'false' ) . '</p>';
	}
);
add_action( 'wp_body_open', function() {
	if ( wp_is_block_theme() ) { // Only use this code when using a classic theme.
		return;
	}
	$block_content = '<!-- wp:post-featured-image /-->';
	$block = parse_blocks($block_content)[0];
	echo render_block($block);
} );

1. Create 2+ dummy posts, each with a title and regular text content (e.g. core/paragraph blocks).
2. Use Twenty Twenty-Three (block theme).
3. Open the home page. You should see under each post "in the loop: true". This works only because of the workaround in the core/post-content and core/post-featured-image blocks.
4. Open the Site Editor and edit the home page ("Blog Home") template: Remove the core/post-content and core/post-featured-image blocks from within the post template (which should lead to the featured image placeholders and content disappearing from all posts displayed).
5. Open the home page again. Now you should see under each post "in the loop: false", confirming that the loop is no longer entered and only worked due to the workaround.
6. Switch to Twenty Twenty-One (classic theme).
7. Open any of the individual posts in the frontend (single template). You should see no post content being displayed. This is because the core/featured-image block is technically displayed (if any of the posts has a featured image, you should see that on top of the page), forcing the loop to start before it should. So then when the actual theme attempts to start the loop, it has already reached the end and therefore displays nothing.

Now, apply this PR to Gutenberg (or the mirror PR WordPress/wordpress-develop#5103 to WP core), then test the above again. You don't need to create more posts of course, so you can start at step 2. You should see in steps 5 and 7 that the bugs have been fixed:

• In step 5, you should now see "in the loop: true" under each post.
• In step 7, you should now see the actual post with its title, content etc.

Last but not least, test the following to ensure the bug fixed in Automattic/wp-calypso#66270 / #43198 remains fixed (i.e. no PHP logic infinite loop):

1. Use Twenty Twenty-One (classic theme).
2. Create a new post and add a core/query block to it (use any of the patterns, such as "Grid"). Make sure the "Inherit query from template" toggle is enabled in the sidebar.
3. Publish the post and view it in the frontend. You should see the post appear within the post, and it should have "in the loop: true" below.

[github-actions]

Flaky tests detected in 67f582a.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6003053661
📝 Reported issues:

• [Flaky Test] allows rotating using the crop tools #36904 in /test/e2e/specs/editor/blocks/image.spec.js

[Mamaduka]

Thank you, @felixarntz!

I still have to start testing changes properly, but I wanted to leave a small note: We'll need to test previewing unsaved changes for posts/pages.

[gigitux]

On WooCommerce Blocks, we are working on the Blockified Single Product Template. We noticed a wrong behavior in the template when the Post Content block was used.
"The reason for this is that the ”Post Content” block calls the_post function, which is a function with global side-effects: it doesn't just set the context, but in fact moves the index of the loop forward when it is called. Instead, the “Product Details” block work, because the logic was copied from the WooCommerce classic template, it uses the loop and also calls the_post function. This means that the two conflict as the order in which the blocks have been laid out within the page will matter and disrupt the rest of the blocks." (for more details, check this discussion.)

I can confirm that this PR fixes the wrong behavior

[ntsekouras]

Cross commenting here from the core PR that updates 2023:

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]

Cross-commenting my reply:

@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 best to continue the conversation here and park the TT3 PR for now as some alternative approach that likely is not the desirable solution.

[felixarntz]

Together with @gziolo, we took a deep dive into the problems here at WordCamp US, and we have since identified the root issue and solution. While the original PR was trying to tackle a different problem, we identified that that is not the key issue, so the PR description has been updated to outline in a detailed way what is happening and why. A core ticket for the root problem has been opened in https://core.trac.wordpress.org/ticket/59225.

Please review the PR description carefully as quite some history research was needed to understand the problem, particularly as this PR addresses 3 different intertwined issues.

The PR has been updated and is ready for full review and testing. Testing instructions can be found in the PR description.

cc @ebinnion @draganescu @aristath @ntsekouras @Mamaduka since you were involved in the referenced issues

[github-actions]

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files

❔ phpunit/blocks/render-post-template-test.php

[draganescu]

This looks as a good fix on top of the previous fixes, one that learned how to only solve the edgecase issues, without breaking expectations. We still clone the query but only doing it when it makes sense. When I authored #43221 I wasn't aware it would cause other problems - the extra check here is a welcome addition.

[ntsekouras]

I haven't tested thoroughly, but the code here looks good and makes sense to me. Thanks!

[gziolo]

I'm going to test the changes with the patch prepared for WordPress Core. It's great to see that the Gutenberg part landed so quickly. Thank you all for help.

[felixarntz]

@gziolo This can be backported now that https://core.trac.wordpress.org/changeset/56507 has been committed.

I have already implemented core specific tests in WordPress/wordpress-develop#5103 (which is otherwise a mirror of the changes here). Can we add those tests together with the commit to port the changes or would that need to happen separately after porting over the changes?

[Mamaduka]

Has anyone tested post previews with these changes?

[gziolo]

I have already implemented core specific tests in WordPress/wordpress-develop#5103 (which is otherwise a mirror of the changes here). Can we add those tests together with the commit to port the changes or would that need to happen separately after porting over the changes?

I'm wondering if it is possible to update get_the_block_template_html in the Gutenberg plugin to support WP 6.3 and 6.2. Maybe it would make sense to apply the same patch in WP 6.3.2 to mitigate the issue instead?

[felixarntz]

@gziolo
Can you clarify what you mean? I don't think we can modify get_the_block_template_html() in Gutenberg since it's a core function that can't be overwritten as far as I can tell. So backporting https://core.trac.wordpress.org/changeset/56507 to the 6.3 branch may be a reasonable approach. Would you mind bringing that conversation over to https://core.trac.wordpress.org/ticket/58154 where that change was made?

[pbiron]

I can confirm that this PR fixes a problem I was just about to open an issue for: I'm developing a block theme and one of the templates has 2 different Query Loop blocks in it and the 2nd one was displaying 1 fewer posts than it should have. With this PR applied the problem is resolved.

Thank you @felixarntz !!

[gziolo]

@felixarntz, I meant that we should consider patching 6.3 because I doubt it's possible to fix the issue for WP 6.2 and 6.3 in the Gutenberg plugin. I commented on Trac, too.

Aside: I really don't know why the plugin still declares support for 6.2 😅

[felixarntz]

@gziolo Do you mean patching with the fix for https://core.trac.wordpress.org/ticket/58154, or this one here (which is for https://core.trac.wordpress.org/ticket/59225)? Just to confirm, since those are two separate fixes and tickets. Or do you mean both?

[gziolo]

Too many tickets and PRs 😅

This can be backported now that https://core.trac.wordpress.org/changeset/56507 has been committed.

I was referring to this commit in core, as it'd be difficult to add the same changes in the Gutenberg plugin. However, this PR could be also a good candidate to backport to WP 6.3.2. I would say both would be fine to include. I hope it isn't too complex as I see that WordPress/wordpress-develop#5103 is still open and not merged to trunk.

Again, https://core.trac.wordpress.org/changeset/56507 is what I was talking about.