Fix Nav Block bug as Contributor user via fix to *loadPostTypeEntities
#18669
Conversation
getdave
added
[Type] Bug
getdave
requested review from
ajitbohra,
jorgefilipecosta,
Soean and
talldan
as code owners
| @@ -199,6 +199,7 @@ export default compose( [ | |||
| parent: 0, | |||
| order: 'asc', | |||
| orderby: 'id', | |||
| context: 'view', | |||
There was a problem hiding this comment.
We can't just switch to using the "view" context because it means we're having inconsistent entities format in the state depending on the caller. I think right now, we just can't use getEntityRecords. Instead, we should use apiFetch in these components directly. One way to make this easier and avoid repeating the same lifecycle/state handling for all apiFetch usage is to consider a useApiFetch hook.
const { data, status, error } = useApiFetch( { path: 'url' } )
There was a problem hiding this comment.
@youknowriad So using apiFetch directly and not updating the "Redux" state at all, only local component state?
Also even if we do use a more direct method to make the API request, the data will still be wrong unless we switch the context to view. Is this what you are recommending?
Are there any good examples (or documentation) for using apiFetch directly that I could reference?
There was a problem hiding this comment.
yes, local state + view context.
And API fetch is just a simple wrapper around wp.fetch to make it work in WP context https://developer.wordpress.org/block-editor/packages/packages-api-fetch/
There was a problem hiding this comment.
Looks like Latest Posts uses it
| return () => { | ||
| isMounted = false; | ||
| }; | ||
| }, [] ); |
There was a problem hiding this comment.
I assume any component using apiFetch will hhave the same state, the same effects (status, data, error), should we extract this into a reusable useApiFetch hook like suggested?
There was a problem hiding this comment.
Yes we could should but I wanted to check the basics were on track first before commiting additional effort.
Ill take this as thumbs up to proceed with the hook 😁
|
@youknowriad Looks like I'll need to be focused elsewhere for the next week so unlikely I'll get to this soon. Apologies. |
|
@getdave what's the status of this one? I think we should hide this block for contributors (possibly also for authors). |
tellthemachines
force-pushed
the
fix/nav-block-broken-contrib-role-getentity-records
branch
from
ad5b2da
to
1c47636
Compare
|
Size Change: +4.5 kB (0%) Total Size: 870 kB
ℹ️ View Unchanged
|
| @@ -0,0 +1,31 @@ | |||
| /** | |||
There was a problem hiding this comment.
I added the reusable apiFetch hook in the navigation folder for now as I'm not sure what the best place for it would be, or where it might be reused. Feedback appreciated!
There was a problem hiding this comment.
Hmm, I suppose if it went in the api-fetch package, react would have to be added as a dependency to that.
There was a problem hiding this comment.
What do you think, @youknowriad? Adding @wordpress/element as a dependency of @wordpress/api-fetch isn't totally out of the question since e.g. @wordpress/data depends on @wordpress/element.
There was a problem hiding this comment.
Yeah, maybe it's fine. It does increase bundle size if loaded on the frontend without react need but I don't have a better alternative here.
| @@ -134,7 +155,7 @@ function Navigation( { | |||
| selectBlock( clientId ); | |||
| } | |||
|
|
|||
| const hasPages = hasResolvedPages && pages && pages.length; | |||
| const hasPages = pages && pages.length; | |||
There was a problem hiding this comment.
I think best to cast this to a boolean so that there's no surprises down the road when someone uses hasPages expecting a boolean but gets e.g. a number.
| const hasPages = pages && pages.length; | |
| const hasPages = !! pages && !! pages.length; |
| // We've stopped fetching | ||
| setIsRequesting( false ); | ||
| } ); | ||
| }, [] ); |
There was a problem hiding this comment.
Right now this won't re-fetch if path changes. Is that right? It's probably a use case worth supporting for e.g. components where path is computed dynamically based on user input.
|
Pulling |
|
This works well when I test it locally! 👍 At first I thought switching to
I'd like #18669 (comment) and #18669 (comment) to be addressed and then this is a ✅ for me. |
| * @param {Function} setData Function to set data being returned, or empty array on error. | ||
| * @param {string} path Query path. | ||
| */ | ||
| export default function useApiFetch( setIsRequesting, setData, path ) { |
There was a problem hiding this comment.
Instead of setIsRequesting and setData why not an API like this:
const { isLoading, data, error } = useApiFetch( queryObject )
similar to https://github.com/marcin-piela/react-fetching-library
I've used this on a side project and it was very nice.
There was a problem hiding this comment.
I have a question. What if I need to make my fetch call cancelable? As far as I know the only way to do that is via something like this
How could I mirror that behaviour with this hook?
Perhaps something like
Although AbortController doesn't have IE11 support...
tellthemachines
force-pushed
the
fix/nav-block-broken-contrib-role-getentity-records
branch
from
33b8b9c
to
90f92fa
Compare
|
Hmm, I moved the hook to |
|
Might need to set it as a property of the default export. apiFetch.useApiFetch = useApiFetch;
...
export default apiFetch; |
| const [ error, setError ] = useState( null ); | ||
|
|
||
| useEffect( () => { | ||
| apiFetch( { path } ) |
There was a problem hiding this comment.
I might be wrong, but I'm wondering if the state should be reset prior to calling apiFetch by calling setIsLoading, setData and setError again with the initial values, as I think if the path changes, useState won't override any existing values being stored.
There was a problem hiding this comment.
Oooh good point, it probably won't because only the effect will re-run.
packages/api-fetch/src/index.js
Outdated
| export function useApiFetch( path ) { | ||
| // Indicate the fetching status | ||
| const [ isLoading, setIsLoading ] = useState( true ); | ||
| const [ data, setData ] = useState( [] ); |
There was a problem hiding this comment.
I noticed the data is referred to as dataList later on as well. The data returned from an API might also be an object, like when fetching an individual post. I think this could just be kept as an undefined initial value useState().
| const ref = useRef(); | ||
| const { selectBlock } = useDispatch( 'core/block-editor' ); | ||
|
|
||
| /* eslint-disable @wordpress/no-unused-vars-before-return */ |
There was a problem hiding this comment.
Do we find ourselves having to disable this often in components? If it's not a useful rule then we should just remove it...
(Not relevant to this PR!)
There was a problem hiding this comment.
The value of the rule is explained here:
Based on specific instances, we can choose to improve it as well. For example, it should probably be exempting hooks as a special circumstances, due to constraints imposed on us externally via Rules of Hooks.
There was a problem hiding this comment.
The value of the rule is explained here:
I should have also mentioned there was previously quite a few offending cases that were cleaned up as part of the introduction of the rule, in case that's a more convincing method for interpreting the value.
See: #12827
There was a problem hiding this comment.
It might not have been necessary to disable the rule here anyways? It was removed in #20884. If I recall correctly, the rule makes exceptions for object destructuring.
To the general problem:
we can choose to improve it as well. For example, it should probably be exempting hooks as a special circumstances, due to constraints imposed on us externally via Rules of Hooks.
This already exists as of #16737:
gutenberg/packages/eslint-plugin/configs/react.js
Lines 10 to 15 in 1f00c74
The problem here is that the hook is named as experimental, __experimentalUseColors. There are two ways we could accommodate for this:
- We can adopt a convention to reference the slot by its non-experimental name in the implementation of the component, renaming via the import (example)
- We can expand the pattern above to include
__experimentaland__unstablenaming conventions
noisysocks
deleted the
fix/nav-block-broken-contrib-role-getentity-records
branch
| * | ||
| * @param {string} path Query path. | ||
| */ | ||
| function useApiFetch( path ) { |
There was a problem hiding this comment.
By naming standards, this should be useAPIFetch.
|
My initial reaction here is that we should be encouraging to use core data entities and the data mechanisms consistently, and not to make arbitrary API requests from components. If there's insufficiencies in these patterns, we should aim to resolve those. I see there was a conversation early on at #18669 (comment) about how this is a particularly troublesome example of where we're unable to get data due to constraints of the REST API and its
|
| .then( ( fetchedData ) => { | ||
| setData( fetchedData ); | ||
| // We've stopped fetching | ||
| setIsLoading( false ); | ||
| } ) | ||
| .catch( ( err ) => { | ||
| setError( err ); | ||
| // We've stopped fetching | ||
| setIsLoading( false ); | ||
| } ); |
There was a problem hiding this comment.
There's no guarantee that the component is still mounted at the point we're calling setState here. If it was removed in the time between the request starting and its completion, the state setters will fail, and React will log a warning to the console.
See also: https://reactjs.org/docs/hooks-reference.html#cleaning-up-an-effect
There was a problem hiding this comment.
Can I also reinforce that this anti pattern is super easy to introduce. I wonder whether we could make the hook handle this edge cases automatically by:
- Wrapping the fetch Promise to allow to be cancelled.
- Automatically calling the
.cancel()on unmount. - Checking for
isCancelledstate in.then/.catchhandlers and exiting early ( to avoid state updates).
For an example see Automattic/jetpack#15228
There was a problem hiding this comment.
This point makes me think of the behaviour of resolvers in wp.data which often include making fetch requests. This same issue can surface there via usage of useSelect with selectors that resolve (and I think already is). We probably should be looking into that too.
There was a problem hiding this comment.
Maybe we could promote good behaviour1 here by having apiFetch return a function that cancels the request.
useEffect( () => {
setIsLoading( true );
setData( null );
setError( null );
const cancelFetch = apiFetch( { path } )
.then( ( fetchedData ) => {
setData( fetchedData );
// We've stopped fetching
setIsLoading( false );
} )
.catch( ( err ) => {
setError( err );
// We've stopped fetching
setIsLoading( false );
} );
return cancelFetch;
}, [ path ] );1: If it's easy to do the right thing, it's more likely that one will do the right thing 🙂
There was a problem hiding this comment.
I'm motivated toward anything which keeps apiFetch cumbersome to use in a component, if it means that the preferred core data entities are the more obvious solution by virtue of their ergonomics advantage. Which isn't to say there's not issues there as well (#18669 (comment)), but I think we're in a better position to solve those internal to those implementations, even if it's something like this.
|
@aduth: What do you suggest we do as a next step? |
|
@noisysocks In my view, I think the most immediate next step could be to simply move There are other points mentioned about async state updates where we should consider making improvements. I believe the specific implementation may be blocking @getdave in some way, though to me it's not entirely clear if his original implementation had migrated from core entities to apiFetch if there was some plans for how to reconcile this. |
|
@noisysocks @aduth @draganescu I'm thinking that we sholuld:
This will:
Any objections? |
Closes #18659
The Nav Block placeholder has a button for
Create from all Top Pages. To work the Block must have Page data available. If it isn't available then the button isdisabled.For higher Role users with greater permissions, this works absolutely fine. However, for the
Contributoruser the Pages data cannot be accessed. This appears to be due to the way we are accessing the Page data in our selectors/resolvers. Primarily the call togetEntityRecordssetscontexttoeditfor the Page post type REST API query. Lower user types don't have permission to access the endpoint under theeditcontext and so the request fails.The (new) direction for this PR is to use
apiFetchdirectly within the component to avoid the selectors entirely and make a request to the API. This ensures thePagesdata is local to the component and allows us to set thecontexttoviewto ensure Pages are returned for all user Roles.Screenshots
This shows me as a User with the
ContributorRole inserting the Block:More Detail
Ultimately this traces back to
loadPostTypeEntities.On
mastertheloadPostTypeEntitiesresolver is returning aWP_ErrorREST API response.The problem here is this line:
The usage of the
editcontext on the request URL is what is causing this. Try toggling the following request betweeneditandviewin the URL param:You will see a different response. You can do the same by
ContributorUser.contexttoview. You'll see all the Post Types returned.Adminuser and repeat from#2above. It works fine for this permission level.This is caused by the permissions check in the WP_REST_Post_Types_Controller REST API endpoint. Specifically
It appears that Contributor Role is not allowed
edit_postsaccess to thePagespost type. As a result the error is returned on this REST permission check. This is in turn because we are passing the?context=editparam in theapiFetchcall.Originally this PR tried to circumvent this by using the
viewcontext on the API request to retrieve the custom post type entities thereby avoiding the permissions check which seems unnecessary. But this was not a good option.How has this been tested?
Manually testing that the Nav Block now has access to Pages data.
Types of changes
Breaking change (fix or feature that would cause existing functionality to not work as expected).
Checklist: