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 Profile module into core, respectively #1726822: Port Profile2 to D8

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fubhy’s picture

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.

sun’s picture

Status: Needs work » Needs review
FileSize
2.7 KB

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.

fubhy’s picture

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

fubhy’s picture

Status: Needs work » Needs review

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

fubhy’s picture

Status: Needs review » Reviewed & tested by the community
tstoeckler’s picture

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.

fubhy’s picture

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.

tstoeckler’s picture

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.

sun’s picture

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

Thanks! That test covers exactly the expectation.

chx’s picture

Title: _menu_load_objects() does not pass already loaded arguments to subsequent argument loaders » Figure 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

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

sun’s picture

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.

chx’s picture

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?
sun’s picture

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?

tstoeckler’s picture

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

sun’s picture

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

jibran’s picture

Status: Needs review » Needs work

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

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.