AccountProxy can do unnecessary user loads to get an ID

Looks great in general I would argue!

+++ b/core/tests/Drupal/Tests/Core/Session/AccountProxyTest.php
@@ -0,0 +1,55 @@
+
+namespace Drupal\Tests\Core\Session {
+
+  use Drupal\Core\Session\AccountInterface;
+  use Drupal\Tests\UnitTestCase;
+  use Drupal\Core\Session\AccountProxy;

You can get rid of the indentation by using this: https://3v4l.org/vWFKR

Good idea. I'm wondering whether there is any situation where the id property gets out of sync?

I'm wondering whether there is any situation where the id property gets out of sync?

Well, its just set in two locations, but sure someone could change the session entry somehow directly, couldn't they?

+1, what about the protected property rename. No problem for 8.2 I think, we exclude protected there, but not sure about 8.1, where we really try to keep changes as minimal as possible?

+++ b/core/tests/Drupal/Tests/Core/Session/AccountProxyTest.php
@@ -0,0 +1,53 @@
+    $account_proxy = new AccountProxy();
+    $this->assertSame(0, $account_proxy->id());
+    $account_proxy->setInitialAccountId(1);
+    $this->assertFalse(\Drupal::hasContainer());
+    // If the following call loaded the user entity it would call
+    // AccountProxy::loadUserEntity() which would fail because the container
+    // does not exist.
+    $this->assertSame(1, $account_proxy->id());

Got it. Couldn't figure out how this worked but this explains it and why id() is special.

+++ b/core/tests/Drupal/Tests/Core/Session/AccountProxyTest.php
@@ -0,0 +1,53 @@
+namespace Drupal\Core\Session;
+
+if (!function_exists('drupal_get_user_timezone')) {
+  function drupal_get_user_timezone() {
+    return date_default_timezone_get();
+  }
+}

gugh, I hate when we have to do this...

+++ b/core/tests/Drupal/Tests/Core/Session/AccountProxyTest.php
@@ -0,0 +1,53 @@
+namespace Drupal\Core\Session ;

There is a space before the semicolon.

For the rest, it seems fine.

@kiamlaluno I think you reviewed #11not #12 where he fixed that. :)

Yes, it seems so.

Nice find.

+++ b/core/lib/Drupal/Core/Session/AccountProxy.php
@@ -23,9 +23,19 @@ class AccountProxy implements AccountProxyInterface {
+   *
+   * @deprecated in Drupal 8.1.9 and to be removed in Drupal 9.
+   *   Use $this->id instead.
    */

This makes a lot of assumptions about what 8.1.9 will be - if it's a security release it won't include this patch for example.

Elsewhere we've been using "Scheduled for removal in 9.0.0, use foo instead" which is a bit simpler.
https://api.drupal.org/api/drupal/core%21tests%21Drupal%21FunctionalTest...

Also protected properties are @internal, so I think we could remove this in 8.3.x not?

Since it is very easy to preserve backwards compatibility for the protected property in this case I think it is fine to keep it even if we consider protected properties @internal.

Fixed the @deprecated message.

Looks good to me.

Let's change the deprecation message to removal in 8.4.x - it's nice to allow code to work with two branches at a time, but we shouldn't be maintaining unnecessary bc layers for the entirety of 8.x's lifetime.

Sure.

Committed/pushed to 8.3.x, thanks!

The promise of the deprecation message committed here did not work out. We should not be removing stuff from Drupal 8, so opened #3033317: AccountProxy initialAccountId and ViewKernelTestBase will only be removed in Drupal 9.0.0, fix deprecation messages to fix the message.