Size Change: +690 B (0%)

Total Size: 1.24 MB

_{compressed-size-action}

The e2e failures are the same as in trunk. I think this is good to be merged as soon as anyone dares to review :D cc @noisysocks @jorgefilipecosta @jsnajdr @draganescu

@adamziel, I just pushed a minor fix when checking resources without an ID + minor refactor and unit tests for this new case.

Suppose anyone is looking for a way to test this fix manually. You can run the following snippet in the DevTools console and confirm that it only triggers one REST API request.

wp.data.select('core').canUser('read', 'template-parts');
wp.data.select('core').canUser('create', 'template-parts');

Yay @Mamaduka, thank you! It looks good and makes sense to me.

@Mamaduka I pushed one more bugfix and unit test. The site title block calls canUser( 'update', 'settings' ) which means that update and delete permissions needs to be stored regardless of whether the ID is passed.

Nice catch, @adamziel. The third-party code might also use update and delete without IDs for custom endpoints.

These may both be for another ticket.

Making an OPTIONS request isn't required to get the Allow header, any request to that entity will have the Allow header included in the response. It seems like we should be storing the headers of our entity fetch responses in core-data somewhere. That way a separate OPTIONS query would be avoidable if the item was already loaded.

When a request does need to be made, we should probably also explore a Core ticket around building specific HEAD responses. While an OPTIONS response is cheaper than a full GET, a proper HEAD should be even faster. Right now, the REST API server will just throw away the response body. But we could register a specific no-op HEAD callback, or potentially have a register_rest_route flag that would let the REST API server only call permission_callback and skip callback.

Why does canUser take a resource instead of a kind and name? It looks like this selector doesn't account for namespaces other than wp/v2 which means the behavior is potentially broken for CPTs and Taxonomies since 5.9. But it also means it can't be used for any non-core entities.

Making an OPTIONS request isn't required to get the Allow header, any request to that entity will have the Allow header included in the response. It seems like we should be storing the headers of our entity fetch responses in core-data somewhere. That way a separate OPTIONS query would be avoidable if the item was already loaded.

I think this is happening since #35527

When a request does need to be made, we should probably also explore a Core ticket around building specific HEAD responses. While an OPTIONS response is cheaper than a full GET, a proper HEAD should be even faster. Right now, the REST API server will just throw away the response body. But we could register a specific no-op HEAD callback, or potentially have a register_rest_route flag that would let the REST API server only call permission_callback and skip callback.

I like the HEAD request idea! It just worked for a few dummy requests I made, so here's a PR to see what will the CI checks look like: #43703

Why does canUser take a resource instead of a kind and name? It looks like this selector doesn't account for namespaces other than wp/v2 which means the behavior is potentially broken for CPTs and Taxonomies since 5.9. But it also means it can't be used for any non-core entities.

My first thought was that it takes a resource to check more than just entity records permissions, but then I didn't find any instances of that. Good call about supporting other namespaces – would you please create a separate ticket for that?

Actually, we already have canUserEditEntityRecord that works with kind and name. It's a tiny wrapper around canUser.

I think this is happening since #35527

Right, when the OPTIONS endpoint is preloaded, no fetch will happen. But in a situation where I have called getEntityRecord, and then want to present the user an option to delete or update it, that shouldn't need a separate apiFetch because the correct Allow header will have been returned when I called getEntityRecord. Similarly, if I fetch a collection response, I already have the Allow header to know if I can create resources for that collection.

Thanks @Mamaduka. I'm going to spin up a new ticket. That makes the DUX a bit nicer, but doesn't actually solve the issue of canUser using the hardcoded namespaces. It also will only work for post types, not other entitiy kinds.

I like the idea of not throwing away Allow headers when resolving getEntityRecord(s). @adamziel, want to team up on this PR? 😄

Another thing we should probably look into is combining canUserEditEntityRecord and canUser. Probably deprecate one.

@TimothyBJacobs, how can we help with adding HEAD request support to the REST API server?

I like the idea of not throwing away Allow headers when resolving getEntityRecord(s). @adamziel, want to team up on this PR? 😄

Sure – let's do that!

I'm going to spin up a new ticket. That makes the DUX a bit nicer, but doesn't actually solve the issue of canUser using the hardcoded namespaces. It also will only work for post types, not other entitiy kinds.

I've created #43751.

@TimothyBJacobs, how can we help with adding HEAD request support to the REST API server?

Core has built-in support, but I'm now less sure it is a good idea. Going to comment in #43703.

@Mamaduka here's two places to refactor to use the allow header we're already receiving via getEntityRecord(s):

The key is using the apiFetch's parse: false option:

wp.apiFetch({
   path: '...',
   method: 'HEAD',
   parse: false
})
   .then(response => response.headers.get('allow'))
   .then(console.log)

Would you like to start that PR?

Do you suggest making another API request with parse: false in getEntityRecord(s) resolvers?

I thought @TimothyBJacobs, the recommendation was to use the header from the GET request. But now I see that parse: true throws away everything besides the body.

I wonder if adding requests to these resolvers defeats the whole purpose of this optimization.

In any case, I can start on this next week, and we'll see how the results.

Do you suggest making another API request with parse: false in getEntityRecord(s) resolvers?

Not at all! I suggest making just one API request and make sure it uses parse: false so that we can process both the response body and the headers. Right now the parse: false isn't in place and the response headers aren't accessible. Let's add parse: false to the existing request and adjust the surrounding code accordingly.

I think that would break the current fetchAllMiddleware for entity records. Let's see if we can find a workaround 😃

I think the fetchAllMiddleware could be updated to work with parse: false. I presume it was omitted because we couldn't return headers for every nested request. But I think we could use the first response's headers safely.