Core Data: specify context=edit only when needed #30482
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
creativecoder
requested review from
vindl,
mattwiebe,
jeyip,
Addison-Stavlo,
Copons and
david-szabo97
This was referenced Apr 3, 2021
|
Size Change: +39 B (0%) Total Size: 1.47 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Apr 5, 2021
packages/core-data/src/entities.js
Outdated
| @@ -39,13 +39,15 @@ export const defaultEntities = [ | |||
| kind: 'root', | |||
| key: 'slug', | |||
| baseURL: '/wp/v2/types', | |||
| params: { context: 'edit' }, | |||
There was a problem hiding this comment.
Should this be something more like "defaultParams" or something like that? (Don't take my suggestion as a recommendation, I'm no good at naming things, just trying to raise a discussion point)
There was a problem hiding this comment.
Good question! I updated to URLparams and moved them all immediately below baseURL to try and make it clearer these are for constructing the endpoint URL.
creativecoder
requested review from
nerrad,
spacedmonkey and
TimothyBJacobs
as code owners
|
Looks like there's a valid test failure. The "types" endpoint does support the context argument. |
Copons
reviewed
Apr 6, 2021
There was a problem hiding this comment.
It all seems to work fine, but I'm a bit unclear how I'm supposed to test this. 🤔
I've tried to compare this branch to trunk, and everything looks exactly the same — with the exception of the URLparams property in the core state (added to the appropriate elements of root.entities.config).
Great point, @Copons. I made another update to the name of this:
@youknowriad I forgot to update the test entity object when changing the name of
@Copons One thing you can test is if the site icon (when set) shows in the upper left corner of the editor. With the filtering of Similarly, loading the editor fails (blank white screen) when the entity api paths don't match the critical preload paths. I know testing for "not failure" isn't ideal, but one thing you can do is verify that the entity urls generated by the resolver match those preload paths (just by console.log, if nothing else!)
|
|
This approach looks great to me. I haven't tested for regressions though. |
Copons
approved these changes
Apr 12, 2021
There was a problem hiding this comment.
Thanks for the pointers @creativecoder!
I already tested in roughly the same ways you suggested and it worked as expected; I also haven't noticed any regressions in either post and site editors, so it should be good to go! 🙂
creativecoder
force-pushed
the
add/core-data--entity-param-config
branch
from
e94c91d to
6825f8c
Compare
|
Note that I've added a new unit test for the Any other suggestions for regression testing this? Otherwise I think it's ready to merge. |
youknowriad
added
the
[Type] Bug
youknowriad
added
[Type] Code Quality
|
Does it require any changes in WordPress core? The list of preloaded paths is here: |
Good question! I don't think so: those existing preload paths match what core-data is using. A main goal of this PR was to remove the |
|
@creativecoder, nice. Thank you for checking 👍🏻 |
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.
Description
API requests for fetching entities currently add
context=editto all requests. Some endpoints do not use this parameter, which can cause unexpected problems, like the need to preload both the/and/?context=editendpoints (which both have exactly the same response).This change allows specifying query parameters specific to each entity, and adds
context=editonly to the entities that use that parameter.This resolves the (rejected) Core ticket (https://core.trac.wordpress.org/ticket/50606) and removes the associated filter for preloading
/?context=edit.Follows up on #22952.
How has this been tested?
/?context=editendpoint.context=editparameter is added only to the endpoints where it's a valid parameterYou can also compare resolver paths for entities with and without this branch applied, for example:
/?context=edit//wp/v2/settings/?context=edit/wp/v2/settings/wp/v2/types/post?context=edit/wp/v2/types/post?context=edit/wp/v2/templates/tt1-blocks//single?context=edit/wp/v2/templates/tt1-blocks//single?context=edit/wp/v2/__experimental/global-styles/26?context=edit/wp/v2/__experimental/global-styles/26?context=edit/wp/v2/media/10?context=edit/wp/v2/media/10?context=edit/wp/v2/taxonomies/category?context=edit/wp/v2/taxonomies/category?context=edit/wp/v2/taxonomies/post_tag?context=edit/wp/v2/taxonomies/post_tag?context=edit/wp/v2/users/1?context=edit/wp/v2/users/1?context=editNote how
/and/settingscorrectly no longer use?context=edit, but other endpoints still do.Screenshots
Site icon displayed in upper left corner:
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).