Core Data: Resolve entity collection user permissions#64504
Merged
Conversation
Mamaduka
added
[Type] Enhancement
|
Flaky tests detected in 9fe7bd6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10398462601
|
Mamaduka
commented
Aug 14, 2024
|
Size Change: +1.18 kB (+0.07%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
Mamaduka
commented
Aug 15, 2024
Comment on lines
+328
to
+340
| for ( const targetHint of targetHints ) { | ||
| for ( const action of ALLOWED_RESOURCE_ACTIONS ) { | ||
| const permissionKey = getUserPermissionCacheKey( | ||
| action, | ||
| { kind, name, id: targetHint.id } | ||
| ); | ||
|
|
||
| dispatch.receiveUserPermission( | ||
| permissionKey, | ||
| targetHint.permissions[ action ] | ||
| ); | ||
| dispatch.finishResolution( 'canUser', [ | ||
| action, | ||
| { kind, name, id: targetHint.id }, | ||
| ] ); | ||
| } | ||
| } |
Member
Author
There was a problem hiding this comment.
A bit crude, but it does its job 😄
Member
Author
|
@WordPress/gutenberg-core, I think this is ready for early reviews/feedback 🙇 |
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
2 tasks
Mamaduka
force-pushed
the
try/target-hits-via-rest-server
branch
from
98e9287 to
357dfaf
Compare
swissspidy
reviewed
Aug 20, 2024
| * @param WP_REST_Response $response Response to extract links from. | ||
| * @return array Map of link relation to list of link hashes. | ||
| */ | ||
| public static function get_response_links( $response ) { |
Member
There was a problem hiding this comment.
Any tests for this? When this should get into core, there need to be tests.
Member
Author
There was a problem hiding this comment.
100%. I would like to do that, though I'm unsure what the best approach is. Maybe @TimothyBJacobs has some suggestions.
Member
There was a problem hiding this comment.
I pushed a commit with some tests.
youknowriad
reviewed
Aug 20, 2024
Contributor
youknowriad
left a comment
youknowriad
left a comment
There was a problem hiding this comment.
I like this approach a lot. Avoids all the extra requests. Would love some validation of the REST API endpoint changes though. I don't feel like my voice is enough for something that can be adopted for all endpoints.
youknowriad
approved these changes
Aug 20, 2024
tyxla
reviewed
Aug 20, 2024
Comment on lines
+107
to
+110
| function gutenberg_override_default_rest_server() { | ||
| return 'Gutenberg_REST_Server'; | ||
| } | ||
| add_filter( 'wp_rest_server_class', 'gutenberg_override_default_rest_server' ); |
Member
There was a problem hiding this comment.
Won't this break any pre-existing code that declares their own custom WP_REST_Server?
Example usage in existing plugin directory: https://wpdirectory.net/search/01J5R1B8MEEW01RJ3WZDC2XPY8
Member
Author
There was a problem hiding this comment.
The same is true for every REST API override in the Gutenberg plugin. Come with the project that builds a new WP in a plugin.
We can use a low-priority filter here (0 or 1) and prioritize other integrations, but this would mean not shipping enhancement for those plugins.
As far as I know, server overrides should be rare in plugins unless they're shipping their custom server.
Member
There was a problem hiding this comment.
A low priority sounds like a good compromise. Hopefully, there will be little to no overlap.
Member
Author
There was a problem hiding this comment.
I updated the hook priority. I also checked most of the plugins from the directory results, and none of them actually override the server class. But there will be some cases that aren't part of the plugin directory.
Member
There was a problem hiding this comment.
Better be safe than sorry. Nice to see that most of these plugin directory instances were actually tests.
tyxla
reviewed
Aug 20, 2024
Member
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
Haven't done extensive testing, just yet. Will do, but wanted to discuss something first.
My biggest concern is around overriding the default REST server, which some plugins already do, and by overriding it ourselves, we may potentially break those plugins.
| * @return array Map of link relation to list of link hashes. | ||
| */ | ||
| // @core-merge: Do not merge. The method is copied here to fix the inheritance issue. | ||
| public static function get_compact_response_links( $response ) { |
Member
There was a problem hiding this comment.
Is there a good reason to keep these methods static?
Member
Author
There was a problem hiding this comment.
I think they've been static since REST API got merged in the core. They need to remain static; otherwise, we'll break backward compatibility.
P.S. I had to copy two extra methods because of how the inheritance of self vs static keywords works. See: https://www.php.net/manual/en/language.oop5.late-static-bindings.php.
| return array(); | ||
| } | ||
|
|
||
| $server = rest_get_server(); |
Member
There was a problem hiding this comment.
If this method is not static, $server can literally be $this, no?
Member
Author
There was a problem hiding this comment.
I think we need an instantiated server here (which rest_get_server provides) and not a reference to the current class.
I copied this from WordPress/wordpress-develop#7139, so I might be bit wrong about reasoning 😅
Member
There was a problem hiding this comment.
Right, this is not a new method, but an overriede of an existing Core method which is static. So this needs to stay static, hence why we get access to the server via rest_get_server().
Member
Author
|
Thanks for adding the unit test, @TimothyBJacobs 🙇 I think this is ready to land. Can I get final approval? |
TimothyBJacobs
approved these changes
Aug 26, 2024
Member
TimothyBJacobs
left a comment
TimothyBJacobs
left a comment
There was a problem hiding this comment.
REST API changes look good to me.
spacedmonkey
approved these changes
Aug 26, 2024
Member
Author
|
It is nice to see clear performance improvements on the CodeVitals page.
|
bph
pushed a commit
to bph/gutenberg
that referenced
this pull request
Aug 31, 2024
Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: TimothyBJacobs <timothyblynjacobs@git.wordpress.org> Co-authored-by: swissspidy <swissspidy@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: spacedmonkey <spacedmonkey@git.wordpress.org>
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
Track: https://core.trac.wordpress.org/ticket/61739
Based on: WordPress/wordpress-develop#7139
PR updates
WP_REST_Severto add thetargetHintsproperty to the link description object as part of the self-link. ThetargetHintsprovides info about actions the user is able to perform for each item.It also updates the
getEntityRecordsresolver to bulk update user permissions for fetched collection items.Why?
Consumers who render a list of posts and CRUD actions related to each item must make a separate OPTIONS request to check if the user has permission to perform these actions.
The N+1 requests affect the performance on the client and the server.
Testing Instructions
wp-admin/site-editor.php?postType=page&layout=table.OPTIONSrequests.Testing Instructions for Keyboard
Same.
Screenshots or screencast
Data Views > Pages
Before
After
Data Views > Templates
Before
After
