Make WordPress Core

Opened 18 months ago

Closed 13 months ago

Last modified 12 months ago

#58154 closed defect (bug) (fixed)

Block theme singular templates do not enter main query loop

Reported by: flixos90's profile flixos90 Owned by: flixos90's profile flixos90
Milestone: 6.4 Priority: normal
Severity: normal Version: 6.1
Component: Editor Keywords: has-patch has-testing-info has-unit-tests
Focuses: Cc:

Description (last modified by flixos90)

Most block theme templates for singular content (is_singular()) are missing the core/query and core/post-template blocks which are needed to start the main query loop. This breaks the long-standing expectation, which is relied on by core and many plugins.

Related, yet a different problem is #59225. As part of that ticket, the workaround found in the core/post-content and core/post-featured-image blocks is removed accordingly, however that is not a coherent fix to the problem anyway.

To test:

  1. Use Twenty Twenty-Three.
  2. In the Site Editor, update the single post template to not display the core/post-featured-image and core/post-content blocks.
  3. Add the below PHP code snippet to your WP installation.
  4. View a single post and notice the "in the loop: false" text.
  5. After applying the PR https://github.com/WordPress/wordpress-develop/pull/5104 to your WP installation and refreshing the URL, you should see "in the loop: true".
add_filter(
        'render_block_core/post-title',
        function( $block_content ) {
                return $block_content . '<p>in the loop: ' . ( in_the_loop() ? 'true' : 'false' ) . '</p>';
        }
);

Change History (29)

#1 @flixos90
18 months ago

  • Description modified (diff)

This ticket was mentioned in PR #4341 on WordPress/wordpress-develop by @flixos90.


18 months ago
#2

  • Keywords has-patch added; needs-patch removed

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

#3 @flixos90
18 months ago

I implemented another PR https://github.com/WordPress/wordpress-develop/pull/4342 just for testing purposes which also includes the fixes needed on the Gutenberg side to make this all work correctly, which will also fix #58027.

Also see the Gutenberg PR https://github.com/WordPress/gutenberg/pull/49904 that includes the same changes, as they will need to be actually implemented in Gutenberg and then ported over to WordPress core.

@flixos90 commented on PR #4341:


18 months ago
#4

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

#5 @poena
18 months ago

I believe one of the reasons why the page and single templates were simplified to use post content is that the query block is or was considered one of the more difficult blocks for end users to manage.

We have to consider that there is a risk that a query block with the incorrect setting (not inheriting as described above) breaks the display of the content.

So how can we make it "right" but also easy?

@gziolo commented on PR #4341:


18 months ago
#6

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

@flixos90 commented on PR #4341:


18 months ago
#7

@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 itself _has_ to be running. 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.

@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 commented on PR #4341:


18 months ago
#8

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.

@flixos90 commented on PR #4341:


18 months ago
#9

@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 https://github.com/WordPress/gutenberg/pull/49904, which is more about the root problem.

#10 @flixos90
16 months ago

  • Milestone changed from 6.3 to 6.4

Based on conversations in the PR and #58027, changing the themes may not be the best approach here. We will have to continue exploring whether this can be solved directly in the underlying core blocks, so that themes don't have to be updated.

Keeping this open for further consideration, but like #58027 it will have to be punted as it is too late to reasonably address this now, even though it's classified as a bug - the changes needed would be too substantial.

This ticket was mentioned in PR #5104 on WordPress/wordpress-develop by @flixos90.


14 months ago
#12

While initially it seemed that this had some overlap with https://github.com/WordPress/gutenberg/pull/49904, a closer review of the underlying issues determined this problem being separate: The other PR addresses problems with the main query loop _in general_, while this PR and the below ticket only relate to singular templates in block themes and that they commonly lack the necessary blocks core/query and core/post-template which are used to start the main query loop.

This PR ensures that the main query loop is still started for block theme templates that lack those blocks. While alternatively we could consider introducing a new "invisible" wrapper block for this purpose, that would then require pretty much all block themes out there to adjust their templates, needless to say that any templates already modified in the database would not receive those updates. So while the fix in this PR is a workaround, it is reliable as there is by definition only a single post in the main query for singular content and thus we can wrap the entire template into the main query loop.

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

