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

Files: 
CommentFileSizeAuthor
#7 menu-2056895-7-FAIL.patch3.4 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 57,841 pass(es), 3 fail(s), and 1 exception(s).
[ View ]
#7 menu-2056895-7-PASS.patch4.14 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 57,905 pass(es).
[ View ]
#7 interdiff-null.txt580 bytesxjm
#1 menu-2056895-1-FAIL.patch3.4 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,524 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
#1 menu-2056895-1-PASS.patch4.14 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,803 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new4.14 KB
PASSED: [[SimpleTest]]: [MySQL] 57,803 pass(es).
[ View ]
new3.4 KB
FAILED: [[SimpleTest]]: [MySQL] 57,524 pass(es), 2 fail(s), and 1 exception(s).
[ View ]

See attached.

can we use null instead of - in the yaml?

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

<?php
    $stripped_path
= static::getPathWithoutDefaults($route);
   
$pattern_outline = static::getPatternOutline($stripped_path);
?>

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

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.

Status:Needs review» Reviewed & tested by the community

okay, let's go with it.

StatusFileSize
new580 bytes
new4.14 KB
PASSED: [[SimpleTest]]: [MySQL] 57,905 pass(es).
[ View ]
new3.4 KB
FAILED: [[SimpleTest]]: [MySQL] 57,841 pass(es), 3 fail(s), and 1 exception(s).
[ View ]

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

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?

Status:Reviewed & tested by the community» Needs review

Sorry, I meant to set this back to NR.

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.

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?

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

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

Status:Reviewed & tested by the community» Fixed

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

Assigned:tim.plunkett» Unassigned

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