This is a followup to #2651716: Entity::getEntityFromRouteMatch() should support bundles, which added partial support for bundle-based entity routes : it only supports them when the bundle is part of the route path, but not when it is a constant parameter, because it only checks using $route_match->getRawParameter() instead of $route_match->getParameter().
Using $route_match->getRawParameter() is needed because some parameters are likely to be upcasted, but to support route-defined (vs request-defined) parameter values, a pair of extra calls to $route_match->getParameter() appears to be required.
TBD
TBD
Update issue summary
Code review
TBD
TBD
TBD
NA
| Comment | File | Size | Author |
|---|---|---|---|
| #31 | 2743303-31-entity-form-route-parameters.patch | 3.18 KB | tim bozeman |
| #31 | interdiff.txt | 645 bytes | tim bozeman |
| #28 | 2743303-28-entity-form-route-parameters.patch | 2.96 KB | tim bozeman |
| #28 | test-only.patch | 1.46 KB | tim bozeman |
| #27 | fix-existing-entity-form-test.patch | 687 bytes | tim bozeman |
Comments
Comment #2
fgmTesting initial simple patch to see where it goes. It fixes my current use case, but likely needs work.
Comment #3
fgmAdded related issue.
Comment #4
fgmOops, bad related issue number.
Comment #5
tstoecklerSo, are you talking about what you could get from
$route->getDefault($bundle_key)? If so, could we doelseif ($bundle = $route_match->getRouteObject()->getDefault($bundle_key))? I personally would find that a bit more intuitive.Comment #6
fgmIndeed, that works just as well in my case (just tested). I used getParameter() instead of going through the route object because it was more direct (just a call to the underlying attributes bag), but I have no specific reason to use one or the other.
Comment #7
tstoecklerSo what I was intending to say before, but I kind of forgot, was whether we can maybe drop the rather verbose if-check if we go through the route object? If so, that would be a reason to do it IMHO. If we still have to do the same checks then I would agree with #6 that the parameter bag is preferable, but I think it shouldn't be necessary.
Comment #8
fgmThe problem AIUI is that these values can be arrays or objects if the route was configured that way, whether one uses the route object or the parameter bag: this is probably an error, though: I don't think I've seen any example with non-scalar arguments in the route definition. I could not find any description of restrictions regarding the parameters, hence these checks.
Comment #9
tstoecklerSo you're basically saying that this could happen:
?
Yeah, not sure we should support that TBH. We already check if the entity has bundles in the first place and if it does and you want to fetch the bundle in some way without a route parameter, only then does that mean that you can't also have a route default that happens to be named like the entity type ID of your bundle entity type.
Comment #10
fgmExactly so. At some point, we had a policy of "not babysitting broken code", and indeed in most cases d8 core does not try to catch errors caused by ill-advised code (like the example in #9), so maybe we can just skip this test : since the data does not come from the user but from the module (unlike
rawParameters), there should not be a need for sanitizing these data.I think that this remains true whether using the data from getParameter() or the route object. In that case, getParameter() seems a possibly more natural choice, relying on the "result" of route matching instead of manipulating the route ourselves.
Rerolled patch without the sanitization. Also, I did not include it to keep the patch minimal, but I think it would be nice to use the same
$parameter = (...) ; $values[$bundle_key] = $parameter(or$parameter->id()in the first case) for the first two cases too.Comment #11
tstoecklerNote that the other two cases use
getRawParameter()in the if-check and then usegetParameter()in the "actual" code. I'm not sure why that is off the top of my head, but what you suggest would change that.This is also related to the above, but if we leave it this way (and if it actually works as intended) we need to document the usage of
getParameter()in the "actual" code and then only later in the if-check. That's not at all self-documenting.Comment #12
fgmRight, so it's probably for the better not to add that change.
Comment #22
tim bozeman commentedNice! This is really close to fixing another use case too.
How about a route like this? Where there is no slug, but a default is provided.
Calling
getParameter($bundle_entity_type_id)works swimmingly. The description says that we are callinggetRawParameter()in case the parameter is upcasted, but I think we can check if we have an entity and get the ID and support both uses.Oh, btw I think you'll need the patch from #3277784-2: copyRawVariables should support default route parameters too for that route to load.
Comment #24
tim bozeman commentedWhoops, forgot to define
$bundle. 🤦🏻♂️Comment #25
fgmGood improvements. I think you now need to add a couple tests for these changes, though.
Comment #26
tim bozeman commentedhmm, on closer look, aside from this issue entirely, the existing test doesn't seem to be covering the code we are talking about.
The part of
EntityForm->getEntityFromRouteMatch()inside ofif ($bundle_key = $entity_type->getKey('bundle') )isn't being tested because$bundle_keyis empty.Here is a tweak to get
EntityFormTestto test that bundles from route part.Comment #27
tim bozeman commentedWhoops, forgot the patch. 🤦🏻♂️
Comment #28
tim bozeman commentedHere is testing a route like the example above where the bundle is resolved from a default route parameter via
getParameters().Comment #31
tim bozeman commentedThis one too!
Comment #33
smustgrave commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
This could use an issue summary update. I started it but will need someone more familiar with the issue to fill the rest in.
Thanks.
Comment #34
tim bozeman commentedI don't think this is needed after applying #3277784-22: copyRawVariables should support default route parameters.