#13 @flixos90
14 months ago

  • Component changed from Bundled Theme to Editor
  • Description modified (diff)
  • Keywords has-testing-info needs-testing added
  • Summary changed from Twenty Twenty-Three uses query block incorrectly to Block theme singular templates do not enter main query loop

@flixos90 commented on PR #4341:


14 months ago
#14

Closing in favor of #5104.

#15 @flixos90
14 months ago

Cross-posting from https://github.com/WordPress/wordpress-develop/pull/5103#issuecomment-1696381466: The fix from https://github.com/WordPress/wordpress-develop/pull/5104 should preferably be committed to WP core before porting over the changes from #59225, in order to keep the loop on singular block templates functional together with the other change.

@flixos90 commented on PR #5104:


13 months ago
#16

Thanks @youknowriad!

This looks good to me. Is this dependent on the Gutenberg fix? Should we be waiting for the package update first before committing?

No, in fact it would be better to first commit this one before backporting the other Gutenberg fix, see https://core.trac.wordpress.org/ticket/58154#comment:15.

@youknowriad commented on PR #5104:


13 months ago
#17

This looks good to me. Is this dependent on the Gutenberg fix? Should we be waiting for the package update first before committing?

@gziolo commented on PR #5104:


13 months ago
#18

I did quite extensive testing using the Gutenberg plugin version downloaded from the linked PR:

https://github.com/WordPress/gutenberg/actions/runs/6003053680

https://github.com/WordPress/gutenberg/suites/15571281198/artifacts/888604674

I identified some edge cases in the block-based theme that I believe we saw together with @felixarntz last week during WC US:

Homepage rendered on the front

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/699132/da014395-dc3f-4221-85e5-d723fc090eff

Everything works here as expected – "Third post with loop" renders the same loop as the one on the homepage and it correctly skips rendering the post content, which would lead to infinite loop.

Blog Home template in the editor

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/699132/956ef1e8-a549-470b-954f-be514db08295

For some reasons, the nested query loop doesn't get displayed, but that might be good in this context.

Third post with loop on the frontend

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/699132/409a0656-9e5f-4ec5-b003-ffc6bdf8c3dd

The debugging message is only visible when WP is in the debug mode. More importantly, the query loop isn't correctly rendered.

Third post with loop in the editor

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/699132/10667ace-56b3-4ade-8fdf-fab23fc44a2f

Everything works as expected in the editor though.

@flixos90 commented on PR #5104:


13 months ago
#19

Thank you for the detailed testing @gziolo! Some follow up feedback on your notes:

  • I think your observations on the "third post with loop" aren't quite right: on the frontend, when viewed as the singular post in TT3 theme, its query loop only displays itself, which is correct, since the global query loop on that URL is only that post. I don't think there's any real use-case for doing that anyway, but it's working as it should. If anything, that part is off in the editor, where it should also display just that post, not the list of all posts. It should only display all the posts when looking at the post in an archive. Basically what is displayed in that post will differ depending on the current main query loop.
  • Regarding your observation in the TT1 theme, as you've already suspected, in the archive view the loop inside "third post with loop" is not displayed as that template is only using the excerpt where any "rich content" is cut.

@flixos90 commented on PR #5104:


13 months ago
#20

Thanks @gziolo. Based on the findings, I also left an update on https://github.com/WordPress/gutenberg/issues/50790: I don't think it makes sense that we even allow inheriting the main query inside a specific post.

joemcgill commented on PR #5104:


13 months ago
#21

A new concern I have after thinking about this some more, is that it essentially forces all singular block templates to include the entire template in "the loop", which can cause inconsistent behavior for things like headers or other template parts that are traditionally outside the loop. For example, if we have some logic that attempts to apply some image optimizations only when the image is inside the loop, with this change, a header image would be included on singular pages but not on list pages like archives. This is very edge case, but could be confusing. Would it be safer to only apply this logic to the full template if we are able to first do a has_block() check to see if a theme has omitted one of the blocks that would start the loop as expected?

