Problem/Motivation
Subissue of #2300677: JSON:API POST/PATCH support for fully validatable config entities
Given that full REST support requires for example config validation we limit the scope for now onto GET requests.
All what GET requests need is a way to serialize those entities, which is already given using the serializer API.
Proposed resolution
- Just provide GET routes for config entities, remove POST, PUT, DELETE, and PATCH routes which are available now but don't actually work.
- The permission logic will not change. It uses entity view access for all entities on GET. Since core config entities don't have separate view permissions core config entities will rely on admin access permissions. At least one config entity, field_storage_config, doesn't provide admin permissions so will effectively it will not be available via REST because there is currently no way to grant entity view access. There may be other config entities that have this problem.
- Fix a couple of places in the code where we assume fieldable entities, so just run it in those cases
- Since most configuration entities do not canonical links they will be available at the path 'entity/[ENTITY_TYPE]/[ENTITY_ID]'.
For example to retrieve the tags vocabulary use.
curl --user admin:admin --request GET "http://drupal.d8/entity/taxonomy_vocabulary/tags?_format=json"
- Configuration entities that do have canonical links, contact forms are the only example in core, will be available at their canonical link and not the path pattern above.
For instance to retrieve the Feedback contact form you can use:
curl --user admin:admin --request GET "http://drupal.d8/contact/feedback?_format=json"
Remaining tasks
None
User interface changes
None
API changes
Removed methods 'POST', 'PUT', 'DELETE', 'PATCH' from Config Entity resources. While this methods were available before this change they would result in fatal errors if you actually tried to use them.
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#46 | 2724823-46.patch | 17.57 KB | tedbow |
#46 | interdiff-2724823-44-46.txt | 1.77 KB | tedbow |
#44 | 2724823-44.patch | 17.53 KB | tedbow |
#44 | interdiff-2724823-41-44.txt | 3.36 KB | tedbow |
#41 | 2724823-41.patch | 17.95 KB | tedbow |
Comments
Comment #2
dawehnerCopying the existing patch file from the other issue
Comment #3
dawehner.
Comment #5
clemens.tolboomComment #6
tedbowJust got rid of debug line and some old commented out lines.
In \Drupal\rest\Tests\ReadTest::testRead we have
Is this still true? Is there any use in making a ConfigTest class in REST to test or property of the test configuration objects that make sure config entities are being returned correctly?
If not then we probably won't need ConfigTest class until we support updating them.
Comment #7
tedbowComment #8
clemens.tolboomWhat is this _core? Should we filter out '_' values?
Is it OK to expose default_config_hash?
Comment #9
clemens.tolboomUsing a non-admin user + basic auth I get access denied although having REST permission to view taxonomy is set.
The code checks for 'administer taxonomy' which make reading a vocabulary useless for getting the name of the vocabulary.
We need view permission for config entities.
Comment #10
clemens.tolboomThis [edit]post code[/edit] belongs not in this issue as this is about to view a config entity
Comment #11
dawehnerI would agree we should filter them out. They seem to be pretty internal.
Total good point! It feels like having this as a follow up seems to be a good idea. It conceptually doesn't belong into the issue we have.
Fair point.
Comment #12
tedbowAssigning to myself to work on the issues noted above.
Comment #13
tedbowWas looking into this. It seems weird to be filtering out different values.
I could see a use case you might want to default_config_hash externally.
What if you had a decoupled admin application that was maintaining multiple Drupal sites. If the application was keeping config entities on their side default_config_hash would be 1 way of keeping track of which config entities came from environments that were from the same install(dev, stage, prod of the same site).
Comment #14
tedbowRemoved post code from the patch
Comment #15
tedbowComment #16
clemens.tolboom@tedbow thanks for cleaning up a bit.
My #9 is not fixed. Do you have any idea?
Comment #17
tedbow@clemens.tolboom I agree with @dawehner in #11
We should make a follow up issue for exposing permissions for Config Entities once this is fixed
Comment #19
clemens.tolboomHmmm I miss read #11.
We need to make clear this patch only works for users with permission administer taxonomy [edit]or other permissions depending on request.[/edit]
Comment #20
Wim LeersI think this can be deleted? :)
Nit: we don't do this anymore since 8.2.
I don't understand this name at all.
It's meant to be another default route provider I think, but then it then omits "default" from the name unlike
\Drupal\Core\Entity\Routing\DefaultHtmlRouteProvider
.Even more confusingly, it's called "Empty". Why? It's not always empty.
Do we need this? AFAICT it's not used anywhere. I suspect it was used for methods other than GET, but that means this is out of scope here too.
Why not typehint to
ContentEntityTypeInterface
? I suspect I know why, but this should be explicitly documented.Comment #21
dawehnerFair question. The code in the next line though always deals with iterating over fields, so its something that is bound to fieldable entities. We should use the entities which are out there.
Well, the requirement here is to provide a canonical route. Here we provide an empty controller in order to not have to think about how to implement a view builder for a config entity.
Its empty in the sense of rendering nothing.
See before
Right, many of those changes have been bound to the idea of trying to support POST/PATCH etc.
Comment #22
tedbowHere is patch the removes the serialization_context logic because it is not need for read.
It also adds comments that explains EmptyRouteProvider
Comment #24
Wim Leers82 KB patch vs 13 KB before. Something unwanted snuck in there :)
Comment #25
tedbowWhoops!!!
Okay here is an updated patch.
Comment #26
Wim LeersThe reason we have this was given in #21:
Is it really impossible to get around this? Having this controller is very confusing/icky.
If we need this, then why do we have this in
EntityResource
:and this in
EntityDeriver
:Why can't we rely on those defaults?
Because we don't want
/entity/vocabulary/tags
, but we want/vocabulary/tags
instead? If so, we should be updating all of the config entities' annotations too, otherwise this just works forconfig_test
entities, which is less than helpful.This also indicates this is missing test coverage for config entity types other than
config_test
. Finally, this needs test coverage for a config entity type that does not have acanonical
route. Because having a canonical route is an opt-in thing, so we must verify the behavior for those config entity types that don't have it.Nit: missing trailing period.
Nit: Trailing space.
I'd rename this to
EmptyCanonicalRouteProvider
.Can we document why we're not checking
\Drupal\Core\Entity\ContentEntityInterface
? #21 contains the answer.Comment #27
dawehnerWhen I remember correctly the point is that all kind of other code reacts to the canonical link path being there. Stuff like config translation calls to
$entity->toUrl('canonical')
etc. , its not all about rest. There should be a test failure in the original issue about it: mh, the only one I could find is https://www.drupal.org/pift-ci-job/162268 but I remember having issues with stuff like that in the past.Comment #28
Wim LeersWhat are all those things then? And why is it not broken today?
So how can config translation work today then?
Comment #29
dawehnerWell, feel free to remove it, but I seriously added it not just for the fun of it, but rather based upon subconsciousness from a lot of experience with #2350509: Implement auto-route generation for all core entities and convert all of the core entities. and co.
Comment #30
Wim LeersHah! :) I don't think at all you added it for fun! But I think for this patch to be committable, we at least need to understand why canonical URLs for config entities are a necessity.
I can only speak for myself, of course. And I have to say that I personally don't understand why we need this. We don't seem to be using it. As I said #26.1, I don't understand why we're adding a canonical route for
config_test
, but not for any other entity type. That makes it look like this issue itself doesn't actually add read-only/GET support for config entities: it looks like this only adds support for readingconfig_test
config entities. That's a good step, but doesn't do what the issue title says.So, assuming that we indeed must add a canonical route for every single config entity type, that means:
I sincerely hope that I'm wrong on both counts.
I hope this clarified where I'm coming from.
I also just grepped #2350509: Implement auto-route generation for all core entities and convert all of the core entities. for comments mentioning config entities. And based on #2350509-178: Implement auto-route generation for all core entities and convert all of the core entities., #2350509-201: Implement auto-route generation for all core entities and convert all of the core entities. and #2350509-202: Implement auto-route generation for all core entities and convert all of the core entities., it doesn't look like there was even agreement on how to deal with
canonical
for config entities there. So it seems wrong to start introducing it here?Comment #31
dawehnerTotal fair point!
Well, this is why we have change records ... Let's assume we validate the canonical as part of rest.settings we could provide a super helpful exception, can't we?
Yeah its tricky. We have no concept of how to render a config entity, and well, don't render anything, seemed to be a good compromise in this issue.
Comment #32
Wim LeersSo, per #26-#31, this needs documentation, a change record and an issue summary update. I think mostly the same text can be shared among them.
Per #26.1, I think this needs tests to show how this behaves for a config entity type that doesn't have a
canonical
route.Comment #33
tedbowOk I removed the need in the test to have a canonical url for config entities.
This got rid of the EmtpyRouteProvider and updated ReadTest to handle entities without canonical urls.
So now config_test entity does not have a canonical url.
I have not addressed any other issues in this patch
Comment #34
dawehnerMh, so we change the expected behaviour in the test rather than expecting the behaviour which will happen on the actual site? This feels wrong, honestly. Won't have those entities actually have canonical link templates?
Comment #35
e0ipsoI'll make sure to follow up on #2733097: [FEATURE] Add GET support for configuration entities when this gets in. It feels that we may be able to reuse code/concepts from this patch.
Comment #36
tedbowUpdate the patch
This whole change that was being commented on shouldn't have gone in. It was a problem in how I read the original patch. It actually is checking for access to the user entity, $account, because that resource is NOT rest enabled. I updated the comment so that it will be more obvious.
It seems like this patch now works expect that it sounds like we are going to address the permission of viewing configuration entities in another follow up issue. So that would be mean if this patch got in only user with "administer [CONFIG ENTITY TYPE]" could actually use rest to View configuration entities
Comment #37
dawehnerWell, just because something is working doesn't mean something is ready . :)
Which, if you think about it, is kind of a legit behaviour. Most of the usecases are some form of special admin UI.
Adding a todo referencing #2164373: [META] Untie config validation from form validation — enables validatable Recipes, decoupled admin UIs … would be great I think
For me it still feels wrong, given that means that user level code cannot just use
$entity->toUrl()
, but people seem to not worry about it.Nitpick: The
$url->setRouteParameter($entity_type, $entity_id)
call could be moved inside the else.Comment #38
tedbowHere is patch that addresses #37 and also add taxonomy_vocabulary entity to ReadTest
I chatted with @dawehner on IRC about this. He was in favor of leaving the routes but adding validation saying methods other than GET aren't supported on config entities.
I this patch I overwrote availableMethods in EntityResource to remove these methods. To me this seems better because:
I intentionally left this outside the else because if the method is receiving a value for $entity_id to test creating a URL for an non-existing entity then you would want to override it in both cases. Setting it if you are just using $entity->id() also doesn't cause a problem.
Comment #39
Wim LeersI wrote this in #32:
The first thing is yet to be done. The second thing is effectively addressed. Although I personally would like to see proof that more config entities can be read, e.g.
block
,field_storage_config
,role
.I don't have any actual further complaints besides nitpicks. Leaving at NR.
Nit: s/get/GET/
Nit: s/view/viewing/, s/rest/REST/
IMO useless comment, because the prior comment already implies this.
Unnecessary change.
s/url/URL/
Not the format for the URL…
Single line, and s/id/ID/
s/URL/Url/
s/route not found exception/RouteNotFoundException/
Comment #40
tedbowJust noticed I didn't update this to use dependency injection. Fixing this and other issue in @Wim Leer's review
Comment #41
tedbowOk here is the patch that addresses @Wim Leers' review in #39. It also has 2 fails \Drupal\rest\Tests\ReadTest::testRead which I will explain.
I have added testing for these entities. We could add more but could do that as part of #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method
Adding field_storage_config ran into 2 problems.
array('_permission' => 'administer ' . $entity_type_id . ' fields'),
for field storage but _entity_access used for field config routes.This feels like a separate issue where FieldStorageAccessControlHandler would be introduced and \Drupal\field_ui\Routing\RouteSubscriber should updated to use it.
But I wanted to put this patch with failing test out there and get feedback. IMO we should remove field_storage_config from the entities to check because we know it will fail for none REST reasons, i.e. in \Drupal\rest\Plugin\rest\resource\EntityResource
is correct but fails because the problem mentioned.
This remains to be done.
Comment #43
Wim Leers+1! This makes sense.
Thanks for adding tests proving that it works for multiple config entity types. That raises the confidence we can have in this patch.
I took care of updating the documentation:
So, remaining tasks:
field_storage_config
from test coverage, out of scope change required for that one to workThis looks a bit wonky. But it's also what
\Drupal\Core\Entity\TypedData\EntityDataDefinition::getPropertyDefinitions()
does.Note that you can make this much simpler:
Comment #44
tedbowThis patch does that and simplifies \Drupal\rest\Plugin\rest\resource\EntityResource::isConfigEntityResource and \Drupal\rest\Tests\RESTTestBase::isConfigEntity according to @Wim Leers' suggestion
Thanks!
So, remaining tasks now
I hope to take care of those this afternoon.
Comment #45
Wim LeersTo me this patch looks ready.
Just one remaining nit, but keeping at
:Should typehint to the interface.
Comment #46
tedbowFixed type hints
Comment #47
tedbowComment #48
dawehnerOne could use the ConfigEntityTypeInterface but I really don't care.
Are those issues tags still up to date?
Comment #49
tedbowMainly for follow up I did quick check to see if there were other config entity types that are like field_storage_config in that they don't have an admin permission and don't have there own access controller. From my understanding standing this would mean they won't be available for REST GET.
Here are the ones I found:
I think the good news is there is only 7 out 30 for the modules I enabled in core.
field_storage_config would actually be important for a headless client because as it is now it is the only way to figure out if a field is multiple value or not
Comment #50
dawehnerWell, we have the other issue to figure that out ... so its really not something we have to care about.
I could imagine entity_view_display / entity_form_display being sort of helpful as well if you want to actually render it.
Comment #51
tedbowComment #52
tedbowComment #53
tedbowCreated change record https://www.drupal.org/node/2665276
and updated Issue Summary
Remaining tasks: none?!
Comment #54
dawehnerCool! Thank you!
Comment #55
alexpottCommitted 6a16932 and pushed to 8.2.x. Thanks!
Should be ID... fixed on commit.
Comment #57
Wim LeersYay!
Published https://www.drupal.org/node/2746015.
Updated the issue that was blocked on this: #2300677-74: JSON:API POST/PATCH support for fully validatable config entities.
Comment #59
Gábor HojtsyWas discussing this issue in light of #2135829: [PP-1] EntityResource: translations support. The way read support is exposed for config entities here if I am not mistaken is via their current loaded values which may be in the current translation, may contain environment specific overrides, domain overrides, etc. As per the discussion in #2135829: [PP-1] EntityResource: translations support, that makes the endpoints deal with different data based on which language prefix is present in the URL for example. Is that desired? Was that considered or should there be a followup for that?
Comment #60
effulgentsia CreditAttribution: effulgentsia at Acquia commented