Problem:

Any developer may add an arbitrary {foo} element to the path and expect the raw or upcast value in the request attributes. However, Drupal core is currently putting may elements into attributes including 'system_path', 'theme', etc.

This makes for an unpredictable/poor developer experience since they would have to hope that a chosen path slug doesn't conflict with a value used by core, or worse, some contributed module.

Symfony already adds some such elements, but they are named with a "_" prefix like "_route".

Proposed Solution:

Based on the solution at:
#2043781: Drupal::request()->attributes->get('account') may conflict with an account object loaded from the path
We should at least prefix every one of these with "_" so that it does not conflict with an attribute from a path slug.

We should also add to the coding standard an expectation that and module-added attributes should be prefixed with _ or something else.

Note: an ideal solution would b more comprehensive (e.g. subclassing the Request class), but this solution ought to be sufficient for Drupal 8.

CommentFileSizeAuthor
#9 2052389-7.patch22.29 KBpwolanin
#9 2052389-2-7.increment.txt3.68 KBpwolanin
#2 2052389-2.patch19.16 KBpwolanin

Comments

pwolanin’s picture

pwolanin commented
Issue summary: View changes

Updated issue summary.

dawehner’s picture

dawehner
Primary language German
commented

Just linking one subissue: #2048099: Replace system_path with _system_path

pwolanin’s picture

pwolanin commented
Status: Active » Needs review
StatusFileSize
new 2052389-2.patch19.16 KB

This converts system_path to _system_path, which is the main culprit.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

I don't really understand why you redid it, even there was an existing issue with a patch, but well.

pwolanin’s picture

pwolanin commented

@dawehner - oh, sorry I didn't know about the other issue. I want to add any others like 'menu_item' to this as well (though happy for it to go in multiple different patches).

pwolanin’s picture

pwolanin commented

some other possible problems:

core/lib/Drupal/Core/EventSubscriber/LegacyControllerSubscriber.php:40:
    $router_item = $request->attributes->get('drupal_menu_item');

core/modules/shortcut/lib/Drupal/shortcut/Access/LinkDeleteAccessCheck.php:30:
    $menu_link = $request->attributes->get('menu_link');

core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.php:38:
    $request->attributes->get('user.data')->set('overlay', $account->id(), 'message_dismissed', 1);

core/modules/menu/lib/Drupal/menu/Access/DeleteLinkAccessCheck.php:30:
    if (user_access('administer menu') && $menu_link = $request->attributes->get('menu_link')) {

core/modules/menu/lib/Drupal/menu/Access/DeleteMenuAccessCheck.php:30:
    if (user_access('administer menu') && $menu = $request->attributes->get('menu')) {

core/modules/node/lib/Drupal/node/Access/NodeRevisionAccessCheck.php:76:
    $revision = $this->nodeStorage->loadRevision($request->attributes->get('node_revision'));
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Reviewed & tested by the community » Needs work

The menu_link, menu, and node_revision are coming directly from the route placeholders, that's correct. Not sure about user.data.

But drupal_menu_item should be handled here.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Reviewed & tested by the community

Can we please get the system_path in first, as it is really simple for now.
drupal_menu_item is something we don't want to deal with on the longrun anyway, as it just exists on the legacy router system.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

Fine by me.

pwolanin’s picture

pwolanin commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 2052389-2-7.increment.txt3.68 KB
new 2052389-7.patch22.29 KB

handles drupal_menu_item

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Great.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

This makes a ton of sense to me. I'm pleasantly surprised that these are the only places that need to be changed, but cool.

Committed and pushed to 8.x. Thanks!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Title: All elements added to the Request attributes should have a _ prepended unless they come from the path » Change notice: All elements added to the Request attributes should have a _ prepended unless they come from the path
Category: bug » task
Status: Fixed » Active
Issue tags: +Needs change record

Hm. And actually, let's change notice this.

dawehner’s picture

dawehner
Primary language German
commented

I changed the example on https://drupal.org/node/1539454/revisions/view/2789033/2789035 but yeah it is not a real change notice.
The question is whether we should try to group change notes.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

Fixing tags

catch’s picture

catch
he/him
Primary language English
commented
Priority: Critical » Major

#2083415: [META] Write up all outstanding change notices before release.

dawehner’s picture

dawehner
Primary language German
commented
Status: Active » Fixed

https://drupal.org/node/2083979

The existing ones already got updated, so this is just a new one.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Title: Change notice: All elements added to the Request attributes should have a _ prepended unless they come from the path » All elements added to the Request attributes should have a _ prepended unless they come from the path
Category: task » bug
Priority: Major » Critical
Issue tags: -Needs change record
pwolanin’s picture

pwolanin commented
Category: bug » task
Priority: Critical » Major

It was unclear to me from Daniel's comment, but https://drupal.org/node/2083979 is the requested change notice.

effulgentsia’s picture

effulgentsia commented

Wondering if anyone here has feedback on #1778122-87: Enable modules to inject attributes into field formatters, so that RDF attributes get output where I'm asking whether we should adopt a similar leading "_" convention in Field API: specifically, for adding non-field-type-specific properties to field items: in that issue's case, an "_attributes" property for HTML attributes added by RDF and similar modules. If you do, please comment there; thanks.

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Updated issue summary.