Load user entity in Cookie AuthenticationProvider instead of using manual queries

Does entity api requires user to be usable?
Seems only access checking "prepareUser()" cares about user in entity manager
Anyway EM dependency should be declared

+++ b/core/lib/Drupal/Core/Session/SessionHandler.php
@@ -112,22 +113,25 @@ public function read($sid) {
+      $user = \Drupal::entityManager()->getStorage('user')->load($values['uid']);

should be injected properly

I mean that chicken-egg about session and session account that uses entity api.
and yes, currentUser() is fragile at least with global $user

+++ b/core/modules/views/src/Plugin/views/display/Page.php
@@ -336,30 +333,6 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
-        // Only display the menu selector if Menu UI module is enabled.

Is this hunk intentional?

Overall this looks good. The entity system dependency in sessions isn't great, but it's just making an existing dependency explicit.

I'm very in favor of this. In the long run we should load and set the current user in the AuthenticationManager and that will move the entity manager dependency out of the session handler again.

Still it looks like this reverses the decoupling which was introduced in #1825332: Introduce an AccountInterface to represent the current user.

Filed #2347799: Remove bugged session-related methods from AccountInterface

What about introducing a core AccountProviderInterface and implement it in the user module?

Firing an event or something after having loaded the session and then user can chime in and provide the user.

I think this is overkill (and probably out of scope here). I do not think that we should support more than one user-provider (at least not in core) and therefore firing an event is not appropriate here.

Instead the responsibility of identifying the user-id (or perhaps even the user account) associated with a given session should be moved to the cookie authentication provider. However this requires us to pass that information (i.e. the uid) form the SessionHandler::read() to the authentication provider somehow. I planned to simply use the symfony session for that (#2229145: Register symfony session components in the DIC and inject the session service into the request object). This would have the effect that the uid would be duplicated though, once it would be recorded in the uid-column and then also in the serialized session record. I do not consider that a problem. The uid-column on the session table simply is an additional index used to kill all sessions of a given user for example.

Given the above, cookie provider would then work like basic auth instead of the other way around.

Also, I've planned to remove the methods here, because they are alraedy non-functional now and are obviously unused.

Great, in addition you should kill getHostname and move getLastAccessedTime to the User entity. The former is unused and the latter is really only useful to order the list of users in the administrative interface. Also the code in SessionHandler::write() which updates the access-timestamp should be moved to a TERMINATE event handler in the user module.

We could just put the uid column value inside the session data so that it is available to the cookie provider?

That would be wrong, because at this point in time the $_SESSION is not yet present (and also the symfony session bags are not yet associated with it). It will only be populated later from the return value of SessionHandler::read().

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -633,6 +633,9 @@ public function updateModules(array $module_list, array $module_filenames = arra
   +      if ($this->containerNeedsDumping && !$this->dumpDrupalContainer($this->container, static::CONTAINER_BASE_CLASS)) {
   +        $this->container->get('logger.factory')->get('DrupalKernel')->notice('Container cannot be written to disk');
   +      }
   

This can be removed after #2313135: setting page_cache_without_database in settings.php prevents the container from being dumped

2. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -880,6 +880,8 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +    \Drupal::service('router.builder')->setRebuildNeeded();
   +
   

   Not sure about that, but maybe related to #2248297: Ensure routes are rebuilt when install modules or #2230091: Route rebuilding is not guaranteed to finish in time for the next request

3. +++ b/core/lib/Drupal/Core/Session/SessionHandler.php
   @@ -112,35 +112,34 @@ public function read($sid) {
   +    if (!$user && $values) {
   +      // The user is anonymous or blocked. Only preserve the timestamp from the
          // {sessions} table.
          $user = new UserSession(array(
   -        'session' => $values['session'],
   -        'access' => $values['access'],
   +        'access' => $values['timestamp'],
          ));
        }
   

   Removing getLastAccessedTime() from the AccountInterface would make it possible to kill that entirely. In addition getHostname() is unused and can be removed too.

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -632,6 +632,9 @@ public function updateModules(array $module_list, array $module_filenames = arra
   +      if ($this->containerNeedsDumping && !$this->dumpDrupalContainer($this->container, static::CONTAINER_BASE_CLASS)) {
   +        $this->container->get('logger.factory')->get('DrupalKernel')->notice('Container cannot be written to disk');
   +      }
   

   Looks like a leftover?

2. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -892,6 +892,8 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +    \Drupal::service('router.builder')->setRebuildNeeded();
   +
   

   Dito.

#2363099: Using translated field definition descriptions in entity schema results in different schema definitions, resulting in update.php changes is in.

Should be much easier now after #2228393: Decouple session from cookie based user authentication went in.

@Berdir: could you explain the TranslationWrapper problem, it is not reflected in the IS and not very clear from the comments.

I also re-opened #2347799: Remove bugged session-related methods from AccountInterface so we can finish that piece quickly, even though it had been incorporated into this patch.

Let's see whether I'm getting that right: The AuthenticationSubscriber needs to run before LanguageRequestSubscriber for some reason. At the time the authenticate() method is executed, the language subsystem is not yet completely initialized.

In the BasicAuth provider we currently load the whole user entity. But in the Cookie provider we execute the queries manually. The question now is whether the former is broken in multilingual sites or whether it would be possible to convert the latter to entities now that we do not authenticated at random points in the request response cycle?

Drupal\user\Tests\UserCancelTest seems to be missing.

Great! Only user entity changes needs clarification - why status key value used to return field value?

result of \Drupal\Core\Entity\ContentEntityBase::getEntityKey() described as

The value of the entity key, NULL if not defined.

1. +++ b/core/modules/user/src/Authentication/Provider/Cookie.php
   @@ -73,26 +73,14 @@ public function authenticate(Request $request) {
   +        if ($user->isActive()) {
   +          return $user;
   +        }
   +        else {
   +          $session->remove('uid');
   +        }
   

   else superflowous

2. +++ b/core/modules/user/src/Entity/User.php
   @@ -48,7 +48,8 @@
   + *     "status" = "status"
   
   @@ -312,14 +326,14 @@ public function setLastLoginTime($timestamp) {
   -    return $this->get('status')->value == 1;
   +    return $this->getEntityKey('status') == 1;
   ...
   -    return $this->get('status')->value == 0;
   +    return $this->getEntityKey('status') == 0;
   

   this looks strange, getEntityKey() should return a value of key (string "status")

I mean that

I'm trying to come up with a sensible performance test. Trying the following alternative front controller test.php:

use Drupal\Core\DrupalKernel;
use Symfony\Component\HttpFoundation\Request;

$autoloader = require_once 'autoload.php';
$request = Request::createFromGlobals();
$kernel = DrupalKernel::createFromRequest($request, $autoloader, 'prod');
$kernel->prepareLegacyRequest($request);
var_dump($kernel->getContainer()->get('authentication.cookie')->authenticate($request));

ab -c1 -CSESSxxxxx=yyyyyy -n 1000 -k http://localhost:3000/test.php > /tmp/ab-head-test.txt

Requests per second:    101.32 [#/sec] (mean)
Time per request:       9.870 [ms] (mean)
Time per request:       9.870 [ms] (mean, across all concurrent requests)
Transfer rate:          337.21 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:     8   10   2.3      9      29
Waiting:        8   10   2.3      9      29
Total:          8   10   2.3      9      29

ab -c1 -CSESSxxxxx=yyyyyy -n 1000 -k http://localhost:3000/test.php > /tmp/ab-patch-test.txt

Requests per second:    71.43 [#/sec] (mean)
Time per request:       14.000 [ms] (mean)
Time per request:       14.000 [ms] (mean, across all concurrent requests)
Transfer rate:          518.36 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    12   14   3.2     13      39
Waiting:       11   14   3.2     13      39
Total:         12   14   3.2     13      39

I do not think that test is fair, in head the whole entity system is likely not instantiated.

Another test with the path admin/get-node-one wired up to the following controller (see attached module):

class HiController {
  public function getNodeOne() {
    return new Response(\Drupal::entityManager()
      ->getStorage('node')->load(1)->getTitle());
  }
}

HEAD:

Requests per second:    38.26 [#/sec] (mean)
Time per request:       26.139 [ms] (mean)
Time per request:       26.139 [ms] (mean, across all concurrent requests)
Transfer rate:          17.48 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    21   26   6.4     23      64
Waiting:       21   26   6.4     23      64
Total:         21   26   6.4     23      64

patch:

Requests per second:    36.31 [#/sec] (mean)
Time per request:       27.542 [ms] (mean)
Time per request:       27.542 [ms] (mean, across all concurrent requests)
Transfer rate:          16.59 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    23   27   6.0     25      64
Waiting:       23   27   6.0     25      64
Total:         23   27   6.0     25      64

Hacked my way around authentication in order to get xhprof-kit working for authenticated users (see attached diff). There is something weird going on: Only 50% of the whooping 4ms increase is contributed by Cookie::authenticate(), the rest comes from AccountProxy::setAccount(). The heavy path there is AccountProxy::setAccount() - drupal_get_user_timezone() - AccountProxy::getTimeZone() - User::getTimeZone() - ContentEntityBase::get() - ContentEntityBase::getTranslatedField() - ...

See also attached logs and screenshots. Not really sure whether there is room for optimization down that rabbit hole.

For performance test there's two things to look at:

1. Where the current user is not loaded at all on the request with HEAD.

2. Where the current user is loaded in the current request already.

A controller that returns a minimal response, then one that loads a user and does the same would work for testing.

I'd expect #1 to be slower - since we have a user load where there was previously none (although with entity caching this might not be bad either). But on the other hand #2 should be faster, because we only load the user information once rather than doing the direct query also.

Regrettably entity caching does not help here. The profiling revealed that most of the regression is due to accessing data through fields on the user entity (username, last accessed time, timezone). @Berdir correct me if I'm wrong.

Needs a reroll after #2347799: Remove bugged session-related methods from AccountInterface.

I've applied the idea of #47 (sidestep field definitions) to a couple of other methods in the user entity (
getPassword(), getCreatedTime(), getLastAccessedTime(), getLastLoginTime(), getTimeZone(), getUsername()), and profiled again. The regression due to the expensive field definition stuff is gone. I.e. the expensive AccountProxy::setAccount() visible in #63 is gone (see the on-request-authenticate screenshots.

From the xhprof results it may look like EntityManager::getStorage() is still a regression. However, that is not the case at all. The method is used a couple of times throughout the request/response cycle and it just happens to be called in Cookie::authenticate() for the first time. The profiler output for EntityManager::getStorage() shows the same inclusive wall-time even though the method is called 28 times (patch) instead of 27 (head).

There is still a minor regression of about 0.5ms(?) due to EntityStorageBase::load which seems to be a little bit slower than manual queries. In my opinion that is acceptable.

Given those results, is there anything in the entity subsystem which we can leverage to access selected properties directly if necessary? I think there is a category of data where translation / typed data should be optional (e.g. timestamps, timezones, boolean flags, password hash) and direct access should be possible.

Re-roll and removal of protected getUserFromSession to inline code

I think there is a category of data where translation / typed data should be optional (e.g. timestamps, timezones, boolean flags, password hash) and direct access should be possible.

+1

1. +++ b/core/modules/user/src/Authentication/Provider/Cookie.php
   @@ -58,41 +58,14 @@ public function applies(Request $request) {
   +    if ($uid = $request->getSession()->get('uid')) {
   +      /** @var \Drupal\user\UserInterface $user */
   +      if ($user = $this->entityManager->getStorage('user')->load($uid)) {
   

   It is though a little bit sad that, as it is in HEAD, we basically have a dependency to user module here, but yeah this is not a new thing.

2. +++ b/core/modules/user/src/Entity/User.php
   @@ -154,9 +155,22 @@ public function getRoles($exclude_locked_roles = FALSE) {
   +    // Optimize for the case where the field object has not been initialized
   +    // yet, directly access the values.
   

   Should we point to the code using it, I guess its hasPermission?

Just re-roll

#69,2 makes sense, looks we can revert that part because it's no more a case since we use AnonymousUser for that, and we should never get a User object half-initialized

#70 is not a good idea. @Berdir: Could you please explain the reason for #47. I only recall that the hunk in getRoles() is necessary because the field definitions somehow depend on the language subsystem being initialized (and that one depends on authentication already being performed). It would be fantastic if you also could give some direction on what to do about the slowness of getTimeZone() and related methods.

Failed test because UserTest inherited from UserSessionTest
Probably we need decouple them

looks we need to deprecate here UserSession or make core depends on user entity as #69.1 said
and how to update #2477213: Move AccountInterface out of Session namespace now

Unassigning stale issue. Hopefully someone else will pursue this.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -503,6 +503,51 @@ protected function getTranslatedField($name, $langcode) {
      /**
   +   * Gets the value of a specific property of a field.
   +   *
   

   Should we document atht this is optimized for performance reasons?

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -503,6 +503,51 @@ protected function getTranslatedField($name, $langcode) {
   +  public function getFieldValue($field_name, $property) {
   

   Just curious, could you specify a third $delta parameter with it?

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -503,6 +503,51 @@ protected function getTranslatedField($name, $langcode) {
+  public function getFieldValue($field_name, $property) {

I think we should add delta as an optional argument.

I came across this issue while working working on a project. The method used to setup the user session make it harder to work with the current user since its not a real user and as said previously you end up loading the user anyway. If this could get merged into core, I am sure it would make many people pleased.

There are a number of modules that rely on \Drupal::currentUser() to get access information but without having a real user this can have unexpected functionality as it is implemented now.

I have rewritten the patch from #88 against the latest version of 8.3. I also added delta as an optional argument as suggest in #90.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -541,6 +541,53 @@ protected function getTranslatedField($name, $langcode) {
   +        if (isset($field_values[$delta][$property]) && is_array($field_values[$delta])) {
   

   Shouldn't this be other way around first check the is_array and then isset?

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -541,6 +541,53 @@ protected function getTranslatedField($name, $langcode) {
   +    return $this->get($field_name)->$property;
   

   We should use delta here.

I added the delta from #96's 2.

Uploaded the wrong version of the patch the first time.

Related to #63 profiling

I've been running this patch in production for the past six months without problem, and seeing that all raised issues have been resolved I think this is ready to be RTBC'ed.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -541,6 +541,58 @@ protected function getTranslatedField($name, $langcode) {
   +  public function getFieldValue($field_name, $property, $delta = 0) {
   

   The method should be defined in the interface FieldableEntityInterface.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -541,6 +541,58 @@ protected function getTranslatedField($name, $langcode) {
   +      if (isset($this->values[$field_name][$this->activeLangcode])) {
   ...
   +      elseif ($this->values[$field_name][LanguageInterface::LANGCODE_DEFAULT]) {
   

   I think, that the elseif needs the same isset check like the if.

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -541,6 +541,58 @@ protected function getTranslatedField($name, $langcode) {
   +        if (isset($field_values[$delta][$property]) && is_array($field_values[$delta])) {
   

   What is the point of the second condition? If the first one is met, then the second one will always be true.

4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -541,6 +541,58 @@ protected function getTranslatedField($name, $langcode) {
   +        elseif (isset($field_values[$property]) && is_array($field_values)) {
   

   Same question as the previous one.

Thanks for the review. 2, 3, and 4 all sound like good suggestions. They have been applied in the attached patch.

For 1 however, I think there is a concern here with moving the function definition. Ultimately, moving it will create a potential backwards incompatibility. If there is a EntityType which implements the FieldableEntityInterface without extending the ContentEntityBase then they won't receive the new function, and code will break. While I hope this doesn't occur, its still possible and as a result has a negative effect since it may prevent sites (and modules) from upgrading to the latest version.

Has this been done with interfaces previously without causing havoc? If not, its probably best to make a new interface to hold this definition. Thoughts?

Reverted patch from #112 up to 8.9

Updated the patch so it applies in 9.2 and up again.

Last patch is broken for current core

Yeah I was a little quick to upload the patch before verifying it on 9.3.
Updated it again and now verified it applies on 9.3 and 9.4 as well.

Edit: I'm well out of my depth here. Can see all tests failing already. Apologies :(.

Fixed the service definition and class constructor to also include the messenger service.

Okay I think I'm getting there. Actually ran the commit-code-check script this time and turned out fine.

Added reroll of patch #121 on Drupal 9.5.x.

Patch #124 didn't pass the test.

Restored the changes of patch #117 as per comment #127.

Addressed typo of patch #128.

Sorry didn't notice, patch #130.

Rerolled the patch #137 in Drupal 10.1.x.

Rerolled patch #138, please review it.

Fixing custom commands failure issues in patch #140, please review it.

Re-roll #137 for D10.0.x

Needs a reroll for 11.x. I'll stick around for reviews here. Find me in the sky lounge at DDD vienna if there are any questions.

Also desperately needs an issue summary update.

Issue summary: done.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -618,6 +618,58 @@ protected function getTranslatedField($name, $langcode) {
   +   * Only the first delta can be accessed with this method.
   

   This is not true. There is an optional $delta argument.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -618,6 +618,58 @@ protected function getTranslatedField($name, $langcode) {
   +  public function getFieldValue($field_name, $property, $delta = 0) {
   

   Let's add type hints here.

Rerolled for 11.x as mentioned in #147.
Tried to address pointer 2 in #151.

This is not true. There is an optional $delta argument.

In #151.1, True $delta argument is optional, As per my understanding, Either we should update the comment like

By default the first delta can be accessed with this method.

or we can remove this line from the comment.

These are two of very few database queries executed on an authenticated page request when dynamic page cache is a cache hit, if we load the user it will usually come from the cache.
See for example https://grafana.prod.cluster.tag1.io/explore?panes=%7B%22taO%22:%7B%22datasource%22:%22tempo%22,%22queries%22:%5B%7B%22query%22:%2269f4dbd9a3acdf0e49e11acc90cbae81%22,%22queryType%22:%22traceql%22,%22refId%22:%22A%22,%22datasource%22:%7B%22type%22:%22tempo%22,%22uid%22:%22tempo%22%7D,%22limit%22:20%7D%5D,%22range%22:%7B%22from%22:%22now-6h%22,%22to%22:%22now%22%7D%7D%7D&schemaVersion=1&orgId=1 this trace from the Umami authenticated performance test. Would only be a small optimization from that perspective but worth doing I think, so tagging with performance for that.

This needs a reroll from #152 , and it should be converted to a merge request.

As per the #155 re-rolled the patch #152 and created the Mr . also Addressed the #151 Points
Please review.

Thanks @prem suthar, the MR looks right. Regrettably, there are two test failures in @core/modules/user/tests/src/Unit/Plugin/Core/Entity/UserTest.php@. I can reproduce them locally. The full output:

% ../vendor/bin/phpunit modules/user/tests/src/Unit/Plugin/Core/Entity/UserTest.php
PHPUnit 10.5.38 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.14
Configuration: /home/lo/src/drupal/core/phpunit.xml

.EE                                                                 3 / 3 (100%)

Time: 00:00.199, Memory: 16.00 MB

There were 2 errors:

1) Drupal\Tests\user\Unit\Plugin\Core\Entity\UserTest::testHasRole
InvalidArgumentException: The entity object refers to a removed translation (x-default) and cannot be manipulated.

/home/lo/src/drupal/core/lib/Drupal/Core/Entity/ContentEntityBase.php:609
/home/lo/src/drupal/core/lib/Drupal/Core/Entity/ContentEntityBase.php:597
/home/lo/src/drupal/core/modules/user/src/Entity/User.php:193
/home/lo/src/drupal/core/modules/user/src/Entity/User.php:206
/home/lo/src/drupal/core/tests/Drupal/Tests/Core/Session/UserSessionTest.php:77
/home/lo/src/drupal/vendor/bin/phpunit:122

2) Drupal\Tests\user\Unit\Plugin\Core\Entity\UserTest::testUserGetRoles
InvalidArgumentException: The entity object refers to a removed translation (x-default) and cannot be manipulated.

/home/lo/src/drupal/core/lib/Drupal/Core/Entity/ContentEntityBase.php:609
/home/lo/src/drupal/core/lib/Drupal/Core/Entity/ContentEntityBase.php:597
/home/lo/src/drupal/core/modules/user/src/Entity/User.php:193
/home/lo/src/drupal/core/modules/user/tests/src/Unit/Plugin/Core/Entity/UserTest.php:56
/home/lo/src/drupal/vendor/bin/phpunit:122

--

2 tests triggered 2 PHP warnings:

1) /home/lo/src/drupal/core/lib/Drupal/Core/Entity/ContentEntityBase.php:608
Undefined array key "x-default"

Triggered by:

* Drupal\Tests\user\Unit\Plugin\Core\Entity\UserTest::testHasRole
  /home/lo/src/drupal/core/tests/Drupal/Tests/Core/Session/UserSessionTest.php:71

* Drupal\Tests\user\Unit\Plugin\Core\Entity\UserTest::testUserGetRoles
  /home/lo/src/drupal/core/modules/user/tests/src/Unit/Plugin/Core/Entity/UserTest.php:53

2) /home/lo/src/drupal/core/lib/Drupal/Core/Entity/ContentEntityBase.php:608
Trying to access array offset on null

Triggered by:

* Drupal\Tests\user\Unit\Plugin\Core\Entity\UserTest::testHasRole
  /home/lo/src/drupal/core/tests/Drupal/Tests/Core/Session/UserSessionTest.php:71

* Drupal\Tests\user\Unit\Plugin\Core\Entity\UserTest::testUserGetRoles
  /home/lo/src/drupal/core/modules/user/tests/src/Unit/Plugin/Core/Entity/UserTest.php:53

ERRORS!
Tests: 3, Assertions: 2, Errors: 2, Warnings: 2.

Not sure, maybe it is necessary to modify User::getRoles() and use $this->getFieldValue('roles') instead of $this->get('roles'). Possibly @berdir could help here.

Looking at CI jobs I see no viable perf regression, so except fixing the last test it looks ready to go (good to get it in in early days on 11.2)

I was able to get the test past the initial failure, however, since we now use an entity and not a session object, there are other dependencies, including the field definitions, which aren't loaded in the unit test scope. We could mock the full User Entity, but this seems overly complicated and costly. Alternatively, we could move the test to a kernel or functional test so we get access to the container and entity schema. Not sure what the core best practice recommendations are here though. I would think we would copy the required test scenarios to the new test and move it to the Kernal Space, if that's enough to bootstrap what we need.

Just ran into a memory leak with ContentEntityBase::get() so I've opened a new issue to add ::getFieldValue() independently of here. If we can decide that's actually worth adding with the now two use cases identified for it, it would then unblock this issue.

After look at that issue, I think it's theoretically possible to fix the memory leak in a different way, but that still leaves the use case here, so I've opened #3574012: Add a getFieldValue() method to bypass typed data overhead for specific use cases to add the method from here. Leaving the PP-1 on, just points somewhere else now.

@richgerdes moving the test to a kernel test seems reasonable.

Left a note on the MR :)

$this->entityType is really awkward to handle if we'd try to do BC for this. because it's an existing property from the parent that we're updating, so we can't really use __get() or so on this.

We might be able to handle this with property hooks, maybe.