Change the current_user service to a proxy

Seems simple enough... Although I thought there was discussion of something more involved/complete in IRC?

1. +++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
   @@ -135,6 +144,25 @@ public function defaultProviderId() {
      /**
   +   * {@inheritdoc}
   +   */
   +  public function getUser() {
   +    if (empty($this->currentUser)) {
   +      $this->currentUser = drupal_anonymous_user();
   +    }
   +
   +    return $this->currentUser;
   +  }
   

   So that means that calling this before the access listener stuff did run, the current user object is anonymous. Makes sense, but should be documented then.

2. +++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
   @@ -83,8 +91,6 @@ public function applies(Request $request) {
       */
      public function authenticate(Request $request) {
   -    global $user;
   -
        $account = NULL;
   

   Other than the listener, should this even be called anymore publicly? Maybe at least document as such, possibly remove from the interface?

3. +++ b/core/lib/Drupal/Core/Authentication/AuthenticationManagerInterface.php
   @@ -19,4 +21,21 @@
   +
   +  /**
   +   * Sets a user.
   +   *
   +   * @param \Drupal\Core\Session\UserSession $user
   +   *   The user session to set.
   +   */
   +  public function setUser(UserSession $user);
   

   This should be AccountInterface I think?

   To be honest, I don't understand why the current service/naming was user, the idea was that $account could be anything that implements AccountInterface and $user is a user entity.

   For example basic auth afaik currently uses the storage controller to load the user, so it would be a entity for them, and setUser() is quite often used for a loaded user entity.

In general, this is obviously a bit more code for services that get it injected, but I do like a lot that we can interact with an actual service when switching/setting the user instead of having a direct dependency of the container and interacting directly with that.

This has the potential to cause all kind of circular dependency issues, this is clearly major.

Related about comment module manager #2182055-14: Comment module causes Circular reference error on a REST request

Great work!

1. +++ b/core/lib/Drupal.php
   @@ -163,7 +163,7 @@ public static function request() {
      }
   
   ...
       * @return \Drupal\Core\Session\AccountInterface
       */
      public static function currentUser() {
   -    return static::$container->get('current_user');
   +    return static::$container->get('authentication')->getAccount();
   

   Given that we remove the current_user service do we longer want to define a currentUser function?

   I guess yes?

2. +++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
   @@ -135,6 +144,29 @@ public function defaultProviderId() {
   +  public function getAccount() {
   +    // Set the current account to anonymous if the current account has been
   +    // requested but no authentication providers have authenticated an account.
   +    // As well as times when the request may not be set, like during
   +    // rebuild.php.
   +    if (empty($this->currentAccount)) {
   +      $this->currentAccount = drupal_anonymous_user();
   +    }
   +
   +    return $this->currentAccount;
   +  }
   

   I wonder whether we want to authenticate if something asks for the user?

3. +++ b/core/lib/Drupal/Core/Cron.php
   @@ -78,10 +89,10 @@ public function run() {
   +    $this->authManager->setAccount($anonymous);
   +    $GLOBALS['user'] = $anonymous;
   
   @@ -147,9 +158,9 @@ public function run() {
        $GLOBALS['user'] = $original_user;
   

   Should we just set the global in the authManager?

4. +++ b/core/lib/Drupal/Core/Routing/Enhancer/AuthenticationEnhancer.php
   @@ -21,7 +20,7 @@
   +class AuthenticationEnhancer implements RouteEnhancerInterface {
   ...
   -class AuthenticationEnhancer extends ContainerAware implements RouteEnhancerInterface {
   

   Nice!

1. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkManager.php
   @@ -155,7 +155,7 @@ public function getContextualLinksArrayByGroup($group_name, array $route_paramet
   -      if (!$this->accessManager->checkNamedRoute($route_name, $route_parameters, $this->account)) {
   +      if (!$this->accessManager->checkNamedRoute($route_name, $route_parameters, $this->authManager->getAccount())) {
   
   @@ -89,17 +89,17 @@ class ContextualLinkManager extends DefaultPluginManager implements ContextualLi
   -    $this->account = $account;
   +    $this->authManager = $auth_manager;
   

   This kinda sucks.

2. +++ b/core/lib/Drupal/Core/Routing/Enhancer/AuthenticationEnhancer.php
   @@ -54,7 +53,7 @@ public function enhance(array $defaults, Request $request) {
   -        $this->container->set('current_user', $anonymous_user, 'request');
   +        $this->manager->setAccount($anonymous_user);
   

   This is nice, but very internal.

3. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Block/CustomBlockBlock.php
   @@ -143,7 +143,7 @@ public function build() {
   -        '#access' => $this->account->hasPermission('administer blocks')
   +        '#access' => $this->authManager->getAccount()->hasPermission('administer blocks')
   

   Filed under *DX regression*

Given that we remove the current_user service do we longer want to define a currentUser function?

Please please please do not do this.

Well, I have to say that loading the current user don't have to be perse a trivial operation. Technically it was always wrong to put
a 100% state dependent service onto the container.

An alternative implementation could be http://symfony.com/doc/current/components/dependency_injection/lazy_serv...
This would create a proxy user object which won't trigger authentication automatically, just if someone calls any method on the user.

Drupal::currentUser() can obviously stay, although I consider it to be named incorrectly but whatever :)

Yes, it is a bit more code, but everything else is just wrong.

It is IMHO exactly the same problem/principle as http://symfony.com/blog/new-in-symfony-2-4-the-request-stack. Having something that can change during a request as a service and inject it into services is weird, having to interact directly with the container to switch the logged in user is wrong :)

+1 and a better title:)
coming from #2187431: Remove current_user service, retrieve it from the authentication manager

1. +++ b/core/update.php
   @@ -409,6 +408,8 @@ function update_check_requirements($skip_warnings = FALSE) {
   +  \Drupal::service('authentication')->authenticate($request);
   
   @@ -350,8 +350,7 @@ function update_check_requirements($skip_warnings = FALSE) {
   +\Drupal::service('authentication')->authenticate($request);
   
   @@ -409,6 +408,8 @@ function update_check_requirements($skip_warnings = FALSE) {
   +
   

   why this needs to happen twice?

2. +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/AccessSubscriberTest.php
   @@ -100,9 +100,10 @@ public function setUp() {
   +    $this->authManager = $this->getMock('Drupal\Core\Authentication\AuthenticationManagerInterface');
   +    $this->authManager->expects($this->any())
   +      ->method('getAccount')
   +      ->will($this->returnValue($this->getMock('Drupal\Core\Session\AccountInterface')));
   

   i guess we could add a hellper method for this, i keep seeing the same logic in different tests

Re-roll.

patch-wise i think this is good to go:)
damianklloip++

I really wonder whether exposeing the authentication like that is a good idea. Can't we at least provide some service which wraps the authentication manager, so that it for example won't have to store the current user, which feels a bit like a design flaw :(

we could have the session do that, but that issue is postponed to this one

@dawehner: Can you explain why you think the authentication manager is a bad place for this?

IMHO, the authentication manager doesn't simply manage the authentication backends, he manages the currently logged in user so this makes sense.

To be able to properly abstract the drupal_save_session() stuff that we have in place when switching the currently logged in user, then the authentication manager feels like the only possible place for this, as he is the only one that knows about the active authentication provider instance to which $authenticationManager->disableSaveOrWhateverYouCallIt() can be forwarded to?

@Paris: Wouldn't the session service only be used by the cookie authentication provider and isn't supposed to be accessed directly?

Wouldn't the session service only be used by the cookie authentication provider and isn't supposed to be accessed directly?

Well the session service should be accessed directly for many common things, like setting/getting $_SESSION values (and hopefully drupal_set_message() if we manage to convert it).
So it will be exposed more regularly to developers than authentication (right now i think we have just 2 services asking for the authentication even in core.services.yml).

We could have the session service providing the getter/setter for the current user object.
One would say it feels wrong and essentially kills the pluggability of our authentication system. but this is just a lie..there is no pluggability and there will never be as long as we have uid field in the session table

So yes, the patch in #34 makes complete sense and adding more layer (a service that has two methods (get/set)Account) makes the whole thing even more unnecessary complex. i would rather we remove the authentication system and retrieve the current logged in user from session, but that might be a step backwards and also probably out of scope here

I don't want to block the issue, it just feels wrong to expose such subsystems for that important pieces of functionality.

I think this needs a bit more discussion first, sorry.

I understand why we need to retrieve it from the authentication manager.
I do not understand why we need to expose that.

Can't we keep a current user service of some sort? Or call it a factory, or whatever. That has two methods, get/set, and it uses the authentication manager.

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -1810,7 +1810,7 @@ protected function drupalStaticReset($name = NULL) {
   -      if (\Drupal::getContainer()->has('current_user')) {
   +      if (\Drupal::getContainer()->has('authentication')) {
   

   I don't care if the container has authentication. I care if I can get the current user.

2. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   @@ -114,10 +114,10 @@ class LocalTaskManager extends DefaultPluginManager {
   -  public function __construct(ControllerResolverInterface $controller_resolver, Request $request, RouteProviderInterface $route_provider, ModuleHandlerInterface $module_handler, CacheBackendInterface $cache, LanguageManager $language_manager, AccessManager $access_manager, AccountInterface $account) {
   +  public function __construct(ControllerResolverInterface $controller_resolver, Request $request, RouteProviderInterface $route_provider, ModuleHandlerInterface $module_handler, CacheBackendInterface $cache, LanguageManager $language_manager, AccessManager $access_manager, AuthenticationManagerInterface $auth_manager) {
   

   The local task system does not care about authentication. It cares about having a user.

If the problem is that the service is assumed to return the user object directly, that's fine. We can switch all calls to use getAccount or whatever, and hide the authentication stuff internally.

Especially if we find out this was all overkill, or someone finds another solution, they can actually swap out the current user service separately from the authentication one.

I'm totally fine with not rushing anything and discussing it through, too many changes related to this were already rushed in, which resulted in the current confusing state :)

Trying to understand your reasons, but I'm not sure I do:

- The current user isn't a service, it's a state/data object just like request. Yes, that is a service too right now, but the referenced blog post above documents that it has been deprecated and will be removed as a standalone service in Symfony 3.0.
- Given that, I think we can not make it a standalone service. Sure, we could try to only use setter injection and what not, but again, the referenced issue states that Symfony tried all that for the request and finally came to the conclusion that it simply doesn't work and replaced it with RequestStack.
- So then next thing that I don't understand is why you consider the authentication manager to be some "irrelevant subsystem thing that code shouldn't have to interact with"? How is this different from getting a config object from the config factory, for example? I don't understand how it would be easier to understand if we'd have a CurrentUserStack (or whatever, just basing on RequestStack) service, instead of asking the authentication manager? I think we have a large amount of managers and factories now that code is interacting with (cache, config, entity, state, hooks, pretty much everything?), so why is having an authentication manager confusing but not the others?

#48.1: Yep that's weird, but so is the code in HEAD, when would the current_user service (not having a logged in user, but the actual current_user object existing or not) not exist? Maybe we can assume that authentication always exists and we would ask it if there is a current user? "Drupal::service('authentication')->hasCurrentUser()? Would that be better? Isn't the fact that current_user can conditionally exist or not an indicator that it is not a service? This seems like another example where we directly interact with the DIC and make it an essential part of an API while it should be just a very dump container of services? Instead of interacting with a service, that has an explicit API and responsibility.

#48.2: Also agreed, but if the authentication manager is *the* way to get the current user, that's the thing to inject? Again, just like you inject the entity manager if you want to do anything with entities, or the key value factory if you want to use a given key value collection? True, we do have a few shortcuts, like a @state service, and @database for the default database connection, and each cache bin is exposed as a service, but those are still all services and not their own data objects.

How is this different from getting a config object from the config factory
or the key value factory if you want to use a given key value collection

It's all about naming. Those are both "foo" and "foo factory" pairs.

I think @ParisLiakos was joking, but in IRC he said:

why dont we just alias authentication to current_user

I think that maybe just seeing s/current_user/authentication is my problem here, and that authentication is just a weird name. Maybe I'll get used to it, maybe I won't.

If it was the current_user.factory service, I would get it. Even if that is the authentication service underneath, great.

In addition to CurrentUserFactory, we floated CurrentUserProvider.

Tentative renaming.

I apologize for coming in late here, but I'm baffled as to why we're re-architecting this based on the need to change the current user mid-request.

The point of moving away from global $user is to avoid global, mutable state, because it makes our code complex and unpredictable. What we're doing here is just a fancy version of global $user. As stated in this thread, it's still totally unsafe to hold on to a current user object.

I cannot think of a single use case for supporting this. Anytime I've changed the current user in the past was because I was calling code that referenced global $user. We should no longer have code that does that, and if we do, that code needs to be fixed.

The only case I see in core is cron, which can easily be handled with a separate front controller.

The current trend of injecting "managers" and "factories" instead of what you actually need is not a good one. Let's not continue it here.

If we don't support changing it directly, we need to support a mechanism to switch to a different user for the *next* request. Think logging in, or masquerade module, or even running cron.

IMHO, we have to support it.

Login/Masquerade: session.inc does not support to save a session for something that is not current_user, so we have to switch on request, save it to the session, so that we can become that user on the next request. Yes, we have issues to replace that with symfony code, but we've been trying that for years now, with different approaches and haven't gotten far.

Cron is an interesting example, we could theoretically have a specific controller thing that that enforces an anon user from the beginning. We currently have two routes to run cron though, one that needs an administrative permission and does a redirect back to the status report and the other that does checks access based on the cron key. So that idea would only work for the second case?

Also my use case that I mentioned in IRC with simplenews.module generating personalized e-mails in the context of a user (think access, history/read information for that user, views current user arguments) and so on either requires being able to switch it *or* not having current user as global state at all, which would require passing it through everything that needs it, which seems more than unrealistic to me. It also *not* related to the request (the object or in "info from browser" concept) in anyway. So I don't see how ideas like re-boot kernel with a different request helps.

As much as @msonnabaum's theory about "global state should be unmutable" might be true, I don't see how that could possibly work in Drupal (not just current user, also for example current language).

Suppose better to fix code and pass user as required argument.
The question is about services that need current user like Comment manager that we need to split #2188287: Split CommentManager in two other case here is #2081585: [META] Modernize the History module but in perfect world current user should be passed here and UID should be used as static cache key or just skip the cache.

In my pov services should not depend on user at all.
Controllers need to get user from request context.

EDIT Masquerade module does logout/login so does not affected this change, the module just need to save original UID in new session somewhere

Yes, it is not ideal to have global
state, everyone understands that, though we cannot try to sacrifice usecases which aren't really rare, as berdir has proven.
Drupal is not the system known for the best possible architecture but for a system which is flexible enough as well as has many features out of the box.

As a compromise we should try to keep out the current user out of as many places as possible. One small example, which might be worth to consider, would be to figure out how to integrate $user->hasPermission with #access on render arrays directly. '#access_permission' => array('name'), so just the render service knows about users.

I dont see why we are discussing if we want to support changing user mid-request here.
Unless i am mistaken, there are two more bullets unrelated to that in OP.
The current_user approach we have now in HEAD, will never work and that was proven with the request service, which had the same approach and made everyone in symfony world endup to the request_stack.
Lets move on here, and open a new issue and discuss the setAccount() method. we can even deprecate it to show that using it is a bad practice and in the meantime fix session to not change the user but only send the related cookies. And also the cron service..but i dont see why we need to block this issue on it

btw, we can later add a CurrentUserTrait for this ($this->getCurrentUser() or $this->getAccount() or whatever) and hide the authentication part in there for those who dont like it

I think we need the ability to set the account to be able to replace those cases where we currently have to set it directly on the container.

k, so we basically made a complex version of global $user, that's actually more work to stub in tests. Seems legit.

go damian!

To the guy who is currently on the plan @damiankloip it is an empty patch.

I tried recreate the patch by combining the patch in #65 and interdiff in #70 but I am unable to apply the interdiff cleanly. Perhaps interdiff is also wrong.

Mark did mention something in IRC yesterday (Just to put it out there) that if we are doing something like this then we might as well just store the current user on \Drupal, i.e. Drupal::currentUser/Drupal::setCurrentUser or something. I personally prefer this way. probably biased though.

Right, we could actually do quite a bit, but using your way allows us to replace the component with different implementations without adding knowledge to our Drupal class.

I'm not sure I'm following all the debates here, but wouldn't it make more sense to simply define the current_user as:

• A synthetic service;
• Synchronised on the scope "request"

That seems to make a lot more sense to me then this weird factory.

The back-and-forth on this issue over the past few months is more then painful (we are storing the account on the request, that's cool, no? oh, no, let's store it as a synchronized current_user service instead synchronized with the request scope; oh no, let's not make it synchronized; oh, no let's store it inside a factory service).

How do we know this is the last design?

@Damian - If I understand scopes, that would mean changing the current user would be a subrequest? If so, #57 seems to be arguing that even that isn't good enough, and we just need it to work like it did in D7.

Upfront: Sorry, while also subscribed to comments, I mainly followed this issue via discussions in IRC thus far.

We're spending at least half of the time in this debate to discuss potential use-cases and whatnot. That's what a technical requirement is and what test expectations are for.

So, architecturally and strictly technically speaking, the one and only proper way to move forward is to actually codify the possible and officially supported expectations in tests.

From there, we should theoretically have a better picture and understanding of which proposal works and which does not.

That said: Overall I agree with @msonnabaum:

The entire current_user service should not be a thing in the first place. — We're just duct-taping a flawed architecture, because we're not injecting the dependency into all consumers, and thus have to rely on a global state construct instead.

→ Whether that global state construct is global $user or \Drupal::currentUser() factually makes no difference.

Due to that, there's little point in turning that global state construct into a "factory" service. As long as it is a global state construct, any additional complexity like a factory or whatnot does not solve a single problem.

On architectural grounds, @Damien Tournoud's input of turning the current_user service both into a scope as well as synchronized service makes some more sense, since that at least allows us to properly "invalidate" all dependent services by re-instantiating them upon scope-change.

But in the end, our intention/use-case of changing the current user/language/context "mid-request" is not to invalidate/re-instantiate services upon scope change. Instead, we want to enter a new context with changed parameters, so as to (temporarily) execute some operations within that context. And upon completion, we want to switch back and revert to the original context — without re-initializing the context, because stuff like [external] authentication either does not support that or the attempt to do so would be terribly slow.

→ That's the concept of a stack. A context stack.

We had plenty of in-depth discussions about a context stack ~2-3 years ago already. One of the primary consumers back then being Panels, Context, and similar stuff. Somehow, all of that collectively fell off our radar. It seems it would be a good idea to take a big step back, take a deep breathe, and re-evaluate that prior art.

Given that Symfony 3.0 introduced a new RequestStack, which seemingly appears to attack a very similar conceptual problem, it would be a good idea to take some time to study that revised architectural approach in-depth. (We should actually update, immediately, TBH.)

Speaking of, in the end, this entire debate leaves me with the impression we're trying to solve a generic architectural problem that should actually be solved upstream instead. Did we reach out to @fabpot and others already?

As commented in #24 with the link to http://symfony.com/blog/new-in-symfony-2-4-the-request-stack, they clearly came to the conclusion that all that scope and synchronized stuff for request did not work. So they deprecated it in favor of this is IMHO the same approach, just applied to current user. So IMHO, we are already following upstream?

I don't get why it has to be called factory as it doesn't factor anything, it just contains it. And with the impersonate issue, setAccount(), it would essentially become a stack.

But +1 on asking @fabpot on his opinion.

But the real problem is that the request is not a service; the request is a value object... like the response. And as you know, the response instance has never been accessible from the container, and that never caused any problems.

Quoting http://symfony.com/blog/new-in-symfony-2-4-the-request-stack ... it is just drupal who decided that users should be as simple as possible to access. They even throw it away in 3.0

Why are synthetic services a bad idea here: Synthetic services breaks all kind of lazy-execution, so we would have to create the current user as early as the request to really be sure.

1. You would have to convey to people that they have to enter a new scope, and set the user on the container again. Hmm, yes, that will be the obviously choice for people.
2. All services would have to have a setCurrentUser() method to synchronize this, so you have to make sure people use this pattern and not constructor injection.
3. You also have to try and make sure people understand what '?@current_user=', so you forget an '=' sign, your implementation is broken. Also - great.
4. You would STILL have the same issue where people should not be storing this on their classes, and if they did, would also break the implementation.
5. You are back to having to inject the whole container into things to change the user
6. The current_user service would also have to be synthetic? So our auth service for example would have to inject the container and set this? would be strange.

And note: All this fancy synchronization/scope handling does not work at all for non services code like plugins are, which also might be persisted during different user scopes.

Marked #2102027: Add back the request scope to the current user service as duplicate.

@sun, #87

→ That's the concept of a stack. A context stack.

We had plenty of in-depth discussions about a context stack ~2-3 years ago already. One of the primary consumers back then being Panels, Context, and similar stuff. Somehow, all of that collectively fell off our radar. It seems it would be a good idea to take a big step back, take a deep breathe, and re-evaluate that prior art.

I am fairly new to Drupal, so am not aware of the prior work on context stacks that you mention - is this visible somewhere ?

I've used Symfony in several applications with complex user authentication / authorization / switching scenarios, and when necessary I've handled changing / restoring the user "object" and user "session" in a similar way to the Symfony 2.4 request stack, so +1 for that general concept.

I know there are other open issues on session, but I'm a bit surprised session handling doesn't feature more in this discussion. My experience is that switching or "stacking" the user "context" often implies switching or "stacking" the session at the same time. In Drupal, Is it really possible to switch the user object without regard for the state of the session ?

We discussed this at length in today's WSCCI meeting. Here's the conclusion:

Part of the problem is there are really three different issues being discussed here, overlapping.

1) Currently, due to dependencies we are implicitly running the login code twice: Once when creating various services in the container (the current_user service self-instantiates) and once in a request listener.

2) Some services that have a current_user dependency cause the user to be authenticated too early, and because users are entities and entities depend on all kinds of crap it can cause circular dependency issues.

3) The question of whether or not the current user should be a mutable value.

For the first two points, I believe we can address it by removing the authentication listener and JUST authenticating on the fly by the factory specified in the DIC. Then, we add a second service that is a user proxy. Whether that's a transparent proxy (ie, implements AccountInterface) or an explicit wrapper, I don't much care. Then most services that need the current user as a dependency can pass that in their constructor while those that would cause a circular dependency can break it by using the proxied version instead. Note that using the proxied version in the constructor would *still break things*, so don't do that. We'll need to determine how to communicate that properly.

Note: Mark Sonnabaum pointed out that the number of services we have that need the user as a constructor parameter is a problem in and of itself, and most if not all should be accepting a user object as a parameter to the relevant method instead. In general, I agree. However, firstly there are cases where that's simply impossible (eg, the service is behind a COR so the COR object doesn't know it needs to get the user object to pass along in the first place) and secondly I don't want to hold up this issue and those that depend on it while we clean up those cases. Such clean up and can and should happen in parallel. Anywhere that a service needs to use the proxied user I would argue is a design flaw. We cannot fix all of those design flaws in Drupal 8, but we can at least highlight them clearly so we know where they are to fix later (in 8.x or 9.0, depending on the situation).

As for point 3, whether or not the current user should be a mutable value, the answer is simple: Abso-frackin-loutly not. We've poured thousands of hours into eliminating global state and mutable context for a reason: those things make code brittle. We are *not* going to build in a mechanism to encourage code to be brittle. Not happening. Anywhere that functionality is needed it's a sign that we ought to be doing something else instead. This is a design flaw we *cannot* push off to later.

For cases like cron or masquerade, the answer is to use a subrequest. Subrequests are the only way that request-based context can and should change. (The request itself, the user, etc.) If cron wants to run everything as user 0, it can fire a subrequest that does that. It doesn't mess with global state.

For cases like simplenews, if it needs to change the global state of the user then it means it is missing functionality to begin with. That is, it doesn't support using "a user" as a context, it only supports "the user viewing the site" as a context. Faking the latter as a way to support the former is entirely wrong, and is the sort of spaghetti logic we're trying to eliminate from Drupal entirely. The solution is to let simplenews take a passed user object and pass that along as needed, and use *that* as a driver to find other places that are depending on the current user incorrectly. If we run into places where it simply cannot due to a design flaw, then it can fake it using a subrequest, not global mutation. A subrequest is a totally fugly way to do it... because it's a totally fugly thing to do. We shouldn't hide the pain of doing something wrong, because then we forget that it's wrong and keep doing more of it. And that's how we got into the mess we're in to begin with. :-)

In short, out-of-band dependence on the current user is an ugly we can live with, and I don't think we can eliminate entirely. Mutable current user is an ugly we cannot allow, as it undermines everything else we've been trying to do in Drupal for the past 3 years. Berdir, I'm happy to chat with you about how to refactor simplenews so that it doesn't need to do that sort of hackery anymore if you have time at some point (perhaps an upcoming conference).

As I said before - masquerade does not need sub requests! It does logout->login and storing old UID in new session, nothing more

@damiankloip Currently code is broken/not-ported so I follow core.

Core does user_logout() with following hook_user_logout() and user_login_finalize() with hook_user_login() so both should care about container state, the masquerade will call them coherently and redirect the visitor back to previous URL

@andypost, great, so how do you do that with what the guys are proposing above then? When you cannot change the user during a request?

Just to be clear, I don't understand why we assume that drupal is a academial project without real world usecases, even inside core. Yes (INSERT arbitrary point), but reality is different.

Personally I would just love to get the work in comment 94 in.

94: 2180109-94.patch queued for re-testing.

Hi folks! Joining the party late here. I read through all of the issue comments, but not through the patch or any IRC conversations.

1. I agree with Berdir that Drupal\Core\Session\UserSession is a value object, and therefore, doesn't make sense as a service.
2. CurrentUserFactory is one idea for how to wrap the value object with something that makes sense as a service; however, it's a bad name (it's not a factory, it's just a wrapper), and by being a wrapper, it's exposing unnecessary complexity to all consumers of the service.
3. I like Crell's idea of a transparent proxy instead. In other words, what if we create a class (UserSessionProxy?) that implements all of AccountInterface, has a protected getAccount() method that internally gets the correct object from the authentication manager, and forwards all methods to that protected object?
4. A benefit of a transparent proxy is that all consuming code can still just deal with AccountInterface. But the consuming code could safely hold onto the proxy object (i.e, receive it in the constructor). Additionally, unlike with a factory/wrapper, the proxy's getAccount() method would be protected, so consuming code would be unable to get and hold onto the underlying value object.
5. #98 implements the transparent proxy via Symfony's lazy services feature. However, that would require us to add the ProxyManager component, and I don't think that's worth doing, especially since I think having control over the proxy class to this extremely important value object will come in handy.
6. For example, the proxy class can have a setRequest() method, and implement whatever logic we decide to be appropriate for handling a subrequest.
7. Additionally, we could decide to provide a MutableUserSessionProxy class that extends UserSessionProxy with a public setAccount() method. This would not be the class assigned to the 'current_user' service in core.services.yml, but for example, if we decide that it's legitimate for tests to change the current user without going through a subrequest, then those tests can change the service to use the mutable proxy. And, if it turns out some contrib use cases require that as well, then they can do so too, despite whatever warnings we want to add into the class docs stating why doing so is not recommended.

Thoughts? Would this address everyone's goals?

Just playing around with the idea. It is still odd that we consider explicitness as a bad idea in the first place.

Some fixes.

@dawehner if this helps at all, I've itemized each remaining global $user replacement into it's own patch so you can grab the whole thing or each one you need:
#2213899: Remove global $user wherever possible (outside bootstrap and authentication) Round 2

Retitling for the duration that the latest patch on this issue reflects this approach.

+++ b/core/lib/Drupal/Core/Authentication/LazyAccount.php
@@ -0,0 +1,209 @@
+  /**
+   * Implements the magic method for getting object properties.
+   */
+  public function &__get($name) {
+    return $this->getAccount()->{$name};
+  }
+
+  /**
+   * Implements the magic method for setting object properties.
+   */
+  public function __set($name, $value) {
+    $this->getAccount()->{$name} = $value;
+  }
+
+
+  /**
+   * Implements the magic method for isset().
+   */
+  public function __isset($name) {
+    return isset($this->getAccount()->{$name});
+  }
+
+  /**
+   * Implements the magic method for unset.
+   */
+  public function __unset($name) {
+    unset($this->getAccount()->{$name});
+  }
+

Do we need these? There should be no acceptable reason for consuming code to access properties directly, since this is only about implementing AccountInterface, not UserInterface. In fact, I wonder if we should even keep these but have them throw exceptions.

@effulgentsia
Oh I just assumed we would need it, as maybe some code will check for UserInterface and in that case do something with it.

In general the current failure is somehow caused by a memory problem. Is that a know problem, maybe related AGAIN with serialization?

112: current_user-2180109-112.patch queued for re-testing.

.

1. +++ b/core/lib/Drupal/Core/Authentication/LazyAccount.php
   @@ -0,0 +1,213 @@
   +/**
   + * @file
   + * Contains \Drupal\Core\Session\LazyAccount.
   + */
   +
   +namespace Drupal\Core\Authentication;
   

   namespace mismatch.

2. +++ b/core/lib/Drupal/Core/Authentication/LazyAccount.php
   @@ -0,0 +1,213 @@
   +class LazyAccount implements AccountInterface {
   

   Is this a good name for this?

   Should it maybe be AccountProxy instead?

3. +++ b/core/lib/Drupal/Core/Authentication/LazyAccount.php
   @@ -0,0 +1,213 @@
   +    // Resets the current user so that authentication takes part again.
   +    $this->account = NULL;
   

   // Reset the current user to ensure that new calls will receive the accordingly authenticated user?

   Better suggestions anyone?

4. +++ b/core/lib/Drupal/Core/Authentication/LazyAccount.php
   @@ -0,0 +1,213 @@
   +    // Prevent people from setting a lazy account object into here.
   

   Maybe something like this instead:

   // If the passed account is already proxied, use the actual account instead to prevent loops.

5. +++ b/core/lib/Drupal/Core/Authentication/LazyAccount.php
   @@ -0,0 +1,213 @@
   +  /**
   +   * Implements the magic method for getting object properties.
   +   */
   +  public function &__get($name) {
   +    return $this->getAccount()->{$name};
   +  }
   +
   

   Yes, can we try to leave this out? Wondering what's going to happen...

6. +++ b/core/modules/system/tests/modules/router_test_directory/lib/Drupal/router_test/TestContent.php
   @@ -58,9 +58,9 @@ public function test11() {
      public function testAccount(UserInterface $user) {
   -    $current_user = $this->currentUser();
   -    \Drupal::getContainer()->set('current_user', $user);
   -    return $current_user->getUsername() . ':' . $user->getUsername();
   +    $current_user_name = $this->currentUser()->getUsername();
   +    \Drupal::getContainer()->get('current_user')->setAccount($user);
   +    return $current_user_name . ':' . $user->getUsername();
   

   Given that we use \Drupal:: anyway, simplify to use currentUser() ?

1. +++ b/core/lib/Drupal/Core/Authentication/LazyAccount.php
   @@ -0,0 +1,213 @@
   +class LazyAccount implements AccountInterface {
   

   AccountProxy? (Technically the fact that it's lazy is an implementation detail.)

2. +++ b/core/lib/Drupal/Core/Authentication/LazyAccount.php
   @@ -0,0 +1,213 @@
   +  public function setAccount(AccountInterface $account) {
   

   This method makes me nervous. Why are we allowing this? I can't think of a (good) use case.

3. +++ b/core/lib/Drupal/Core/Authentication/LazyAccount.php
   @@ -0,0 +1,213 @@
   +  /**
   +   * Implements the magic method for getting object properties.
   +   */
   +  public function &__get($name) {
   +    return $this->getAccount()->{$name};
   +  }
   

   I agree that these aren't really necessary.

Another advantage of the transparent proxy approach: Code that takes an AccountInterface as a parameter or constructor dependency can, under testing, take a real account object rather than the proxy object, which simplifies the test.

Yeah I am not sure whether the response from crell was meant like this. We discussed usecases for quite a while

Until all the code in core that can possibly be executed during a cron run or queue processing has the user injected, there are use cases for the setAccount() method.

Pretty wtf. let's see whether it passes.

I think this should fix those fails.

Not convinced that we actually have to. Let's talk about that in a follow, ...

128: 2180109-128.patch queued for re-testing.

FYI if it helps any, #128 fails the same in the CLI for:
\Drupal\file\Tests\UsageTest

Exception Notice     Cron.php            83 Drupal\Core\Cron->run()
    Undefined index: user...

\Drupal\file\Tests\DeleteTest

Exception Notice     Cron.php            83 Drupal\Core\Cron->run()
    Undefined index: user

Drupal\system\Tests\Session\SessionTest

Fail      Other      SessionTest.php    170 Drupal\system\Tests\Session\Session
    Session was not empty.

Though particular to the SessionTest:
the CLI show's $_SESSION as undefined (expected I assume). But the UI fails with $_SESSION contents of:

array (
  'batches' => 
  array (
    3 => true,
  ),
)

Which is not the case in the UI without the patch. And UI only has that error and not the two "file" test fails.

This fixes the two cron fails locally.

Then we probably need to do something with the Cron::run code based on this patch.

Re: #128 this still needs to be addressed but if #134 passes maybe it would work as it should now and we can remove the todos and scrap the remaining $GLOBALS['user'] from that Cron::run function?

Well this does what I suggested in #135, it should be the same result and hopefully a 1 fail positive result for both.

@damiankloip re #138 It likely won't "work". I was only reading through what was there, playing with what I saw and noticed other patches were replacing global $user and that pattern seemed to fix the 2 fails for cron locally. So disclaimer I don't know what is supposed to happen there, just saw an opportunity to push this patch one inch closer...

Though unless #134 is a testbot hiccup it seems I went in the wrong direction and both my patches should be ignored, but I'll retest it to be sure.

134: 2180109-user-proxy-134.patch queued for re-testing.

Talked to @damiankloip on IRC and now I understand: $original_user should be a referenced variable to the original proxy object and be overwritten on the next call. But I'm confused at why #134 is failing so bad and #136 is doing what I wanted #134 to do...

This are changes you can do to get the session test green.

Whether or not we should allow the setting of an account has been going on for the duration of this issue (and a fair bit outside it). WE NEED TO REACH CONSENSUS ON THIS :)

I haven't read the patches since #119, so not sure how much a setAccount() is still needed by core itself. However, I'll reiterate my position from #109.7: if we can make core work without it, then the default 'current_user' service shouldn't have it, but we should provide a MutableAccountProxy class for tests and questionable contrib projects to use if necessary.

Whether or not we should allow the setting of an account has been going on for the duration of this issue (and a fair bit outside it). WE NEED TO REACH CONSENSUS ON THIS :)

To be honest I kinda think that people who still discuss this should have a look at what Drupal is today. This patch itself changes the current user in varios places, from session.inc over cron and simpletest.

I'm patiently waiting for a good outcome with this issue. I think that #2205295: Objectify session handler may be stalled until we can benefit from the work of this patch.

That one is sadly especially hard. Yes #2205295: Objectify session handler would help a little bit, but the actual problem is probably not in there.

Okay here is the thing, all changes since #2180109-136: Change the current_user service to a proxy changed session.inc and how it works internally. This is problematic because
acccessing the current user in session.inc potentially triggers authentication again.

So the newest patch started back from #136 and just fixes the test failure. From there one the bad state of session.inc
needs to be accepted, as this patch does not interfere with it.

The test failure is caused by having $_SESSION initialized lazyly, which means it is not available all the time.

After some discussion with tim:

• Introduced an interface for the getAccount and setAccount methods
• Moved the interface and implementation into Core\Session instead of Core\Authentication
• Typehint the new interface

Better ...

Okay.

Looks sane to me. thanks dawehner!

1. +++ b/core/lib/Drupal/Core/Routing/Enhancer/AuthenticationEnhancer.php
   @@ -55,7 +66,7 @@ public function enhance(array $defaults, Request $request) {
   -        $this->container->set('current_user', $anonymous_user, 'request');
   +        $this->currentUser->setAccount($anonymous_user);
   

   does that mean that we can get rid of ContainerAware from this class?

2. +++ b/core/lib/Drupal/Core/Session/AccountProxyInterface.php
   @@ -0,0 +1,26 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setAccount(AccountInterface $account);
   ...
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getAccount();
   

   i suppose the use of inheritdoc here is wrong:) where does it inherit from?

1. +++ b/core/lib/Drupal/Core/Cron.php
   @@ -78,10 +78,8 @@ public function run() {
   +    $original_user = \Drupal::currentUser()->getAccount();
   

   This should have an @todo that setting the user is evil and Cron should do it's own kernel or service container instead, which does not do any authentication in the first place if it only wants to operate with anonymous users.

2. +++ b/core/lib/Drupal/Core/Session/AccountProxy.php
   @@ -0,0 +1,196 @@
   + * Contains \Drupal\Core\Authentication\AccountProxy.
   

   does not match the namespace.

3. +++ b/core/lib/Drupal/Core/Session/AccountProxy.php
   @@ -0,0 +1,196 @@
   +/**
   + * A proxied implementation of AccountInterface.
   + */
   +class AccountProxy implements AccountProxyInterface {
   

   This should have more documentation why we need this.

   "We need this for legacy code which relies on switching the global user instead of injecting the user or starting a new request." -- could be improved.

4. +++ b/core/lib/Drupal/Core/Session/AccountProxy.php
   @@ -0,0 +1,196 @@
   +  /**
   +   * Set the current wrapped account.
   +   *
   +   * @param \Drupal\Core\Session\AccountInterface
   +   *   The current account.
   +   */
   +  public function setAccount(AccountInterface $account) {
   

   This should have a big fat warning "do not use this! Inject the desired user into dependent code instead".

5. +++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
   @@ -1087,7 +1088,7 @@ private function prepareEnvironment() {
   -    $this->container->set('current_user', drupal_anonymous_user());
   +    $this->container->set('current_user', new AnonymousUserSession());
   

   why doesn't this use ->setAccount() on the proxy? Are you switching out the proxy here?

6. +++ b/core/modules/system/tests/modules/session_test/lib/Drupal/session_test/EventSubscriber/SessionTestSubscriber.php
   @@ -30,6 +30,9 @@ class SessionTestSubscriber implements EventSubscriberInterface {
   +    // Trigger the authentication in the test to ensure that $_SESSION has the
   +    // needed data. On the longrun we remove the globalness of $_SESSION.
   +    \Drupal::currentUser()->getAccount();
        $this->emptySession = intval(empty($_SESSION));
   

   we are not removing the globalness of $_SESSION, since it is global by definition in PHP. And this should have an @todo that we should remove this call once session.inc and related session handling is refactored properly.

1. +++ b/core/core.services.yml
   @@ -730,11 +730,10 @@ services:
          - { name: event_subscriber }
        arguments: ['@authentication']
      current_user:
   -    class: Drupal\Core\Session\AccountInterface
   -    factory_method: authenticate
   -    factory_service: authentication
   -    arguments: ['@request']
   -    synchronized: true
   +    class: Drupal\Core\Session\AccountProxy
   +    arguments: ['@authentication']
   +    calls:
   +      - [setRequest, ['@?request=']]
      asset.css.collection_renderer:
        class: Drupal\Core\Asset\CssCollectionRenderer
        arguments: [ '@state' ]
   

   I'm glad that the on-construction-authentication via the factory method is gone here. I'm sad that a new dependency on the synchronized request service is introduced.

   From reading the rest of the patch I infer that the intention behind this dependencies are the following:

   1. Have a way to authenticated a user on demand
   2. Have a way to invalidate the user session when the request object changes

2. +++ b/core/lib/Drupal/Core/EventSubscriber/AuthenticationSubscriber.php
   @@ -40,18 +39,6 @@ public function __construct(AuthenticationProviderInterface $authentication_prov
      }
    
      /**
   -   * Authenticates user on request.
   -   *
   -   * @see \Drupal\Core\Authentication\AuthenticationProviderInterface::authenticate()
   -   */
   -  public function onKernelRequestAuthenticate(GetResponseEvent $event) {
   -    if ($event->getRequestType() == HttpKernelInterface::MASTER_REQUEST) {
   -      $request = $event->getRequest();
   -      $this->authenticationProvider->authenticate($request);
   -    }
   -  }
   -
   -  /**
       * Triggers authentication clean up on response.
       *
       * @see \Drupal\Core\Authentication\AuthenticationProviderInterface::cleanup()
   

   So, before the authentication did only take place when entering a master request. This is actually the proper approach because it is save to expect that those authentication parameters the client supplied will not change during the request/response cycle. There is no point in reauthenticating a subrequest, and thus there is no point in invalidating the user session when the request object changes. Let's just keep that behavior.

3. +++ b/core/lib/Drupal/Core/Session/AccountProxy.php
   @@ -0,0 +1,196 @@
   +class AccountProxy implements AccountProxyInterface {
   +
   +  /**
   +   * The current request.
   +   *
   +   * @var \Symfony\Component\HttpFoundation\Request
   +   */
   +  protected $request;
   +
   ...
   +  /**
   +   * Sets the current request.
   +   *
   +   * @param \Symfony\Component\HttpFoundation\Request $request
   +   *   The current request.
   +   */
   +  public function setRequest(Request $request) {
   +    $this->request = $request;
   +    // Reset the current user to ensure that new calls will return the correct
   +    // user based on the request.
   +    $this->account = NULL;
   +  }
   

   Let's get rid of that for the reasons stated above.

Currently that event listener is invoked usually after the current_user factory service has authenticated once anyway.

That's right, I consider this as a bug and not as a feature. When accessing the current user (proxy) before the request was authenticated, I'd rather expect an anonymous user to be returned. Authentication should only happen at one (deterministic) point in time in the request/response cycle.

Saying no auth should ever happen on a sub request is not right IMO.

Please give an example.

Anything can ask for a current user at any point before the auth event has happened.

This is exactly the reason for the proxy approach. A service consuming the current_user proxy on construction time will no longer be stuck with the anonymous user. The authentication fires long before controllers actual will act on the injected object. If we discover that there is legacy code requiring an authenticated user before the event fires, we always can temporarily move the authentication step into a bootstrapping phase. However I doubt that this will be necessary.

If something like simplenews decided to use a sub request approach in the future, you would need to pass in a request and authenticate.

Nope. Authentication is different to masquerading. If simplenews decides to run under another user, it always can choose to replace the currently active user account (i.e. via the proxy). This is possible independent of the approach taken (with / without subrequests). Authentication (read: validate client supplied credentials) must be exactly once early in the request. The client supplied credentials never change during a request / response cycle, regardless of how many subrequests the main request triggers.

why doesn't this use ->setAccount() on the proxy? Are you switching out the proxy here?

Note: This is just a temporary container, without anything actually booted up. We especially can't create an authentication manager
which is needed for that.

I'm glad that the on-construction-authentication via the factory method is gone here. I'm sad that a new dependency on the synchronized request service is introduced.

I would like to postpone the cleaning up, this here solves actual problems, while not relying on the request is certainly nice, but
does not solve us multiple criticals.

1. +++ b/core/lib/Drupal/Core/Session/AccountProxy.php
   @@ -0,0 +1,206 @@
   + * The reason why we need an account proxy instead the simple account interface
   

   should be "instead of".

2. +++ b/core/lib/Drupal/Core/Session/AccountProxy.php
   @@ -0,0 +1,206 @@
   + * If you would not authenticate lazy, you potentially authenticate multiple
   + * times, execute code in constructor phase and cannot change the current user
   + * in multiple places all over the place.
   

   "This proxy object avoids multiple invocations of the authentication manager which can happen if the current user is accessed in constructors. It also allows legacy code to change the current user where the user cannot be directly injected into dependent code." -- is that better?

3. +++ b/core/lib/Drupal/Core/Session/AccountProxy.php
   @@ -0,0 +1,206 @@
   +   * Setting the current account is highly discouraged! Instead the desired user
   

   "Instead, make sure to inject the desired user object into the dependent code directly" -- is that better?

4. +++ b/core/lib/Drupal/Core/Session/AccountProxy.php
   @@ -0,0 +1,206 @@
   +  /**
   +   * Gets the current account.
   +   *
   +   * @return \Drupal\Core\Session\AccountInterface
   +   *   The current account.
   +   */
   

   Actually this documentation should be on the interface. Same for setAccount(). Use inheritdoc here.

But otherwise I think we should move forward with this. I don't see the point of having the authentication in a fixed place at the beginning of the request - it only needs to happen if the account is actually used.

And to re-iterate one more time: we need to setAccount() method on the proxy because there will be Drupal 8 code needing to set it, unfortunately. Example: Simplenews needs to built an email for a specific user where a View is invoked, which renders some entity, which invokes the translation system to render some text, which needs access to the user's preferred language. It can be very hard to impossible to pass down that language over 5 levels - we are not there yet in all situations in D8.

Actually this documentation should be on the interface. Same for setAccount(). Use inheritdoc here.

This is a little bit embarassing.

This is not necesarry an API change, given that we didn't had an API before.

+++ b/core/lib/Drupal/Core/Session/AccountProxy.php
@@ -0,0 +1,198 @@
+  public function getRoles($exclude_locked_roles = FALSE) {
+    return $this->getAccount()->getRoles();
+  }
+

You're not passing $exclude_locked_roles on to $this->getAccount()->getRoles().

klausi opened a new pull request for this issue.

I applied the patch and tested issue #2113681: Node author can't be set when posting via HAL. It seems to be working.
Fixed issue in comment #171

Cross-post with MartijnBraam, sorry.

Just fixed the $exclude_locked_roles parameter, interdiff https://github.com/klausi/drupal/commit/d7cc212bebe8c522384835046f837e9d...

Otherwise I think this is RTBC. I played around with avoiding the \Drupal::currentUser()->getAccount(); call in SessionTestSubscriber, but the alternative is only to initialize the session at some hardcoded place, which we don't want. So the implication is right now that you have to call \Drupal::currentUser()->getAccount(); before using global $_SESSION when no user account is involved at all. This can/will be fixed by refactoring the session system, which is not part of this issue.

So we only have to wait for the testbot now.

since everyone ignored me, i will roll a patch instead

it helps if you also remove the setContainer call for services.yml, right?:)

OH paris, we don't ignore you. the discussion just got crazy so we accidentally skipped it.

I'd also like to see a note either on the setCurrentUser() method or in the Cron class (or both) saying "don't do this, it's for legacy only, cron needs refactoring". Otherwise, I am +1 on the latest patch.

Committed/pushed to 8.x, thanks!

Cross-posted with Crell, but either way I'd prefer to see an issue about that. It's not cron that needs refactoring. Cron just needs to use whichever the recommended mechanism is to ensure that anything which might get rendered during the cron run (for example nodes during search indexing) is rendered as if it's being viewed by the anonymous user. Once we have that mechanism, cron could be converted to it, but it's just using what we have at the moment.