Load user entity in Cookie AuthenticationProvider instead of using manual queries

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.

The last submitted patch, 143: 2345611-143.patch, failed testing. View results →

Re-roll #137 for D10.0.x

Needs a reroll for 11.x-dev. 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.

prem suthar → made their first commit to this issue’s fork.

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.

The performance aspect of this is a tradeoff, that's also why this issue has been stuck so long.

Yes, we save two queries, but the other side of the coin is that to access the information we need from (status, roles) requires that we load the field definitions and create the field objects for it, which is much slower than the UserSession object.

(FWIW, core has been doing that anyway for years due to 🐛 Many calls to ContextRepository::getAvailableContexts() due to entity upcasting Needs review but that's fixed now).

Especially when profiling, that cost is clearly visible and it's hard to compare that with the cost of database queries. On high load sites, possibly with multiple servers and redis/memcache, that cost is easier to balance against extra database queries, but that's not the default setup.

I tried to address that with the getFieldValue() method. But that's complex and I'm not convinced it's entirely reliable and shouldn't be mixed up with this issue iMHO. So either we accept that downside of the change or we postpone it on an issue to improve that performance cost. Which might or might not ever happen.

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)

Well yes, because there's a complex workaround in place to counter the performance problem. But even without that, it is very unlikely to have an impact on CI execution time. But it's been tested and shown a long time ago in comments around #60. I doubt things are fundamentally different now.