Repository navigation
REST API: Add author filtering to autosave collections. - #7555
adamsilverstein wants to merge 26 commits into
Conversation
|
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
WordPress stores at most one autosave per author, and the block editor only ever reads the current user's, so returning every autosave for the post makes the editor download a collection to discard all but one row of it. Register an `author` parameter on the autosaves collection, shaped like the one on the posts controller, and answer it with `wp_get_post_autosave()` for each requested id. This replaces the `per_page` parameter tried earlier, which narrowed the revision query before autosaves were filtered out of it and so could return nothing at all on a post with revision history. The block editor preload is scoped to match the path the editor requests, rather than removed: the preload cache is keyed on the path, so an entry the client never asks for is served to nobody. See #62057.
…or param. The new author collection param on the autosaves endpoints changes the REST schema, so the committed QUnit fixture was stale and every single-site PHPUnit job failed the version-controlled files check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012XGNSFgvkHJVapMFkJo3VU
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DxbbMKz3sa741SjHuE1ZLS
The block editor still requests /autosaves?context=edit without an author, so an author scoped preload is never consumed: it adds a request on every load and triggers an unconsumed preload warning. Keep trunk's preload until the client change lands in core. See #62057.
Running wp_get_post_autosave() once per requested author made the query count grow with the size of the author list and returned duplicates when an ID was repeated. A single author__in query sorted by date and ID, newest first, matches the ordering of the unscoped path, and the result is reduced to one autosave per author. See #62057.
WP_Query ignores an author of 0 and treats negative IDs as exclusions, so author=0 or author=-1 would return other users' autosaves instead of narrowing the result. See #62057.
Adds coverage for author IDs below 1, repeated IDs, ordering parity with the unscoped collection and the inherited parameter on the template autosaves route, and corrects a misleading comment on the extra test users. See #62057.
The ordering and newer-revision tests relied on records created in sequence, so they mostly exercised the ID tiebreaker. Set dates that disagree with ID order, and cover an author with two stored autosaves so the newest-record deduplication runs against real duplicates. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012aBHgCnrQQ5WWMiPZjtZK1
| 'post_status' => 'inherit', | ||
| 'post_parent' => $parent_id, | ||
| 'name' => $parent_id . '-autosave-v1', | ||
| 'author__in' => wp_parse_id_list( $request['author'] ), |
There was a problem hiding this comment.
Is wp_parse_id_list() needed?
Is not author already guaranteed to be a list of positive integers per the schema?
| 'author__in' => wp_parse_id_list( $request['author'] ), | |
| 'author__in' => $request['author'], |
| $seen_authors = array(); | ||
|
|
||
| foreach ( $autosaves as $autosave ) { | ||
| if ( isset( $seen_authors[ $autosave->post_author ] ) ) { |
There was a problem hiding this comment.
A hash lookup is surely overkill and likely less performant than just a plain array, considering that it is highly unlikely there is ever going to be more than a handful of revisions here, right?
| if ( isset( $seen_authors[ $autosave->post_author ] ) ) { | |
| if ( in_array( $autosave->post_author, $seen_authors, true ) ) { |
| continue; | ||
| } | ||
|
|
||
| $seen_authors[ $autosave->post_author ] = true; |
There was a problem hiding this comment.
| $seen_authors[ $autosave->post_author ] = true; | |
| $seen_authors[] = $autosave->post_author; |
The schema already sanitizes `author` to a list of positive integers, so running it through wp_parse_id_list() again was redundant. The seen-author check now uses a plain array, since an author scoped query only ever holds a handful of autosaves and a hash lookup buys nothing at that size. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EzTaxRNEc1fpMMaLJWcNyd
westonruter
left a comment
There was a problem hiding this comment.
The following PHPStan errors remain, specifically in the tests:
------ ---------------------------------------------------------------------------------------------------------------------------------------------------
Line tests/phpunit/tests/rest-api/rest-autosaves-controller.php
------ ---------------------------------------------------------------------------------------------------------------------------------------------------
27 Property WP_Test_REST_Autosaves_Controller::$extra_users type has no value type specified in iterable type array.
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
🪪 missingType.iterableValue
132 Parameter #1 $user_id of static method WP_UnitTestCase_Base::delete_user() expects int, mixed given.
🪪 argument.type
990 Parameter #1 $id of function wp_set_current_user expects int|null, mixed given.
🪪 argument.type
991 Possibly invalid array key type mixed.
🪪 offsetAccess.invalidOffset
1000 Method WP_Test_REST_Autosaves_Controller::create_autosaves_for_extra_users() should return array<int> but returns array<int|WP_Error>.
🪪 return.type
1010 Method WP_Test_REST_Autosaves_Controller::set_post_date() has no return type specified.
🪪 missingType.return
1013 Cannot call method update() on mixed.
🪪 method.nonObject
1014 Cannot access property $posts on mixed.
🪪 property.nonObject
1037 Parameter #2 $haystack of method PHPUnit\Framework\Assert::assertCount() expects Countable|iterable, mixed given.
🪪 argument.type
1059 Parameter #2 $haystack of method PHPUnit\Framework\Assert::assertCount() expects Countable|iterable, mixed given.
🪪 argument.type
1060 Cannot access offset 'id' on mixed.
🪪 offsetAccess.nonOffsetAccessible
1060 Cannot access offset 0 on mixed.
🪪 offsetAccess.nonOffsetAccessible
1060 Possibly invalid array key type mixed.
🪪 offsetAccess.invalidOffset
1061 Cannot access offset 'author' on mixed.
🪪 offsetAccess.nonOffsetAccessible
1061 Cannot access offset 0 on mixed.
🪪 offsetAccess.nonOffsetAccessible
1080 Parameter #2 $haystack of method PHPUnit\Framework\Assert::assertCount() expects Countable|iterable, mixed given.
🪪 argument.type
1082 Possibly invalid array key type mixed.
🪪 offsetAccess.invalidOffset
1082 Possibly invalid array key type mixed.
🪪 offsetAccess.invalidOffset
1083 Parameter #1 $input_list of function wp_list_pluck expects array, mixed given.
🪪 argument.type
1127 Cannot access property $ID on int|WP_Post|false.
🪪 property.nonObject
1135 Parameter #2 $haystack of method PHPUnit\Framework\Assert::assertCount() expects Countable|iterable, mixed given.
🪪 argument.type
1136 Cannot access offset 'id' on mixed.
🪪 offsetAccess.nonOffsetAccessible
1136 Cannot access offset 0 on mixed.
🪪 offsetAccess.nonOffsetAccessible
1136 Possibly invalid array key type mixed.
🪪 offsetAccess.invalidOffset
1189 Parameter #2 $haystack of method PHPUnit\Framework\Assert::assertCount() expects Countable|iterable, mixed given.
🪪 argument.type
1190 Cannot access offset 'id' on mixed.
🪪 offsetAccess.nonOffsetAccessible
1190 Cannot access offset 0 on mixed.
🪪 offsetAccess.nonOffsetAccessible
1190 Possibly invalid array key type mixed.
🪪 offsetAccess.invalidOffset
1210 Parameter #1 $arr1 of function array_intersect expects an array of values castable to string, array given.
🪪 argument.type
1210 Parameter #1 $input_list of function wp_list_pluck expects array, mixed given.
🪪 argument.type
1217 Parameter #1 $input_list of function wp_list_pluck expects array, mixed given.
🪪 argument.type
1239 Parameter #1 $id of function wp_set_current_user expects int|null, mixed given.
🪪 argument.type
1261 Parameter #1 $post_id of method WP_Test_REST_Autosaves_Controller::set_post_date() expects int, int|WP_Error given.
🪪 argument.type
1271 Parameter #2 $haystack of method PHPUnit\Framework\Assert::assertCount() expects Countable|iterable, mixed given.
🪪 argument.type
1272 Cannot access offset 'id' on mixed.
🪪 offsetAccess.nonOffsetAccessible
1272 Cannot access offset 0 on mixed.
🪪 offsetAccess.nonOffsetAccessible
------ ---------------------------------------------------------------------------------------------------------------------------------------------------
------ ---------------------------------------------------------------------------------------------------------------------------------------------------
Line tests/phpunit/tests/rest-api/wpRestTemplateAutosavesController.php
------ ---------------------------------------------------------------------------------------------------------------------------------------------------
921 Parameter #2 $haystack of method PHPUnit\Framework\Assert::assertCount() expects Countable|iterable, mixed given.
🪪 argument.type
922 Cannot access offset 'wp_id' on mixed.
🪪 offsetAccess.nonOffsetAccessible
922 Cannot access offset 0 on mixed.
🪪 offsetAccess.nonOffsetAccessible
------ ---------------------------------------------------------------------------------------------------------------------------------------------------
[ERROR] Found 39 errors on changed lines (base: trunk)
| if ( in_array( $autosave->post_author, $seen_authors, true ) ) { | ||
| continue; | ||
| } | ||
|
|
There was a problem hiding this comment.
Micronit: This blank line separates two bits of PHP code which are part of the same “paragraph”.
| * | ||
| * @return int[] Author id keyed to the autosave id created for them. | ||
| */ | ||
| private function create_autosaves_for_extra_users() { |
There was a problem hiding this comment.
| private function create_autosaves_for_extra_users() { | |
| private function create_autosaves_for_extra_users(): array { |
| * @param int $post_id Post ID. | ||
| * @param string $date Date in MySQL format. | ||
| */ | ||
| private function set_post_date( $post_id, $date ) { |
There was a problem hiding this comment.
| private function set_post_date( $post_id, $date ) { | |
| private function set_post_date( int $post_id, string $date ): void { |
| ); | ||
|
|
||
| $revisions = wp_get_post_revisions( self::$post_id, array( 'check_enabled' => false ) ); | ||
| $this->set_post_date( reset( $revisions )->ID, '2030-01-01 00:00:00' ); |
There was a problem hiding this comment.
| $this->set_post_date( reset( $revisions )->ID, '2030-01-01 00:00:00' ); | |
| $this->set_post_date( array_first( $revisions )->ID, '2030-01-01 00:00:00' ); |
| ) | ||
| ); | ||
|
|
||
| $revisions = wp_get_post_revisions( self::$post_id, array( 'check_enabled' => false ) ); |
There was a problem hiding this comment.
| $revisions = wp_get_post_revisions( self::$post_id, array( 'check_enabled' => false ) ); | |
| $revisions = wp_get_post_revisions( self::$post_id, array( 'check_enabled' => false ) ); | |
| $this->assertCount( 1, $revisions ); |
| wp_update_post( | ||
| array( | ||
| 'ID' => self::$post_id, | ||
| 'post_content' => 'A revision made after every autosave.', | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Might want to assert that this returns true.
| * @return array[] | ||
| */ | ||
| public function data_get_items_rejects_invalid_author() { |
There was a problem hiding this comment.
| * @return array[] | |
| */ | |
| public function data_get_items_rejects_invalid_author() { | |
| * @return array<non-falsy-string, array<mixed>> | |
| */ | |
| public function data_get_items_rejects_invalid_author(): array { |
| * @return array[] | ||
| */ | ||
| public function data_get_items_with_several_autosaves_for_one_author() { | ||
| return array( |
There was a problem hiding this comment.
| * @return array[] | |
| */ | |
| public function data_get_items_with_several_autosaves_for_one_author() { | |
| return array( | |
| * @return array<non-falsy-string, array{ non-falsy-string, non-falsy-string, non-negative-int }> | |
| */ | |
| public function data_get_items_with_several_autosaves_for_one_author(): array { | |
| return array( |
| * @param string $second_date Date of the autosave created second. | ||
| * @param int $expected Index of the autosave expected to be returned. | ||
| */ | ||
| public function test_get_items_with_several_autosaves_for_one_author( $first_date, $second_date, $expected ) { |
There was a problem hiding this comment.
| public function test_get_items_with_several_autosaves_for_one_author( $first_date, $second_date, $expected ) { | |
| public function test_get_items_with_several_autosaves_for_one_author( string $first_date, string $second_date, int $expected ) { |
| * @param string $first_date Date of the autosave created first. | ||
| * @param string $second_date Date of the autosave created second. | ||
| * @param int $expected Index of the autosave expected to be returned. |
There was a problem hiding this comment.
| * @param string $first_date Date of the autosave created first. | |
| * @param string $second_date Date of the autosave created second. | |
| * @param int $expected Index of the autosave expected to be returned. | |
| * @param non-falsy-string $first_date Date of the autosave created first. | |
| * @param non-falsy-string $second_date Date of the autosave created second. | |
| * @param non-negative-int $expected Index of the autosave expected to be returned. |
Trac ticket: https://core.trac.wordpress.org/ticket/62057
This started as removing the autosave preload and adding
per_page; it has turned into a smaller, more targeted change.Claude rewrote this description to match where the patch ended up:
AI Use
Code and description both came out of a back and forth with 🤖 Claude Code. I will review and test.
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.