Updated: Comment #0

Problem/Motivation

When a route has an optional placeholder, and its placed into a menu as a menu link, it can cause fatal errors.
This is because the function used to determine the path of a route doesn't handle optional placeholders at the end of the path.

Proposed resolution

Rewrite _menu_router_translate_route() to properly use Symfony routes's pattern outline functionality to properly determine the path.

Remaining tasks

N/A

User interface changes

N/A

API changes

N/A

#1938888: Convert user_admin_permissions to a new-style Form object
#2056513: Remove PluginUI subsystem, provide block plugins UI as one-off

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
4.14 KB
3.4 KB

See attached.

pwolanin’s picture

can we use null instead of - in the yaml?

dawehner’s picture

+++ b/core/includes/menu.incundefined
@@ -2803,11 +2803,11 @@ function menu_router_build($save = FALSE) {
-  return preg_replace('/{' . DRUPAL_PHP_FUNCTION_PATTERN . '}/', '%', $path);

+++ b/core/modules/system/tests/modules/menu_test/menu_test.moduleundefined
@@ -438,6 +438,11 @@ function menu_test_menu() {
+    'type' => MENU_LOCAL_TASK,

Isn't it a bit safer, especially in the future, to just rely on 'type' => MENU_NORMAL_ITEM?

According to http://yaml.org/type/null.html there is support for both '~', 'null', etc. Currently all of the conversions went with ~ but I do actually prefer 'null', so you don't have to guess what "~" could mean.

The getPatternOutline() is the perfect method, as it gets the stripped path, see

    $stripped_path = static::getPathWithoutDefaults($route);
    $pattern_outline = static::getPatternOutline($stripped_path);
tim.plunkett’s picture

I don't believe MENU_NORMAL_ITEM triggers the bug in quite the same way, but I can try it.

tim.plunkett’s picture

Tried it locally, it breaks differently and not as spectacularly, and isn't related to anything I've encountered in core.

#1 is the bug as it is in core and as it blocks that issue. And since the local task stuff is still in flux, I don't think its wrong to use this in test coverage. The rest of that test module uses all of those menu type constants.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

okay, let's go with it.

xjm’s picture

@alexpott and I both also prefer the explicit NULL to the ~, and NULL is what we use elsewhere in core. ("Elsewhere" being Views UI.)

xjm’s picture

Oh, re:

According to http://yaml.org/type/null.html there is support for both '~', 'null', etc. Currently all of the conversions went with ~ but I do actually prefer 'null', so you don't have to guess what "~" could mean.

grep -r ": ~" * | grep "\.routing\.yml" | grep -v "vendor"

returns no results. So unless these conversions are still being worked on?

xjm’s picture

Status: Reviewed & tested by the community » Needs review

Sorry, I meant to set this back to NR.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Why NR? This is literally the least important part of the patch. It's YAML, they're equivalent, done.

xjm’s picture

Why NR? This is literally the least important part of the patch. It's YAML, they're equivalent, done.

Because @alexpott asked me to make the change, and I wanted to let @dawehner give feedback on it because of #3 and #8, and I don't RTBC my own changes to functional code or tests.

Edit: Aren't you supposed to have no laptop?

pwolanin’s picture

@xjm - the YAML spec says "null" not "NULL"?

xjm’s picture

@xjm - the YAML spec says "null" not "NULL"?

I just read it and it specifies any of the following:

 ~ # (canonical)
|null|Null|NULL # (English)
| # (Empty)

http://yaml.org/type/null.html

Since NULL is consistent with our coding standards elsewhere, and since we also use TRUE and FALSE in yaml, I went with NULL.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 4e0d7b5 and pushed to 8.x. Thanks!

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

Automatically closed -- issue fixed for 2 weeks with no activity.