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.

Sign up for GitHub

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

Jump to bottom

Query Loop: Remove is_singular() check and fix test #65483

Merged
merged 3 commits into from
Sep 20, 2024
Merged

Query Loop: Remove is_singular() check and fix test #65483

merged 3 commits into from
Sep 20, 2024

Conversation

mikachan
Copy link
Member

@mikachan mikachan commented Sep 19, 2024 •
edited
Loading

What?

This is a follow-up to #65067 to improve the implementation of querying 'post' post types.

Why?

As inherit is now automatically set to false when a Query Loop is inserted within content (a single post/page/CPT), we don't need to manually update the query in an is_singular() check.

Also, query_posts() should generally be avoided as it could cause some unwanted side effects. See this comment for more details.

How?

Removes the is_singular() check from the Post Template server-side render, and updates the test_rendering_post_template_with_main_query_loop_already_started test so that it is correctly using inherit: false in the test block markup.

Testing Instructions

  1. Insert a query loop block in a singular page/post/CPT
  2. Ensure that it defaults to displaying posts in both the Editor and front-end
  3. Ensure that the post type can be changed in the Editor and, if customised, that it shows the custom query on the front-end
  4. Ensure the test_rendering_post_template_with_main_query_loop_already_started PHP unit test passes

@mikachan mikachan added [Type] Bug An existing feature does not function as intended Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta [Block] Query Loop Affects the Query Loop Block Backport to Gutenberg RC Pull request that needs to be backported to a Gutenberg release candidate (RC) labels Sep 19, 2024
@mikachan mikachan self-assigned this Sep 19, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 19, 2024 •
edited
Loading

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mikachan <[email protected]>
Co-authored-by: jffng <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@mikachan mikachan mentioned this pull request Sep 19, 2024
Merged
jffng
jffng reviewed Sep 19, 2024
View reviewed changes
Copy link
Contributor

@jffng jffng left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change makes sense to me and still works as described in the original PR. I just have a question about the test.

phpunit/blocks/render-post-template-test.php
@@ -122,7 +122,7 @@ public function test_rendering_post_template_with_main_query_loop_already_starte
global $wp_query, $wp_the_query;

// Query block with post template block.
$content = '<!-- wp:query {"query":{"inherit":true}} -->';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this test need to change? Was this not testing with the correct inherit setting before?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this not testing with the correct inherit setting before?

No 🙈 And it was passing because I was using query_posts(), which I think was replacing the global query, so anything set via the Query Loop block was ignored and wasn't truly being tested.

This test should have been updated when the changes were added for automatically setting inherit to false in the original PR. It was a complete oversight by me, although I'm glad I updated the test earlier on in the process and so I knew how to fix it. 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see! Good thing you caught it early.

@mikachan mikachan changed the title Query Loop: Improve post query implementation Query Loop: Remove is_singular() check and fix test Sep 20, 2024
@mikachan
Copy link
Member Author

Thanks for taking a look, @jffng! With @dlh01's help, I noticed that we no longer need the is_singular() check in the server-side rendering of the Post Template. It looks like setting inherit to false when within singular content was the only change needed.

I've updated the PR to include this change, and we also need the fix for the test so it's testing with the correct markup (inherit: false).

This is ready for another review 🙏

@mikachan mikachan merged commit b1d9255 into trunk Sep 20, 2024
63 checks passed
@mikachan mikachan deleted the update/query-loop-alter-query branch September 20, 2024 16:50
@github-actions github-actions bot added this to the Gutenberg 19.4 milestone Sep 20, 2024
@github-actions github-actions bot removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Sep 20, 2024
gutenbergplugin pushed a commit that referenced this pull request Sep 20, 2024
@mikachan @jffng
@dlh01 @gutenbergplugin
Query Loop: Remove is_singular() check and fix test (#65483)
3215aa5 
* Replace query_posts() with set()

* Fix test_rendering_post_template_with_main_query_loop_already_started test

* Remove is_singular check

Co-authored-by: mikachan <[email protected]>
Co-authored-by: jffng <[email protected]>
Co-authored-by: David Herrera <[email protected]>
@github-actions github-actions bot added the Backported to WP Core Pull request that has been successfully merged into WP Core label Sep 20, 2024
@github-actions GitHub Actions
Copy link

I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: 3215aa5

kevin940726 pushed a commit that referenced this pull request Sep 23, 2024
@mikachan @jffng
@dlh01 @kevin940726
Query Loop: Remove is_singular() check and fix test (#65483)
f731ae7 
* Replace query_posts() with set()

* Fix test_rendering_post_template_with_main_query_loop_already_started test

* Remove is_singular check

Co-authored-by: mikachan <[email protected]>
Co-authored-by: jffng <[email protected]>
Co-authored-by: David Herrera <[email protected]>
@kevin940726
Copy link
Member

I just cherry-picked this PR to the release/19.3 branch to get it included in the next release: f731ae7

@kevin940726 kevin940726 removed the Backport to Gutenberg RC Pull request that needs to be backported to a Gutenberg release candidate (RC) label Sep 23, 2024
@noisysocks noisysocks mentioned this pull request Sep 23, 2024
Closed
This was referenced Sep 26, 2024
Open
Open
This was referenced Sep 27, 2024
Open
Open
Open
Open
Open
Open
Open
Open
@draganescu draganescu mentioned this pull request Oct 15, 2024
Merged
@sirreal sirreal mentioned this pull request Oct 25, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jffng jffng jffng approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra is a code owner

Assignees

@mikachan mikachan

Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Block] Query Loop Affects the Query Loop Block [Type] Bug An existing feature does not function as intended
Projects
WordPress 6.7 Editor Tasks
Status: ✅ Done
Milestone
Gutenberg 19.3
Development

Successfully merging this pull request may close these issues.
3 participants
@mikachan @kevin940726 @jffng