Core Data: Support entities in the 'canUser' selector #63322
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
|
Size Change: +199 B (+0.01%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
Mamaduka
force-pushed
the
update/can-user-support-entities
branch
from
15462d4
to
a74fd72
Compare
Mamaduka
requested review from
ajitbohra,
nerrad,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
|
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. |
Mamaduka
requested review from
TimothyBJacobs,
a team,
mcsf and
aaronrobertshaw
and removed request for
juanmaguitar,
ajitbohra,
ryanwelcher,
nerrad,
ndiego,
fabiankaegy and
a team
Mamaduka
requested review from
youknowriad,
jsnajdr and
tyxla
and removed request for
mcsf
|
I've tested editing post meta bindings and post title and everything seems to be working as expected 🙂 Regarding the code, it looks good after a quick look, but I'll defer the review to other folks because I am not fully familiar with the use cases, and I don't have the full context. |
jsnajdr
reviewed
Jul 10, 2024
| const key = ( | ||
| typeof resource === 'object' | ||
| ? [ action, resource.kind, resource.name, resource.id ] | ||
| : [ action, resource, id ] |
There was a problem hiding this comment.
Let's do some consistency checks on the resource object, and also prevent conflicting keys. The following calls (some of them buggy) lead to the same key:
canUser( 'update', 'posts' );
canUser( 'update', { id: 'posts' } );
canUser( 'update', { name: 'posts' } );The key should have some prefix (ent:postType/post/1, res:posts/1) and also we should trigger failure if there is resource.id and no resource.name etc.
There was a problem hiding this comment.
Where is there a benefit for strings? It doesn't look like it's needed for memoization?
There was a problem hiding this comment.
I'm talking about the key that is used for lookup in Redux state: state.userPermissions[ key ]. That is a string. The resource parameter for canUser is another beast.
There was a problem hiding this comment.
I guess we can return null from the selector when the resource is an object, but the kind and name properties are missing. These two are required to make any entity queries. Resolve can bail early and not make a request.
I can add a warning or error, but these are rare in WP stores.
There was a problem hiding this comment.
Yes, that's what I meant by "failure", simply returning null 🙂 All we need is to avoid storing or returning the wrong data.
There was a problem hiding this comment.
It could also be useful to log an error so the developer knows about unexpected usage.
@Mamaduka I see you mentioned those are rare in stores, but at the same time, when debugging store errors one often has to go deep, exactly because errors will often be silent or swallowed.
There was a problem hiding this comment.
We could throw an error when the resource object is malformed, and the error will be accessible via getResolutionError. We're already doing something similar when action isn't supported.
I don't know the historical reason why resolvers/selectors don't log errors; my best guess is that people often report them as bugs. So basically, support overhead.
cc @youknowriad
There was a problem hiding this comment.
I'm not very happy about throwing an error from the selector. It's not forward compatible. It's likely that we will add a new resource type in the future (e.g., for checking capabilities on the site) and then the new code will be failing on an older WordPress. It's better to return false, that means you cannot do the thing you ask for on an older WP version, and your new feature is effectively disabled. Let's just log a warning to the console, so that the error is not completely swallowed.
There was a problem hiding this comment.
The error is thrown from resolver b1e5dfc.
I think returning false is the correct approach. Core does the same with current_user_can. Calling current_user_can( 'fly' ) will return false. It's up to the devs to pass the correct data. Otherwise, we'll have to pollute the codebase with warnings.
Fun fact: Calling current_user_can( 'exist' ) will always return true.
There was a problem hiding this comment.
The error is thrown from resolver
Oh, I missed that, in resolver it's fine.
Fun fact: Calling
current_user_can( 'exist' )will always return true.
It was added in 2010 here: https://core.trac.wordpress.org/ticket/14696#comment:5
Add an 'exist' capability that all users have, even if they don't have a role on a blog. current_user_can('exist'), yeah, cute. Suggestions welcome.
|
Flaky tests detected in 19e01ea. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9874496323
|
Mamaduka
commented
Jul 10, 2024
tyxla
reviewed
Jul 10, 2024
| config.name === resource.name && | ||
| config.kind === resource.kind | ||
| ); | ||
| if ( ! entityConfig ) { |
There was a problem hiding this comment.
Should we log an error in case kind or name don't correspond to an entity type?
There was a problem hiding this comment.
No other entity resolver does this; maybe we should re-evaluate missing config cases together?
| const key = ( | ||
| typeof resource === 'object' | ||
| ? [ action, resource.kind, resource.name, resource.id ] | ||
| : [ action, resource, id ] |
There was a problem hiding this comment.
It could also be useful to log an error so the developer knows about unexpected usage.
@Mamaduka I see you mentioned those are rare in stores, but at the same time, when debugging store errors one often has to go deep, exactly because errors will often be silent or swallowed.
|
I have resolved most of the feedback and to-do items. It should be ready for another review. |
Mamaduka
force-pushed
the
update/can-user-support-entities
branch
from
f87e919
to
df1dbd2
Compare
|
Thanks everyone for the reviews and feedback. I rebased the branch just in case and will merge after the CI checks are green. |
This was referenced Jul 11, 2024
Mamaduka
added
the
Needs Dev Note
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Jul 18, 2024
* Core Data: Support entities in the 'canUser' selector * Cleanup 'canUser' unit tests * Add unit tests * Replace 'canUserEditEntityRecord' selector usages * Make 'canUserEditEntityRecord' forwarded selector/resolver * Add checks when the resource is an entity * Return false when entity arg is malformed * Update types and JSDoc * Throw an error when an entity resource object is malformed * Deprecate canUserEditEntityRecord Co-authored-by: Mamaduka <[email protected]> Co-authored-by: ellatrix <[email protected]> Co-authored-by: jsnajdr <[email protected]> Co-authored-by: tyxla <[email protected]> Co-authored-by: SantosGuillamot <[email protected]> Co-authored-by: adamziel <[email protected]> Co-authored-by: TimothyBJacobs <[email protected]>
97 tasks
|
Hey @Mamaduka 👋 Would you be able to help write a dev note for this for the 6.7 release? We are planning to have this as part of a larger Miscellaneous Editor Updates note. We are hoping to get all drafts in by October 13th to leave some time for reviews before the RC1. All Dev Notes get tracked in #65784 so feel free to leave a note there or ping me directly :) Please let us know if you can assist with that. Thanks in advance :) |
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?
Resolves #43751.
Alternative to #63292.
PR updates the
canUserselector to support core data entities. The new supported and recommended syntax:Why?
canUser- currently only supports resources in thewp/v2namespace. Additionally, you must know the final REST API path. Typically, however, only an entity's kind and name are known.canUserEntityRecord- is limited to only the edit action check. It was a wrapper forcanUserand only supported thewp/v2namespace, which hasn't been required since WP 5.9.The update also opens up the possibility of leveraging "target hints" and bulk fetch permissions data for DataViews.
Todo
canUserEditEntityRecordselector.Testing Instructions
Basic Testing
Testing canUserEntityRecord changes
Testing Instructions for Keyboard
Same.
Screenshot