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), and bar_load() is invoked with arg(3) and the already loaded arg(1).

    That is, because I did not specify array('1') (string). I specified array(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 Profile2 module into core, respectively #1726822: Port Profile2 to D8 and provide upgrade path from D7 core profile

Files: 
CommentFileSizeAuthor
#10 drupal8.menu-load-objects.10.tests-only.patch2.71 KBtstoeckler
FAILED: [[SimpleTest]]: [MySQL] 49,375 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#10 drupal8.menu-load-objects.10.patch5.54 KBtstoeckler
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.menu-load-objects.10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#10 drupal8.menu-load-objects.10.no-image-hunk.patch4.42 KBtstoeckler
FAILED: [[SimpleTest]]: [MySQL] 49,402 pass(es), 36 fail(s), and 27 exception(s).
[ View ]
#3 drupal8.menu-load-objects.3.patch2.7 KBsun
PASSED: [[SimpleTest]]: [MySQL] 49,212 pass(es).
[ View ]
drupal8.menu-load-objects.0.patch1.17 KBsun
FAILED: [[SimpleTest]]: [MySQL] 49,088 pass(es), 37 fail(s), and 27 exception(s).
[ View ]

Comments

I 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.

Status:Needs review» Needs work

The last submitted patch, drupal8.menu-load-objects.0.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.7 KB
PASSED: [[SimpleTest]]: [MySQL] 49,212 pass(es).
[ View ]

Dang, and I actually hoped that this could be backported to D7... Bummer.

Status:Needs review» Needs work

The last submitted patch, drupal8.menu-load-objects.3.patch, failed testing.

Interesting... The two failing tests seem to work locally. Let's try it again.

Status:Needs work» Needs review

#3: drupal8.menu-load-objects.3.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs tests

Don'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.

Okay, 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.

Status:Needs work» Needs review
StatusFileSize
new4.42 KB
FAILED: [[SimpleTest]]: [MySQL] 49,402 pass(es), 36 fail(s), and 27 exception(s).
[ View ]
new5.54 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.menu-load-objects.10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new2.71 KB
FAILED: [[SimpleTest]]: [MySQL] 49,375 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Here'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)

Status:Needs review» Needs work

The last submitted patch, drupal8.menu-load-objects.10.no-image-hunk.patch, failed testing.

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs tests

Thanks! That test covers exactly the expectation.

Title:_menu_load_objects() does not pass already loaded arguments to subsequent argument loadersFigure out better load arguments semantics
Category:bug» task
Status:Reviewed & tested by the community» Needs work

Please do not RTBC your own patches, thanks!

To quote OP, 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), and bar_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

<?php
  $items
['admin/config/media/image-styles/edit/%image_style/effects/%image_effect'] = array(
   
'title' => 'Edit image effect',
   
'description' => 'Edit an existing effect within a style.',
   
'load arguments' => array(5, (string) IMAGE_STORAGE_EDITABLE),
?>

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?

Status:Needs work» Needs review

@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.

I do not understand the following

  1. How on earth does image module currently even work?
  2. If this is temporary why do we need this change and why can't we use map/index?

Title:Figure out better load arguments semantics_menu_load_objects() does not pass already loaded arguments to subsequent argument loaders
Issue tags:+Quick fix

Code 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:

-function profile2_menu_load($type_id, User $account, $op = '') {
+function profile2_menu_load($type_id, $map, $op = '') {
+  if (is_array($map)) {
+    if (!isset($map[1]) || !($map[1] instanceof User)) {
+      return FALSE;
+    }
+    $account = $map[1];
+  }
+  else {
+    $account = $map;
+  }
   if ($type_id === '' || !$account->id()) {
     return FALSE;
   }

Do we really need to debate this to great lengths? Can't we just simply do this as a stop-gap fix?

@chx:

<?php
  $items
['admin/config/media/image-styles/edit/%image_style/effects/%image_effect'] = array(
   
'title' => 'Edit image effect',
   
'description' => 'Edit an existing effect within a style.',
   
'load arguments' => array(5, (string) IMAGE_STORAGE_EDITABLE),
?>

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) ?

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.

Is there really no way to move forward here more quickly?

This would allow us to remove some ugly %map code from #1668292: Move simplified Profile2 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.

Status:Needs review» Needs work

The last submitted patch, 10: drupal8.menu-load-objects.10.patch, failed testing.