All elements added to the Request attributes should have a _ prepended unless they come from the path [#2052389]

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.

Comment	File	Size	Author
#9	2052389-7.patch	22.29 KB	pwolanin
#9
#9	2052389-2-7.increment.txt	3.68 KB	pwolanin
#2	2052389-2.patch	19.16 KB	pwolanin
#2

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #0.0

pwolanin CreditAttribution: pwolanin commented 28 July 2013 at 18:53

Issue summary:

View changes

Updated issue summary.

Comment #1

dawehner

German

CreditAttribution: dawehner commented 29 July 2013 at 08:50

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

Comment #2

pwolanin CreditAttribution: pwolanin commented 30 July 2013 at 01:23

Status:

Active

» Needs review

File	Size
2052389-2.patch	19.16 KB

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

Comment #3

dawehner

German

CreditAttribution: dawehner commented 30 July 2013 at 08:52

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.

Comment #4

pwolanin CreditAttribution: pwolanin commented 30 July 2013 at 13:09

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

Comment #5

pwolanin CreditAttribution: pwolanin commented 30 July 2013 at 13:43

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'));

Comment #6

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 30 July 2013 at 14:49

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.

Comment #7

dawehner

German

CreditAttribution: dawehner commented 30 July 2013 at 16:50

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.

Comment #8

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 30 July 2013 at 16:55

Fine by me.

Comment #9

pwolanin CreditAttribution: pwolanin commented 30 July 2013 at 17:22

Status:

Reviewed & tested by the community

» Needs review

File	Size
2052389-2-7.increment.txt	3.68 KB
2052389-7.patch	22.29 KB

handles drupal_menu_item

Comment #10

dawehner

German

CreditAttribution: dawehner commented 30 July 2013 at 17:23

Status:

Needs review

» Reviewed & tested by the community

Great.

Comment #11

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 31 July 2013 at 20:50

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!

Comment #12

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 31 July 2013 at 20:52

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.

Comment #13

dawehner

German

CreditAttribution: dawehner commented 3 August 2013 at 06:43

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.

Comment #14

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 9 August 2013 at 18:49

Fixing tags

Comment #15

catch

he/him

English

CreditAttribution: catch commented 6 September 2013 at 14:24

Priority:

Critical

» Major

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

Comment #16

dawehner

German

CreditAttribution: dawehner commented 7 September 2013 at 13:15

Status:

Active

» Fixed

https://drupal.org/node/2083979

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

Comment #17

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran commented 7 September 2013 at 16:05

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

Comment #18

pwolanin CreditAttribution: pwolanin commented 7 September 2013 at 16:09

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.

Comment #19

effulgentsia CreditAttribution: effulgentsia commented 11 September 2013 at 17:33

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.