Create a current user service to ensure that current account is always available

Add Remaining tasks that have same problem and related issues

Awesome!

@@ -0,0 +1,65 @@
+} ¶

Trailing space, needs newline (nitpick)

Should we convert one of the problematic ones listed in the related issues above to demonstrate it works?
Should we add a unit test?

@@ -0,0 +1,65 @@
+} ¶

still persists (nitpick)

but looks good otherwise

Add related issue

New title and updated issue summary

This blocks a critical (#2040065: Remove _account from request and use the current user service instead.), so it's critical.

It's also approved by catch and I. Tagging.

agree with #4 - one of problematic issues should be used to demo the introduced concept

Updated issue summary.

Should this also remove _account from the request object or is that a follow-up?

Why are we removing _account from the request at all?

Why have it there if there's a service handling it?

The approach is nice but require to inject the service into all controllers that accept request as well so probably better:
1) leave _account in global request object
2) when no _account in request use \Drupal::account()

anyway this breaks encapsulation or require to inject service to all controllers

I don't see a problem with injecting this into controllers that need it.

Right now, any controller can just put Request $request in their method and it is automatically passed to them. That's a really nice feature.

Request is automatically passed, _account gets lost all over the place like #2040065: Remove _account from request and use the current user service instead. so you either have to explicitly set it whenever creating a request or can't rely on it being there anyway.

Also the way _account is accessed from request is really, really ugly.

Another reason we should remove _account: https://drupal.org/node/2036351#comment-7752121

@dawehner - we either need to do it here, or open another critical follow-up. I'm not too fussed either way but since there's disagreement on doing it at all, I think we should probably make the final decision here regardless - I don't want to have the same conversation twice about the same thing once immediately after the other.

The CurrentAccount service introduced in this patch (registered as current_account) is very close to Symfony SecurityContext service, usually registered at security.context.

This brings us one step further in our piece-by-piece reimplementation of Symfony's security component. Is it time to stop and just leverage the component that exists?

Damien: I am not going to burn any karma on that drastic of an API change at this stage of the game. Our authorization system is also, as I understand it, very different than Symfony's so it's not like we could just drop it in without a lot of work.

Crell: I did the research, and described what need to happen. It's clearly less complicated than rebuilding the whole thing from scratch, piece by piece. But of course, it's also fine to ship Drupal 8 with an half-backed API that nobody will want to touch with a ten-foot pole.

#22 makes sense because we already have bits of this invented here. Symfony have great doc about security so Drupal is implementing same code step-by-step...

Initially in #335411: Switch to Symfony2-based session handling was proposed to use SessionProxy then context approach #1260864: Convert global $user to a context key was wont-fixed by Crell and finally #1858196: [meta] Leverage Symfony Session components we have 40k patch that passes tests and implements listener on request|response and uses global $user object.

#1890878: Add modular authentication system, including Http Basic; deprecate global $user implements route enhancer to home-grown authentication system very similar to symfony's

As masquerade module maintainer I'd like to have swappable service but I think domain module will bring more questions.

@dawehner suppose more bugs will be filed when contrib will start porting to 8.x, all issues about _account are filed within One week as result of 1 day testing d7 upgrade and 1 day code sprint on issues #2047951: [META] Remove calls to deprecated global $user and $GLOBALS['user'] and #2048171: [meta] Replace user_access() calls with $account->hasPermission() wherever possible.

Suppose at the current release phase better to investigate all known issues and available implementations

#11 looks ok to me. I don't see why we'd try to expand the scope here beyond that. We just need a simple service that returns the current user.

Even if we ended up using symfony's security component in d9 I'd still keep this interface and delegate so that people didnt have to call ->getToken()->getUser().

One of the nice things about it being on the request is that when we use the controller resolver, a controller can always get the request passed to it, with no need for our static factory injection pattern.

This is really common for forms.

This will mean that a lot of forms will now need to get this service, and switch to needing full injection...

My last comment becomes almost obsolete if #2059245: Add a FormBase class containing useful methods goes in.

I agree that getting the current account from the request is what you would expect... but Symfony's Request object is very inextensible, and the only API that we can use for that is the current ugly, non-semantic and error-prone:

$request->attributes->get('_account');

The only way to do that right now would be to sub-class the Request object. I'm not sure I would recommend that.

$request->attributes->get('_account'); is procedural code with a dead-OO pelt draped over it.

blub

@@ -597,6 +597,12 @@ services:
+  current_user:
+    class: Drupal\Core\Session\AccountInterface
+    factory_method: authenticate
+    factory_service: authentication
+    arguments: ['@request']
+    scope: request

I want to have a test to verify that this does work cleanly through a subrequest properly. It should, but I want to be extra careful here. If we can test that it stacks properly then I am OK with committing this approach.

@@ -94,7 +94,7 @@ public function testControllerPlaceholdersDefaultValues() {
-  public function testControllerPlaceholdersDefaultValuesProvided() {
+  public function ptestControllerPlaceholdersDefaultValuesProvided() {

Was this supposed to be in the patch? (A lot of test methods have it now.)

@@ -7,10 +7,37 @@
+  public function __construct(HttpKernelInterface $http_kernel) {
+    // TODO: Implement __construct() method.
+    $this->httpKernel = $http_kernel;
+  }

This TODO seems TODONE. :-)

Much better. :-) I don't see how the test works, though. It's not changing the user that I can see, is it?

#37 looks great.

OK, let's do this thing.

OK. Committed/pushed to 8.x.

I opened #2073531: Use current user service instead of _account, remove _account from the request object for the follow-up.

2) is enough.