Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Related Issues
#1938888: Convert user_admin_permissions to a new-style Form object
#2056513: Remove PluginUI subsystem, provide block plugins UI as one-off
Comment | File | Size | Author |
---|---|---|---|
#7 | menu-2056895-7-FAIL.patch | 3.4 KB | xjm |
#7 | menu-2056895-7-PASS.patch | 4.14 KB | xjm |
#7 | interdiff-null.txt | 580 bytes | xjm |
#1 | menu-2056895-1-FAIL.patch | 3.4 KB | tim.plunkett |
#1 | menu-2056895-1-PASS.patch | 4.14 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettSee attached.
Comment #2
pwolanin CreditAttribution: pwolanin commentedcan we use null instead of - in the yaml?
Comment #3
dawehnerIsn'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
Comment #4
tim.plunkettI don't believe MENU_NORMAL_ITEM triggers the bug in quite the same way, but I can try it.
Comment #5
tim.plunkettTried 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.
Comment #6
dawehnerokay, let's go with it.
Comment #7
xjm@alexpott and I both also prefer the explicit NULL to the ~, and NULL is what we use elsewhere in core. ("Elsewhere" being Views UI.)
Comment #8
xjmOh, re:
returns no results. So unless these conversions are still being worked on?
Comment #9
xjmSorry, I meant to set this back to NR.
Comment #10
tim.plunkettWhy NR? This is literally the least important part of the patch. It's YAML, they're equivalent, done.
Comment #11
xjmBecause @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?
Comment #12
pwolanin CreditAttribution: pwolanin commented@xjm - the YAML spec says "null" not "NULL"?
Comment #13
xjmI just read it and it specifies any of the following:
http://yaml.org/type/null.html
Since
NULL
is consistent with our coding standards elsewhere, and since we also useTRUE
andFALSE
in yaml, I went withNULL
.Comment #14
alexpottCommitted 4e0d7b5 and pushed to 8.x. Thanks!
Comment #15
tim.plunkett