Core Data: Resolve entity collection user permissions #64504
Merged
Conversation
This file contains 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
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
packages/core-data/src/resolvers.js
Outdated
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 }, | ||
| ] ); | ||
| } | ||
| } |
There was a problem hiding this comment.
A bit crude, but it does its job 😄
|
@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 ) { |
There was a problem hiding this comment.
Any tests for this? When this should get into core, there need to be tests.
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.
There was a problem hiding this comment.
I pushed a commit with some tests.
youknowriad
reviewed
Aug 20, 2024
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' ); |
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
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.
There was a problem hiding this comment.
A low priority sounds like a good compromise. Hopefully, there will be little to no overlap.
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.
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
| * @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 ) { |
There was a problem hiding this comment.
Is there a good reason to keep these methods static?
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(); |
There was a problem hiding this comment.
If this method is not static, $server can literally be $this, no?
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 😅
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().
|
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
spacedmonkey
approved these changes
Aug 26, 2024
|
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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