@flixos90 commented on PR #5104:


13 months ago
#22

@joemcgill Your concern is a fair point, and I've considered that initially when I started this PR.

Would it be safer to only apply this logic to the full template if we are able to first do a has_block() check to see if a theme has omitted one of the blocks that would start the loop as expected?

I did have that in here at the start, but there are several limitations with that:

  • There is no existing core block that would realistically be used on a singular template to trigger the loop. I thought originally that the core/query and core/post-template loop would also be usable for that purpose, but they unfortunately include additional markup _forcing_ posts to be in a semantic list, which makes no sense for singular content and can be problematic for accessibility.
    • Figuring this out would require notably more discussion and changes. For example, would we introduce a new block for singular loops? Or somehow adjust the core/post-template block to conditionally not have wrapping markup? Those things may end up quite confusing from an end user perspective too.
  • Even if there were singular-friendly variant of these blocks, we would need something more complex than has_block(), because we would only _not_ need to trigger the main query if the core/post-template block was wrapped in a core/query block that had the inherit: true attribute set - something more costly to check for which there's no dedicated function available.

While what you're raising is indeed a potential concern, I see it more as a theoretical concern. While typically themes indeed only have the loop around the main content of a singular URL, I can't think of any actual problem that wrapping the whole content of such a URL would cause.

In other words: I think this is something worth revisiting in the future, but I don't consider it a blocker, since the current issue is certainly more severe, and properly fixing the additional concern would require more discussion and effort than what we could realistically tackle for 6.4.

#23 @flixos90
13 months ago

  • Keywords needs-testing removed

#24 @flixos90
13 months ago

  • Keywords has-unit-tests added

#25 @flixos90
13 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 56507:

Editor: Ensure main query loop is entered for singular content in block themes.

Block themes currently lack the means to trigger the main query loop for singular content, since they cannot reasonably use the core/query and core/post-template blocks which are intended only for displaying a list of posts. So far, the missing main query loop on singular block templates has been worked around by enforcing the loop in certain core/post-* blocks, which however causes other bugs.

This changeset ensures that the main query loop is still started for singular block theme templates, by wrapping the entire template into the loop, which will by definition only have a single cycle as it only encompasses a single post. This is currently the most reliable solution, since even if there were blocks to properly trigger the main query loop on singular content, it would be unrealistic to expect all existing block themes to update their templates accordingly. It may be revisited in the future.

Props gziolo, youknowriad, joemcgill, costdev, mukesh27, flixos90.
Fixes #58154.
See #59225, #58027.

#27 @gziolo
13 months ago

In https://github.com/WordPress/gutenberg/pull/49904#issuecomment-1703159329 @flixos90 asked whether we should port these changes to the Gutenberg plugin to fix the issue with WordPress 6.2 and 6.3 that are still supported there. I don't think it's possible without too much work. I believe that in this case, it would be preferable to backport this patch to the 6.3 branch in core as part of the planned WordPress 6.3.2 release.

#28 @flixos90
12 months ago

In 57019:

Themes: Skip wrapping block template for singular content with a main query loop when the template was injected from outside the current theme.

As a follow up to [56507], this fixes a bug that could occur for instance when plugins hijack the block template detection process to inject their own block template with entirely custom logic.

Props afragen, hellofromTonya, costdev, mukesh27, huzaifaalmesbah, flixos90.
Fixes #59736.
See #58154.

#29 @flixos90
12 months ago

In 57020:

Themes: Skip wrapping block template for singular content with a main query loop when the template was injected from outside the current theme.

As a follow up to [56507], this fixes a bug that could occur for instance when plugins hijack the block template detection process to inject their own block template with entirely custom logic.

Props afragen, hellofromTonya, costdev, mukesh27, huzaifaalmesbah, flixos90.
Merges [57019] to the 6.4 branch.
Fixes #59736.
See #58154.

Note: See TracTickets for help on using tickets.