Fix all current_user set calls

Need an actual account proxy object! I was certain I already did this change in the current user issue, maybe it got lost somewhere along the way.

We either wait on https://drupal.org/node/2263329 or merge it here.

reroll

The patch in #3 still applied fine?

Yes, without any problem.

So what's the 'reroll' for? :)

I get your point :)

#2263329: CommentDefaultFormatterCacheTagsTest just works because nothing uses AccountProxyInterface in HEAD is in.

6: 2263339-6.patch queued for re-testing.

Fixed those failures. Adding current_user service to the setup in Drupal\system\Tests\KeyValueStore\StorageTestBase.

Simple reason is this test extends from UnitTestBase, not DrupalUnitTestBase. So there is no container ready like normal.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -1089,9 +1090,14 @@ private function prepareEnvironment() {
+    $account_proxy = new AccountProxy(new AuthenticationManager(), $request);
+
     // Run all tests as a anonymous user by default, web tests will replace that
     // during the test set up.
-    $this->container->set('current_user', new AnonymousUserSession());
+    $account_proxy->setAccount(new AnonymousUserSession());
+
+    $this->container->set('current_user', $account_proxy);
+

Is it really necessary to replace the current_user service here? Wouldn't it be enough to set the account on the proxy, in the same way like in restoreEnvironment?

But just above prepareEnvironment a new container is created:

    // Reset and create a new service container.
    $this->container = new ContainerBuilder();

So not sure how that would already have an account proxy object set on it?

I see.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -1229,7 +1235,9 @@ private function restoreEnvironment() {
-    $this->container->set('current_user', $this->originalUser);
+    $current_user = $this->container->get('current_user');
+    $current_user->setAccount($this->originalUser);
+

What happens if this is removed entirely? That seems to affect the original container which is not supposed to be modified during the test.

Sorry but that is not a problem of this issue IMO. This is just converting the instances. That hunk is just a straight swap. If we think restoreEnvironment is doing something wrong. I think we should talk about that in a new concentrated issue?

Rereading the patch I still do not like these changes. Most consumers of the current_user just need the AccountInterface part of it. The AccountProxyInterface part is only important for those services which are supposed to swap out the user instances (i.e. authentication).

Well, also, e.g. the Cron class uses this. Just because tests do not rely on this now does not mean they wont in the future. If we usually have an AccountProxy in the container, we should also have this in the container for tests too. Otherwise, first implementation that needs it in there will have to make this change anyway. You want as few discrepancies in the test env as possible, no? You could encounter subtle issues that tests may not catch if the real life implementation is actually using the proxy and not just a straight up account.

Everything which calls AccountProxyInterface::setAccount() is potentially very dangerous. Only those services which really need to change the user should declare a dependency on AccountProxyInterface. All the others should stick with AccountInterface, therefore it is not clear to me why it is desirable to change the mocks in CommentLockTest, LocaleLookupTest and ViewUIObjectTest.

Perhaps it makes sense to have an AccountProxy in DUTB. But even there I think it would be legit to delegate the responsibility of setting up a real AccountProxy to the tests which cannot get by with a simple account but genuinely require the proxy.

Would love to hear other people's thoughts on znerol's feedback.

@znerol: do you think this needs to be resolved in this patch or is this something that could be done in a follow-up?

Reroll for #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4.

I agree with #22 regarding TestBase, and with #26 regarding the specific test changes as out of scope for this issue unless there's a reason they're needed.

Regarding the hunk in TestBase, see #2275965: Remove all services from the TestBase container.

If you are mocking the container you should mock the services that would actually be in the container. If you are unit testing and just passing parameters into a constructor, use whatever you like to satisfy the interface.

But as mentioned in irc before, if zenerol feels that strongly about these few tests, do whatever gets it in. This was meant to be a quick issue tbh.

@znerol, so if we revert those parts, this is good in your opinion now?

I prefer getting in #2196241: Remove string translation services from TestBase container which will remove all services from TestBase. I do not see a benefit in changing FormCacheTest and StorageTestBase because of the reasons stated in #26.

@znerol it's fixed now.

Reroll. Nothing much left in it. Related: #287292: Add functionality to impersonate a user

Oh, indeed. In my point of view #37 should be incorporated into #287292: Add functionality to impersonate a user or in a followup thereof.

Let's try this again. A very simple re-roll.

Yep, looks like this is the only usage now that is not in a unit test.

Re-roll once again.

Ok, I believe znerol's feedback has been incorporated via other issues, so this should be safe to commit. Also, this seems like just a code clean-up, so no need for it to be major. It does make the code more robust, however, by switching to the user switching API, so seems legit as far as https://www.drupal.org/contribute/core/beta-changes goes.

Committed and pushed to 8.0.x. Thanks!

Oops. Based on dawehner's feedback in IRC, this should actually be using the new account_switcher service instead.

Rolled back for now. Should be an easy-ish fix tho.

Updated patch with account_switcher service.

@@ -88,7 +89,7 @@ function testCacheToken() {
    * Tests the form cache without a logged-in user.
    */
   function testNoCacheToken() {
-    $this->container->set('current_user', new UserSession(array('uid' => 0)));
+    \Drupal::service('account_switcher')->switchTo(new AnonymousUserSession());

It's always a good idea to end calls to ::switchTo() with a corresponding ::switchBack() after the code block that was executed as another user. Though that would not affect this test, because the entire container is rebuilt anyway for the next test, it's good to demonstrate good coding practice in core.

That sounds like a needs work.

Patch updated based on #53. Thanks.

Great, thanks.

Committed and pushed to 8.0.x (for real this time :)). Thanks!