Drupal::request()->attributes->get('account') may conflict with an account object loaded from the path

I agree here - we should not use a name that can conflict with a path parameter.

Something like ":account" would be safer for this pseudo-global

making title more clear

Can we use _account instead of :account?

@jibran - why? That could still conflict with a path variable? This is just an array key, it doesn't have to be a valid PHP identifier.

Actually we should ask fubhy about his vision for the controller resolver and this sort of extra attribute. If the controller resolver fell back to looking for the variable name with ":" (or whatever you like: "-", "|", "#"), that could be an interesting way to avoid collision.

Maybe here we have "#account" and in the other issue stick to ":raw" since that would never be a fall-back as a function param?

Overall this is rather ugly and might be better solved by defining a Request subclass with one or more additional properties to hold this stuff safely away from the regular attributes.

If we want to be able to use $account in our controllers :account does not work as we can't pass $:account (invalid php variable name). Currently we can simply define $account and get the current account passed (with the aforementioned problems that occur when you want to use {account}). $_account would work, but that's still ugly.

We are already special-casing the :raw stuff in our controller resolver. But that's really a special case. Doing that for everything we throw into the request attributes would be weird. It should simply work and not require all this custom code for special use-cases. Otherwise it totally destroys the DX here and makes it harder to debug.

The underlying problem here is that we throw everything into the same pile of dirt ($request->attributes / $defaults) which simply leads to this sort of namespace collisions.

Here is a short list of things that currently lead to collisions:

• {form} - Used in HtmlFormController (only temporarly, but still overrides any potential {form} placeholder. And yes, this is a placeholder name you might want to use in contrib. Think about modules like webform or anything else that tries to work with forms.
• {form_state} - Same as with {form}
• {account} - The currently logged in account - This is something that quite a few contrib modules might want to use.

The problem with forms can be fixed by simply changing how we currently do forms. I think HtmlFormController and _form is absolutely broken at the moment. Especially how it uses this weird FormInterface where buildForm() (the controller entry point) is part of the interface and then expects you to define any additional arguments that you want from ControllerResolver::getArguments() as optional method arguments (in order to not conflict with the interface definition) even though they aren't actually optional. That's totally broken. My standpoint is that a controller method should always be explicitly defined in whatever place specifies it (so in this case, _form should have the full controller name, not just the class) and should NEVER be part of an interface. This goes for anything that should be invoked through reflection. Having an interface definition of a controller method with variable arguments (and that's part of my definition of a controller) is a blatant lie.

We WANT to be able to get the current user as a controller argument of some sort. {account} is probably not the best name, but we definitely want it as a controller argument through ControllerResolver::getController(). You should never have to use the request object in a specific controller. Only generic controllers (which don't know the types or potentially even don't know the list of available arguments) should have to use it (even then it should be rare). Bottom-line: Under normal circumstances a controller should simply not care about the Request (with the aforementioned exceptions and some other special cases).

I really don't like how we are currently dumping stuff into the request attributes. It really feels wrong and induces so many problems with possible collisions. At the same point, however, I want the controller resolver to be able to pass these things. Maybe we've been going about this all wrong. Maybe we need to find other places where we can store things like that which the controller resolver then would have to be aware of. But where should we store it? A service that stores the current user? But shouldn't services be stateless? Maybe we really need to override the request object. I don't know.

The problem is that potentially not only form, form_state and account will participate. If you look at the current request object there is already way more, for example system_path.

So what about adding a new parameter bag on the root level? So there will be $request->attributes as well as $request->ponies?

We've managed to avoid subclassing the Request this long, which IMO is a good thing. To add another top level bag would require subclassing if we want to do it right. (Right = as a defined property not as a PHP randomly added property; in PHP 5.4 and later there's a non-trivial memory difference, plus you need that for IDE autocompletion.)

Everywhere else, _ prefix means "special and important". _form, _controller, _content, etc. Unless you want that magic, you really have no business putting an underscored parameter into the path pattern. (_format is the only one that I can think of; _controller is an instant security risk.) If we have to rename the variable, _account is the only realistic option. If you put _account into your path pattern, you're dumb. But that's no more of a security hole than naming a variable $user was in Drupal 7, and really less so because the underscore should tell you "dude, special meaning, don't do that!"

Let's not turn into Perl with its many magic characters. :-)

So, then we should at least be doing _system_path as well? And/or stopping to use that :-)

What's the magic of "_" other than convention? We could also use a "#" prefix or make up other conventions, but those could never match method argument names obviously.

The only thing special about _ is that Symfony already used that to mean "special meaning", and we're being consistent. _ also means that the key can be used in a YAML file; # or : and so forth all have meaning in YAML.

That said, I don't have a conceptual problem with a few documented black-listed placeholder names.

