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: ellatrix <ellatrix@git.wordpress.org>
Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org>

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

Size Change: -65 B (0%)

Total Size: 1.82 MB

_{compressed-size-action}

It seems to me that, for inherited queries, the query block should not at all concern itself with the perPage property. It should be something that the post template block ignores and overrides for inherited queries. In fact, everything related to the perPage setting is disabled elsewhere when the query is set to inherit.

That seems fine to do in this case (inherit:true) since we don't have dynamic pagination blocks in the editor, but in this PR doesn't work as intended for custom ones.

The reason for this is that custom Query Loop blocks need to have a perPage value set because other child blocks (including pagination ones) rely on that to work properly. One way would be to see if we can change the default value from null to something else and also check if that would be affect existing consumers (I'd expect it would be fine).

Also in your PR you don't remove completely the effect in Query Loop, so I guess the edit can still happen.

Finally I'm curious if this change improves performance and how. Do you have any metrics?

The reason for this is that custom Query Loop blocks need to have a perPage value set because other child blocks (including pagination ones) rely on that to work properly. One way would be to see if we can change the default value from null to something else and also check if that would be affect existing consumers (I'd expect it would be fine).

Good point. Yes a default should be provided there I guess, added it in bcaa6f4.

But I didn't see perPage being used in any of the pagination blocks. Could you point me in the right direction? Isn't this deduced from the REST API result (total pages count etc.)? Maybe the default of 3 should be added in the consumers instead, not sure, but I cannot locate them.

Also in your PR you don't remove completely the effect in Query Loop, so I guess the edit can still happen.

The edit can still happen, but for another reason that not generally occur, and yes, it should also be eliminated.

Finally I'm curious if this change improves performance and how. Do you have any metrics?

This improves performance by eliminating re-renders after the initial mount. Setting attributes in an effect should generally be avoided, it's not a good practice.

But I didn't see perPage being used in any of the pagination blocks. Could you point me in the right direction? Isn't this deduced from the REST API result (total pages count etc.)? Maybe the default of 3 should be added in the consumers instead, not sure, but I cannot locate them.

build_query_vars_from_query_block in core. So to be safer we would need to provide the same default there.

I'm not sure if other consumers (3rd party blocks) rely on that info in some way though, and how to assess that..

So to be safer we would need to provide the same default there.

Agreed, let's use the attribute default value then.

Can you rebase for the conflicts?

@ntsekouras Done :)

Before the perPage would always be set during insertion. There might be third party patterns that do not set this value and can be inserted through the inserter. In that case they would not take the default from block.json and in front end the per_page will not be set before passed to WP_Query. WP_Query sets the posts_per_page in that case having a mismatch with the editor and therefore user expectations.

So, I still think:

build_query_vars_from_query_block in core. So to be safer we would need to provide the same default there.

@ntsekouras I don't get it. perPage is still always set during insertion? After you insert the query block, it's entirely controlled by the placeholder? So it's never the case that it ends up without a query and perPage, unless the variation or pattern knowingly left it out. Thinking about it more, we should actually remove the default from block.json. 🤔

Flaky tests detected in dce564a.
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/11782192622
📝 Reported issues:

• [Flaky Test] submenu click on the arrow interactions #61884 in /test/e2e/specs/editor/blocks/navigation-frontend-interactivity.spec.js

@ntsekouras I don't get it. perPage is still always set during insertion? After you insert the query block, it's entirely controlled by the placeholder? So it's never the case that it ends up without a query and perPage, unless the variation or pattern knowingly left it out.

Exactly. Since before Query Loop would set that property, a variation or pattern could have left it out. Also you can insert a pattern from the inserter directly and you skip the placeholder stage.

Thinking about it more, we should actually remove the default from block.json. 🤔

Without the default value you have the problem we discussed above. But I still believe you also need the same default in php when we build the query, in case it's not set (my other comment).