Problem
- When defining a route
$items['foo/%foo/bar/%bar'] = array 'load arguments' => array(1), );
then the expectation is that
foo_load()
is invoked with arg(1), andbar_load()
is invoked with arg(3) and the already loaded arg(1).That is, because I did not specify
array('1')
(string). I specifiedarray(1)
(integer).The menu system correctly loads the first dynamic argument currently, but fails to pass the loaded argument to the second loader.
The only (ugly) workaround right now is to specify '%map'
as load argument, and manually retrieve the loaded argument from the index.
This bug got revealed though #1668292: Move simplified Profile module into core, respectively #1726822: Port Profile2 to D8
Comment | File | Size | Author |
---|---|---|---|
#10 | drupal8.menu-load-objects.10.tests-only.patch | 2.71 KB | tstoeckler |
#10 | drupal8.menu-load-objects.10.patch | 5.54 KB | tstoeckler |
#10 | drupal8.menu-load-objects.10.no-image-hunk.patch | 4.42 KB | tstoeckler |
#3 | drupal8.menu-load-objects.3.patch | 2.7 KB | sun |
drupal8.menu-load-objects.0.patch | 1.17 KB | sun | |
Comments
Comment #1
fubhy CreditAttribution: fubhy commentedI ran across this a few times already now that I think about it. But I always assumed it was simply a missing feature. Count me +100 for fixing this though. Looking good already and is a pretty straight forward fix so RTBC from my side once testbot comes back green.
Comment #3
sunDang, and I actually hoped that this could be backported to D7... Bummer.
Comment #5
fubhy CreditAttribution: fubhy commentedInteresting... The two failing tests seem to work locally. Let's try it again.
Comment #6
fubhy CreditAttribution: fubhy commented#3: drupal8.menu-load-objects.3.patch queued for re-testing.
Comment #7
fubhy CreditAttribution: fubhy commentedComment #8
tstoecklerDon't want to rain on anyone's parade, but I think this could use some regression tests, i.e. a router item in test menu that exploits this behavior. Especially with the further Symfony-ification of the router system we want to be sure not to re-introduce this bug again in a month.
Comment #9
fubhy CreditAttribution: fubhy commentedOkay, so we simply want a test that checks a single menu item with 2 arguments. That's all we really need. And yes, this code is going to be deleted at some point anyways once we've switched to routes entirely anyways. It's just something we need for the profile2 code for now.
Comment #10
tstoecklerHere's that test.
I stepped through a whole chunk of menu.inc to verify that this change is correct.
I'm also attaching a patch that excludes the image.module hunk. As it's not directly obvious, let me explain the situation: This hunk is not actually needed to update image_help() for the new menu.inc behavior introduced here. The hunk in image_help() that is altered is broken before and after the menu.inc change. This is easily verified by adding a "Scale & Crop" effect to an image style. When adding, you will see the help text up top, that image_help() adds. Save that effect and directly edit it again and you will see no help text. The change here fixes it correctly (together with the menu.inc change), which can be verified just as easily by the above procedure. The thing is that this code can be greatly simplified on-top of what this patch does, because currently it allows to alter the help text for a reason that eludes me. I'll open a follow-up issue to explain that and clean that up, but I wanted to mention that a) it's a pre-existing bug and b) this should be cleaned-up anyway. On the other hand c) it fixed a bug. So I'll let the committers decide.
If someone could quickly verify that the tests are correct, this should be back to RTBC.
Not attaching an interdiff as that would be identical to the new test hunks.
Edit: Missing word(s)
Comment #12
sunThanks! That test covers exactly the expectation.
Comment #13
chx CreditAttribution: chx commentedPlease do not RTBC your own patches, thanks!
To quote OP, when defining a route
then the expectation is that
foo_load()
is invoked with arg(1), andbar_load()
is invoked with arg(3) and the already loaded arg(1).Well,I have no idea who set that expectation and where. Load arguments was rushed into core late because the need for that got discovered too late. The %map workaround might be ugly but that was the only recourse back then. The semantics was rushed too and I would be happy to figure better semantics. Very likely it will involve changing the syntax of load arguments.
So, core does
Looking at this, I am totally dumbfunded. I think this calls image_style_load(5, (string) IMAGE_STORAGE_EDITABLE) and also image_effect_load(5, (string) IMAGE_STORAGE_EDITABLE) ? So how does this even work?? How this is supposed to work? What better, more readable syntax can we figure? 'load arguments' => array('image_style' => array(5, (string) IMAGE_STORAGE_EDITABLE)) might be helpful as long as the same loader does not repeat twice. If it does, what then?
Comment #14
sun@chx:
Most of this code will be rewritten and replaced by the new router anyway, and I've been told that it supports this argument loading or will support it (in a much cleaner way). So the sole purpose of this issue to perform the necessary duct-taping for the old/current router.
The second issue that you raise, 'load arguments' being applied to all argument loaders, is a pre-existing and thus separate issue. But I do not see a reason to create a separate issue for that, since again, this entire API will cease to exist and route argument loaders will get defined in a cleaner way.
Comment #15
chx CreditAttribution: chx commentedI do not understand the following
Comment #16
sunCode that uses %map is unnecessarily complex, because the argument value lookup and validation needs to be performed manually.
It also turns the loader function needlessly into a one-off implementation for the menu router, since
- the $map argument cannot reasonably be provided by any other code,
- the argument loader function is specific to the exact position of arguments in a route, and
- there's also no way for type-hinting.
Proper vs. workaround:
Do we really need to debate this to great lengths? Can't we just simply do this as a stop-gap fix?
Comment #17
tstoeckler@chx:
Not quite, load arguments are appended to the part of the path that replaces the wildcard. I.e. given the router at hand, hitting the URL
http://example.com/admin/config/media/image-styles/edit/large/effects/scale
will result in the menu system calling
image_style_load('large', 'large', (string) IMAGE_STORAGE_EDITABLE)
and
image_effect_load('scale', 'large', (string) IMAGE_STORAGE_EDITABLE)
This patch makes that:
image_style_load('large', 'large', (string) IMAGE_STORAGE_EDITABLE)
and
image_effect_load('scale', ImageStyle $image_style, (string) IMAGE_STORAGE_EDITABLE)
Notes:
1. I am 100% sure that you already know this, and I in no way want seem like I am teaching you anything about the innerworkings of the menu system. I realize how inadequate/ridiculous that would be. I merely wanted to clear this up, as your example code was incorrect, and the current code actually does work.
2. I know that image effects are not actually called 'scale' and such, but referenced by UUID, but I wanted to make this readable/sensible.
Also re #13: The code had been RTBCed (without tests) before, and I explicitly re-RTBCed everything but the tests (which I wrote), so I think @sun RTBCing the patch (including the tests, which he did not write) was quite fine.
Comment #18
sunIs there really no way to move forward here more quickly?
This would allow us to remove some ugly %map code from #1668292: Move simplified Profile module into core.
Ultimately, the entire route argument upcasting will be replaced with new Routing code anyway - which will apparently pass typed data and loaded objects by design. As a result, this code comes much more closely to the new routing architecture in D8. It's a temporary thing to begin with IMHO.
Comment #19
jibran10: drupal8.menu-load-objects.10.patch queued for re-testing.