I guess at some level we should actively throw exceptions if someone specifies any of these special variables as attributes. I guess the route compile level would be perfect, as it is not on runtime.

Imho only the account attribute is really problematic at the moment

Opened a followup/parallel issue: #2048099: Replace system_path with _system_path

ok, so let's use "_account"

This should happen fast because we have started using it in all patches and it also blocks #1938390-42: Convert contact_site_page and contact_person_page to a new-style Controller. It is a straight replace of account with _account. It is green so I think it is RTBC.

So i'm not the hugest fan of this due to the _account key chosen

$request->attributes->get('_account') != $request->attributes->get('account') is going to be confusing...

To be honest I think that people should not use 'account', but go with 'user' as route variable, at least in core, but on the longrun we will not be able to avoid the problem.

We only used {account} in routes because we never use $user unless it was global $user.

Now the magical logged in user has switched to 'account', we can finally go back to {user}. Which will make the routes easier...

So I say go ahead with this, and we'll end up with:
$request->attributes->get('_account') != $request->attributes->get('user')
Except in reality we'll have put the user in the method params, so:
$request->attributes->get('_account') != $user

I'm mostly only an observer from the sidelines when it comes to routing issues, but is there any technical argument against subclassing the Request class to provide separate accessors for this? It does not seem very future-proof to stick arbitrary keys ('account', 'theme', ...) into an array that's auto-magically filled based on the route configuration and the method-parameters, upcasting, etc. I think the more we will be using the routing system, and the more stuff we will set on the request like account, we are going to hit these problems more and more. Depending on the case, they might also be pretty hard to debug or find in the first place.

I'm not against going with '_account' as a stop-gap measure, but I don't have the feeling that this option was sufficiently discussed.

If we decide that $request->attributes should not be filled with stuff from contrib at all we could even to $request->getAccount() which would be cool for auto-completion.

I'm mostly only an observer from the sidelines when it comes to routing issues, but is there any technical argument against subclassing the Request class to provide separate accessors for this. It does not seem very future-proof to stick arbitrary keys ('account', 'theme', ...) into an array that's auto-magically filled based on the route configuration and the method-parameters, upcasting, etc. I think the more we will be using the routing system, and the more stuff we will set on the request like account, we are going to hit these problems more and more. Depending on the case, they might also be pretty hard to debug or find in the first place.

There is not really an easy way of solving this. Overriding the request would just solve the stuff we do in core. We should not even start with that. As it stands now, we are using the request object to replace some of our former superglobals. It's really just swapping it for the better evil. Overriding the request class would just solve it temporarily as we have no clue what else people might add in contrib.

The leading underscore basically marks things as "special" as Larry already pointed out. You should not ever have any {placeholders} with leading underscores. Hence, the only potential for overlap is within contrib modules that try to provide the same name for a special, underscore-prefixed request attribute that they are trying to set in an enhancer or some other thing.

What I was thinking about earlier was another special enhancer pass that would allow explicitly writing these properties including exceptions in case of overlap. Not sure how feasible that is. Possibly including meta data / type definitions for these things. That would give us a generic solution for scotch to use these request attributes too (not just route-related parameters). Not sure how feasible that would be though.

If we decide that $request->attributes should not be filled with stuff from contrib at all we could even to $request->getAccount() which would be cool for auto-completion.

That would be both cool and weird. What's an account in terms of a request? It's definitely not an actual request property. It's just something we resolve during the request and then temporarily store it in there so other things can easily access it later. It's not "actually" a part of the request though.

It's really just swapping it for the better evil.

That would be both cool and weird. What's an account in terms of a request? It's definitely not an actual request property. It's just something we resolve during the request and then temporarily store it in there so other things can easily access it later. It's not "actually" a part of the request though.

Well, it is a part of the request, in a way, as the session ID in the cookie of the requesting user. In more general terms, how would this work in a perfect world, without eiher of the two evils? I.e. what is the perfect world equivalent of user_access('foo') if it's not $this->request->attributes->get('_account')->hasPermission('foo') ? Should we always inject account objects and the controller just passes it down into every single class that needs it? I wasn't aware that the entire concept of sticking stuff into $request->attributes() was considered temporary in any sense.

Let's just do it, and not overthink it. People are not stupid and will just realize that _ is somehow kind of private. This is a common pattern in both drupal and in the outside world. #2051877: Log error when people use invalid route parameters

Done. I like the look of #2051877: Log error when people use invalid route parameters to ensure this decision does not come back to haunt us.

Committed 926a067 and pushed to 8.x. Thanks!

I think we'll need to update a change notice or two... for example https://drupal.org/node/2017231

Updated https://drupal.org/node/2017231 and https://drupal.org/node/2032447

Updated https://drupal.org/node/2049309/revisions/view/2779689/2781485 as well.