Load the entities list using the view context #37685
Conversation
youknowriad
added
[Package] Core data
youknowriad
force-pushed
the
update/entities-fetch
branch
from
1dcb914 to
4672ea0
Compare
|
Size Change: -8 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
|
If I'm not wrong, the label change (plural instead of singular) mostly impacts the multi-entity save panel where it's going to show things like "Posts" instead of "Post" as title. I think it's probably a fine tradeoff for an important fix. If this proves to be something folks are not ok with, we can consider finding a way to access singular labels for users without "edit" context rights. WDYT? |
youknowriad
added
the
Backport to WP 6.7 Beta/RC
Also: the name of the tab in the block sidebar (SettingsHeader) and the breadcrumbs. The proposal makes sense to me, |
|
Yeh sorry got distracted last week. Let me take a look! |
There was a problem hiding this comment.
In general I couldn't see any major UI bugs resulting from this change. However, I'm sure when I surfaced the issue (and when we chatted) there were other knock on effects for lower permission users such as allowing you to attempt edit requests to entities you cannot edit. Going to dig a little deeper.
The UI continued also continued to function for lower permission users.
Context
I flagged this issue to Riad because I noticed that (on trunk) getEntityRecord will not allow for reading CPTs even if the user has permission to view.
This is because the Types API endpoint only returns those posts with edit permission if you provide the context=edit.
As a result this line of getEntityRecord will short circuit any request by lower permission users to read any post that is not returned by the Types endpoint (even if you pass the context=view in your call to getEntityRecord):
gutenberg/packages/core-data/src/resolvers.js
Lines 55 to 59 in c9ba458
Note that the following line makes a request to the Types endpoint with the context set to edit.
This PR changes that so that we make the same request but using the view context. Therefore we get all readable Types back which allows us to read Post Types using getEntityRecord.
Singular vs Plural Lables
@mcsf mentioned the labels in the multi-entity panel are now plural rather than singular. To me that doesn't seem so bad.
However, it could be good to amend the <EntityTypeList> component to use the singular / plural version of the Post label depending on the length of the results. That would however, require amending the Types API as suggested in my review comments. The wp_template already does this manually
| label: postType.labels.singular_name, | ||
| label: postType.name, |
There was a problem hiding this comment.
This change is going to be difficult to test and the results could be unexpected (although likely relatively minor).
Ideally we would modify the Types API endpoint to provide access to the singular label as that will avoid this change cause knock on effects to UI labels (some of which may be handled in 3rd party plugins).
|
In my testing, I was getting a auth error see screenshot. Steps to replicate.
The reason you are getting the auth error here, is the context of edit is being passed to endpoint where the user do not have access to the edit the entities. I noted all of this here. This line needs to change gutenberg/packages/core-data/src/entities.js Line 217 in 812fa1b Based on if the user do / does not have access edit the entity or not. Solutions.
I recommend option 2. Willing to work on the PHP if required. |
|
A new API to solve that particular navigation issue is not necessary, we can already pass "context: view" in specific |
I'm also curious about this. @spacedmonkey, do you happen to know why labels are limited to |
What new api are you talking about here? I don't see reference to a new api anywhere in this thread.
This data isn't public as it is only shown in the admin. But don't see the harm in showing in the view context. |
|
@youknowriad Hey, I have created a breakout PR, with improved handling of context, if user does not have permission to access the post type or taxonomy. #37838 |
I'm talking about adding a "defaultContext" information. We don't really need it. We can just pass the context to the |
|
To summarize:
I think Jonny is on to something here. Riad is correct that we can manually pass a If a developer wants to override that on a case by case basis then that's still fine, but in general the data API should pick the correct defaults.
I believe it would be this one |
|
"defaultContext" is very weird for me. What is the default context for any entity if not "view"? why does it need to be a property. I disagree with the fact that we don't know the context we want on the component level, we know that we want to show a list of pages and not edit them, so that should be "view" context, we know that we want to edit a page, that should be "edit" context...
so changing this to |
What I think you're saying is that as a developer we should know what type of request is being made - are we looking to read or write the data. Currently we default to As a result, we wouldn't need to include the default context because we can simply default to the lowest permission (i.e.
Happy to spin that one up. |
yes, the problem here is that the data package has been built originally for the post editor (editing posts) so we defaulted to edit and now we're stuck with this forever (backward compatibility) |
...oh. So you're saying that in fact we cannot default to Could we introduce some new APIs such as |
|
Sadly we can't just default to using
Not having title.raw or content.raw would break things down stream, as this at the fields that are needed to used to editor. Without these fields, you can not edit the raw values. Title and content returned rendered version, which is useless in editing context.
To be clear, this is not a new api, this is just a new field in two existing apis. See #37838. I too don't love adding a new field, but it is being done for backwards compatibility reasons. This way, all the entities that currect are listed with context=edit, remain the same and entities that user can only view, are added to this list with the context of view. This fixes the issue without breaking / changing very much. Please take the time to test my PR and understand why I have requested this change. |
I think the current workaround (pass "context": "view") when we only need "view" fields and we want access for contributors is good enough IMO |
|
What about ?
|
The root API doesn't change |
|
@youknowriad If that is the case, then why am I seeing this error
|
|
@spacedmonkey I didn't touch the navigation blocks |
|
It's difficult (at least for me) to discuss this in abstract so perhaps that will help. I will get to that shortly spinning off this PR as a base. |
|
Hi! 👋 Friendly nudge that today (Monday) is the last day for this PR to be merged if it is to ship in WP 5.9. I'll begin backporting merged PRs on Tuesday morning Australian Eastern time. |
youknowriad
removed
the
Backport to WP 6.7 Beta/RC
|
I'm not entirely sure this needs backporting, I'll leave it to you all (Navigation block experts) but I feel like if we can wait it's better (due to the labels change). |
Let me check this out now - I'll report back with a view shortly. |
|
I don't think we should backport this PR. For two reasons. Firstly, the problem it's intended to unblock is currently a non-starter because we now no longer allow lower permission users to create menus from classic Menus via the Navigation block. This is because that requires the ability to create Secondly, this is a potentially major change and we're in RC. We should probably avoid unless it's critical - as Im sure you all agree. So let's leave it out of WP 5.9. |
See #37489
Some users have access to "view" entities but not to "edit" theme. These users should be able to use the "core-data" package to fetch these entities. This is not possible sometimes in trunk because the way we "discover" entities is by making an "edit" context request to the
/typesendpoint.I think it makes more sense to use the "view" context to discover entities instead. That said, right now we rely on the "singular" label that is not returned in the "view" context. We have a number of options:
singular label(we already have the "name", we could add a "singlularName") or the entirely "labels" property to the returned value of the "view" context. Not sure if the REST team discussed this.In this current PR, I'm trying the first approach to check the results.