This issue is a follow-up for #1798214: Upcast request arguments/attributes to full objects.
The aforementioned issue provides a nice solution for upcasting single, isolated parameters in a route. In some cases, this is not sufficient, though. For example, in a route that consists of two parameters '/{entity_type}/{entity}' (=> e.g '/node/1', '/user/1') we need to know the value of {entity_type} in order to properly upcast into an EntityInterface object. The patch provided in #1798214: Upcast request arguments/attributes to full objects does not account for that (yet). A possible solution would be to provide the relevant information for how to upcast {entity} through route options, e.g. provide the name of the {slug} that is the {entity_type} and then use that in a AnyEntityParamConverter. I am not sure if that is a proper approach for solving this but I believe that we should support upcasting for 'dynamic' params like this.
Comment | File | Size | Author |
---|---|---|---|
#16 | interdiff.txt | 965 bytes | dawehner |
#16 | 1837388-15.patch | 33.21 KB | dawehner |
#11 | interdiff.txt | 790 bytes | tim.plunkett |
#11 | entity_convert-1837388-11.patch | 32.52 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettThis has been needed in a couple places now.
Comment #2
dawehnerHow is that different from what we do have already: we do pass along $defaults already.
Comment #3
tstoecklerYes, let's provide such a param converter as is described in the issue summary.
Some generic mechanism might (or might not) be cool also, but something like upcasting /{entity_type}/{entity} will surely be required in contrib and let's not have 20 different contribs write that same paramconverter. So let's do that as a first step.
Comment #4
dawehnerHere is an example which works quite fine, to be honest.
Comment #6
tim.plunkettWorking on this, I found the initial problem but uncovered another.
Comment #7
Wim LeersI don't understand the IS well enough to be certain it's perfectly on-topic, but it's at least related:
{entity_type}/{entity}
indeed works… as long as there is a single entity in a route!Quick Edit has an example of multiple entities in a route:
{entity_type}/{entity}
is one,{view_mode_id}
is another. I should have been able to write{view_mode}
, but if I do that, there are two entities, and it blows up. Therefore I had to rename that parameter to{view_mode_id}
, and load the view mode entity manually.Comment #8
dawehnerWell, this is another problem space. The current code just cares about the actual upcasting logic itself, not about the super bonus magic which is done to make life easier for developers.
Comment #9
tim.plunkettIndeed, #7 is a separate (valid) issue.
With this change, EditEntityAccessCheck should be completely obsolete, and the
$request->attributes->set('entity', $entity);
in EditEntityFieldAccessCheck can be removed.In the interdiff, I fixed the failures, and improve EntityConverter by encapsulating the entity_type_id logic in a new method. This includes an exception if the dynamic entity type is invalid.
The rest was fixing or expanding test coverage.
Comment #10
Wim LeersOk — as I said, I didn't know if this would be on-topic, but it's related. It might be useful to keep this in mind while solving this issue. Or maybe not. No matter — carry on :)
Comment #11
tim.plunkettI *thought* that return NULL was wrong.
Comment #12
dawehnerThis looks really nice
Comment #13
Wim LeersHaving read this and re-read the IS, I finally understand what this issue is about! :)
Massive hurray for getting rid of all that custom code that Quick Edit had to introduce!
However… I would like to see at least *some* comment in here to explain what this does, because 99% of developers will not understand this I fear. Something like this would already be enough:
If I'm mistaken and this *is* clearly documented somewhere, then feel free to set this back to RTBC immediately.
In any case, thank you for making this so much cleaner and more maintainable!
Comment #14
dawehnerI'd rather suggest to document it inside the proper routing documentation on drupal.org
and maybe on the EntityConverter itself.
Comment #15
Wim LeersIf you mean https://api.drupal.org/api/drupal/core%21includes%21menu.inc/group/menu/..., that documentation lives in
core.api.php
, and hence should be updated in this patch.Or are you referring to other documentation?
Comment #16
dawehnerHere is a patch which includes some documentation.
Comment #17
dawehnerI talked about https://www.drupal.org/developing/api/8/routing
Comment #18
Wim LeersThat's already much better, thanks!
Comment #19
webchickI love patches like this. ;)
So I was initially nervous because the DX that was originally discussed in the beginning of this effort (not in this issue, but in some other issue I can't find atm) was scary as heck. However, this patch is extremely targeted and only requires some extra typing in case of ambiguity, which is fair enough. Omniscience will need to be a D9 feature. ;)
In addition to obliterating a lot of complex code in e.g. quickedit module that can now rely on standard core entity access-checking, etc. this also makes it so that various functions that act on entities take an EntityInterface versus a Request, and then trying to parse the entity info out of that, resulting in much more portable code.
The only thing that raised my hackles was:
strpos() makes baby kittens cry. ;( And unfortunately this is done in several places throughout the code.
However, in talking to Tim it seems like this is the only way to do it. The upcasted entity itself doesn't know whether it's dynamic or not, only the route definition can say this.
I also committed a small addition to the EntityConverter PHPDoc with tim's help that provides a bit more details on when you need to worry about this, since Drupal does the right thing in the 90% case.
Apart from that, no complaints. Ergo!
Committed and pushed to 8.x. Thanks!
Comment #21
tim.plunkettThanks! See #2326707: Use dynamic entity type upcasting for comment.reply for another usage of this.