Make layout supports compatible with enhanced pagination#5528
Make layout supports compatible with enhanced pagination#5528luisherranz wants to merge 18 commits intoWordPress:trunkfrom
Conversation
tellthemachines
left a comment
tellthemachines
left a comment
There was a problem hiding this comment.
Thanks for the PR! The code change itself LGTM; my question is whether the layout file is the best home for the wp_incremental_id_per_prefix function. Might we be using it elsewhere in the future?
Also might be good to have a unit test for the function, similar to wp_unique_id here.
I pushed a fix for the failing PHP tests that were still expecting the old classnames.
costdev
left a comment
costdev
left a comment
There was a problem hiding this comment.
Thanks for the PR @luisherranz and for the ping @tellthemachines!
I've left some thoughts in this review.
|
Hi there! I applied all changes requested. Thanks for the review @tellthemachines @costdev |
costdev
left a comment
costdev
left a comment
There was a problem hiding this comment.
Thanks for the updates @luisherranz!
I've left a note about a minor alignment issue, and an example of how we can leverage a data provider to ensure all tests run even if one fails.
|
Code updated with changes requested. Thanks for your reviews @tellthemachines , @costdev ! 🙇 |
|
This looks much better now. Thanks, @c4rl0sbr4v0 and @costdev! 🙏 |
Co-authored-by: Tonya Mork <tonya.mork@automattic.com>
|
== Patch Report Patch tested: https://patch-diff.githubusercontent.com/raw/WordPress/wordpress-develop/pull/5528.diff How to read this report: ==== Environment
==== Steps to Reproduce
==== Steps to Test Patch
==== Test Results
==== Screenshots After patch: |
src/wp-includes/functions.php
Outdated
| $id_counters[ $prefix ] = 0; | ||
| } | ||
|
|
||
| return $prefix . (string) ++$id_counters[ $prefix ]; |
There was a problem hiding this comment.
nit: I find the incrementing inside the return to be a bit harder to read. I wonder if it might be better to increment and then return.
There was a problem hiding this comment.
It's consistent with wp_unique_id(). That said, I thought the same thing : separate the increment from the string build/return.
There was a problem hiding this comment.
Maybe we can refactor both in 6.5, as it is code quality. I can leave the PR ready after the RC, pointing trunk branch.
There was a problem hiding this comment.
I think what makes it less readable as compared to wp_unique_id() is the array lookup. To help, split the increment in 11f1cac.
The wp_unique_prefixed_id() function uses a function scoped static variable for memoization. The incremented value may be different between tests, especially over time when more tests are added. Running the tests in separate process improves the stability of these tests.
| 'prefix as (string) "1"' => array( | ||
| 'prefix' => '1', | ||
| 'expected' => array( '11', '12' ), | ||
| ), |
There was a problem hiding this comment.
As wp_unique_prefixed_id() is a general function not specific to block names, it's possible that an ID may contain . and a number of other characters. Reference.
It's also possible that the "id" in question does not link directly to the HTML id attribute`.
Coverage should also be added for a variety of possible "id" values, such as (string) "0.0", etc. to protect behavior as this general function could be used to generate unique IDs for a variety of purposes/accepted formats.
There was a problem hiding this comment.
Sure that can be added, though would be part of the prefix, and not the incremented integer value.
There was a problem hiding this comment.
Thanks Tonya!
If there are any other potential "gotcha" values that a potential future refactor could regress, we should consider those too. The non-string dataset looks good for "gotcha" values, though I think we should complete the scalar datasets with (float) 0.0 and (float) 1.0.
| * @param string $expected_message Expected notice message. | ||
| * @param string $expected_value Expected unique ID. | ||
| */ | ||
| public function test_should_raise_notice_and_use_empty_string_prefix_when_nonstring_given( $non_string_prefix, $expected_message, $expected_value ) { |
There was a problem hiding this comment.
I think this can be a post commit cleanup, but If this test is ever run before test_should_create_unique_prefixed_ids then they will fail.
There was a problem hiding this comment.
Yes, the order of the tests within the class does currently matter. The tests in this class runs in a separate process to avoid a situation where the static$id_counters already has values in it.
To handle the test run order within the class, could change that to run each test in a separate process. By doing so, the static $id_counters will always be reset to an empty array when each test starts.
There was a problem hiding this comment.
Commit 5aac204 runs each test dataset in a separate process to avoid test order concerns,
There was a problem hiding this comment.
I also tweaked the unhappy path tests to ensure the IDs remain unique f4f99fc
How? By allowing each dataset to determine how many unique IDs to generate.
Why? Each of these non-string prefix datasets would have the same ID, as the prefix is omitted (empty string). One more check just to make sure.
To avoid test order, changes to run each dataset in a separate process, which each test starts with the static `$id_counters` equal to an empty array.
Unhappy path tests have non-string prefixes, which will be changed to the default empty string. To further check uniqueness within the dataset's expected results, the unhappy path test now allows each dataset to specify how many unique IDs to generate for its tests.
|
@aaronjorbin @costdev all feedback is now addressed. Can you re-review please? |
THis test guards against a future regression should the defensive guard ever change. Props @costdev.
costdev
left a comment
costdev
left a comment
There was a problem hiding this comment.
Thanks for the updates @luisherranz and @hellofromtonya! LGTM! 👍
hellofromtonya
left a comment
hellofromtonya
left a comment
There was a problem hiding this comment.
LGTM 👍 should be ready for commit
|
Thank you everyone! Luis has asked me to commit this, which I'll promptly do 😊 |
|
Thank you all for your help getting this to the finish line! @ockham, @c4rl0sbr4v0, @hellofromtonya, @aaronjorbin and @costdev 🙏 |
8 participants
This is a backport PR for WordPress 6.4 that includes the following PHP Gutenberg changes:
Trac ticket: https://core.trac.wordpress.org/ticket/59681
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.