Use entity API in session handler.

=> No hardcoded queries on users and user roles tables
=> Should actually be faster because entity storage cache.
=> It is quite common to actually have to load the user entity anyway, this could simplify some code (keep AccountInterface, but if authenticated user, then you can assume that you have a user entity).

Files: 

Comments

Berdir’s picture

Status: Active » Needs review
FileSize
3.84 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,826 pass(es), 530 fail(s), and 221 exception(s). View

First patch.

Status: Needs review » Needs work

The last submitted patch, 1: user-session-load-2345611-1.patch, failed testing.

andypost’s picture

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

Berdir’s picture

Status: Needs work » Needs review
FileSize
6.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,128 pass(es), 187 fail(s), and 59 exception(s). View
4.5 KB

Second and better patch.

I have a feeling that all the session related methods on user/account are completely bogus and unused...

@andypost: Don't get the first part, but sure, it should be injected, will care about that when I know that it is actually working..

Status: Needs review » Needs work

The last submitted patch, 4: user-session-load-2345611-3.patch, failed testing.

andypost’s picture

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

Berdir’s picture

Assigned: Unassigned » Berdir
Status: Needs work » Needs review
FileSize
16.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,280 pass(es), 54 fail(s), and 39 exception(s). View
14.83 KB

See #2248297: Ensure routes are rebuilt when install modules.

That and lots of translation issues.

Status: Needs review » Needs work

The last submitted patch, 7: user-session-load-2345611-7.patch, failed testing.

The last submitted patch, 7: user-session-load-2345611-7.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
20.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,538 pass(es), 10 fail(s), and 17 exception(s). View
3.67 KB

Support TranslationWrapper in config casting (this is a bit weird, as we put translated values there, but we did that before to.

Also merge in the fixes from #2248297: Ensure routes are rebuilt when install modules

Status: Needs review » Needs work

The last submitted patch, 11: user-session-load-2345611-11.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,737 pass(es). View
2.44 KB

Fixing those last tests. TranslationWrapper *is* tricky...

catch’s picture

+++ 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.

znerol’s picture

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.

znerol’s picture

znerol’s picture

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

Berdir’s picture

I had a similar plan yes, firing an event or something after having loaded the session and then user can chime in and provide the user.

I have no idea yet how that would work with basic auth yet, though.

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

znerol’s picture

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.

Berdir’s picture

Sure, that works too for me.

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

I will pick this up when #2313135: setting page_cache_without_database in settings.php prevents the container from being dumped is in.

znerol’s picture

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().

Berdir’s picture

FileSize
23.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,638 pass(es). View

Ok, that got in, so here is a re-roll + I also included #2347799: Remove bugged session-related methods from AccountInterface.

znerol’s picture

Status: Needs review » Needs work
  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');
    +      }
    
  2. This can be removed after #2313135: setting page_cache_without_database in settings.php prevents the container from being dumped

  3. +++ 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

  4. +++ 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.

Berdir’s picture

1. 2. Yes, those changes were added here because I needed this. This issue is the reason I started working on all those routing issues :)

Berdir’s picture

Status: Needs work » Needs review
FileSize
29.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,169 pass(es), 699 fail(s), and 128 exception(s). View
7.48 KB

Rerolled and removed those methods.

Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, 25: user-session-load-2345611-25.patch, failed testing.

znerol’s picture

  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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
29.19 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch user-session-load-2345611-29.patch. Unable to apply patch. See the log in the details link for more information. View
2.13 KB

Yes. Removed that. Also the module handler change seems to fix InstallUninstallTest, some weird caching thing going on. Again.

znerol’s picture

Issue tags: +Needs profiling

Status: Needs review » Needs work

The last submitted patch, 29: user-session-load-2345611-29.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
27.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,048 pass(es), 12 fail(s), and 2 exception(s). View

Re-roll.

Status: Needs review » Needs work

The last submitted patch, 34: user-session-load-2345611-34.patch, failed testing.

Berdir’s picture

Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition(content_translation.manager)
Symfony\Component\DependencyInjection\ContainerBuilder->get(content_translation.manager, 1)
Drupal\Core\DependencyInjection\ContainerBuilder->get(content_translation.manager)
Drupal::service(content_translation.manager)
content_translation_enabled(user)
content_translation_entity_storage_load([Array], user)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->getFromStorage([Array])
Drupal\Core\Entity\Sql\SqlContentEntityStorage->doLoadMultiple([Array])
Drupal\Core\Entity\EntityStorageBase->loadMultiple([Array])
Drupal\Core\Entity\EntityStorageBase->load(2)
Drupal\Core\Session\SessionHandler->read(lXSu4BT_FwfHwWS_8j2itgNA8Mgrt9AlxzKgK1fZzgw)
Symfony\Component\HttpFoundation\Session\Storage\Handler\WriteCheckSessionHandler->read(lXSu4BT_FwfHwWS_8j2itgNA8Mgrt9AlxzKgK1fZzgw)
Symfony\Component\HttpFoundation\Session\Storage\Proxy\SessionHandlerProxy->read(lXSu4BT_FwfHwWS_8j2itgNA8Mgrt9AlxzKgK1fZzgw)
session_start()
Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage->start()
Drupal\Core\Session\SessionManager->startNow()
Drupal\Core\Session\SessionManager->start()
Drupal\Core\DrupalKernel->initializeContainer(TRUE)
Drupal\Core\DrupalKernel->updateModules([Array], [Array])
Drupal\Core\Extension\ModuleInstaller->updateKernel([Array])
Drupal\Core\Extension\ModuleInstaller->install([Array])
Drupal\system\Form\ModulesListConfirmForm->submitForm([Array], Drupal\Core\Form\FormState)
call_user_func_array([Array], [Array])
Drupal\Core\Form\FormSubmitter->executeSubmitHandlers([Array], Drupal\Core\Form\FormState)
Drupal\Core\Form\FormSubmitter->doSubmitForm([Array], Drupal\Core\Form\FormState)
Drupal\Core\Form\FormBuilder->processForm(system_modules_confirm_form, [Array], Drupal\Core\Form\FormState)
Drupal\Core\Form\FormBuilder->buildForm(Drupal\system\Form\ModulesListConfirmForm, Drupal\Core\Form\FormState)
Drupal\Core\Controller\FormController->getContentResult(Symfony\Component\HttpFoundation\Request)
call_user_func_array([Array], [Array])
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Symfony\Component\HttpFoundation\Request, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Symfony\Component\HttpFoundation\Request, 1, TRUE)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Symfony\Component\HttpFoundation\Request, 1, TRUE)
Drupal\Core\StackMiddleware\PageCache->handle(Symfony\Component\HttpFoundation\Request, 1, TRUE)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Symfony\Component\HttpFoundation\Request, 1, TRUE)
Stack\StackedHttpKernel->handle(Symfony\Component\HttpFoundation\Request, 1, TRUE)
Drupal\Core\DrupalKernel->handle(Symfony\Component\HttpFoundation\Request)

Such fear. much scared.

Berdir’s picture

Status: Needs work » Needs review
FileSize
28.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,167 pass(es), 6 fail(s), and 3 exception(s). View
746 bytes

Reroll, let's see if this change fixes those tests, content_translation tried accessing new services after being installed but we didn't replace the global container yet.

Status: Needs review » Needs work

The last submitted patch, 37: user-session-load-2345611-37.patch, failed testing.

Berdir’s picture

Correct, but that wasn't fixed in a way that helps us here. That part was postponed to #2418521: Translating field definition descriptions problematic due to early t(), hard to test.

The problem with moving forward without that is that adding a field to users that does not use TranslationWrapper() could suddenly completely break your site. I'm not sure that's a risk we can take.

Berdir’s picture

Status: Needs work » Needs review
FileSize
27.98 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,155 pass(es), 17 fail(s), and 3 exception(s). View

Reroll.

Status: Needs review » Needs work

The last submitted patch, 41: user-session-load-2345611-41.patch, failed testing.

znerol’s picture

Issue tags: +Needs reroll
Berdir’s picture

Yeah, the TranslationWrapper problem stil exists, though, but if someone wants to re-roll this, now would be the time :)

almaudoh’s picture

@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.

znerol’s picture

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?

Berdir’s picture

Status: Needs work » Needs review
FileSize
4.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

Fresh start, with the discussed getRoles() implementation that avoids fields definitions. Manual testing worked well.

Status: Needs review » Needs work

The last submitted patch, 47: current-user-load-entity-2345611-47.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 47: current-user-load-entity-2345611-47.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
6.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
1.71 KB

Fixed the unit tests, will need more.

Test took forever but I really can't explain why that would be related to the patch. Let's see if it's still that slow.

Status: Needs review » Needs work

The last submitted patch, 51: current-user-load-entity-2345611-57.patch, failed testing.

Berdir’s picture

Again the test got terminated after 3h. I guess one of them loops forever, but how... will need to diff the test list I guess...

znerol’s picture

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

Berdir’s picture

Status: Needs work » Needs review
FileSize
7.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,705 pass(es). View
2.21 KB

Right, that was looping in a weird way because I forgot to check the user status. I added that now and used the entity key to ensure that we can load and check that without having to load the field definitions. And also explicitly unset the uid in the session during the user cancel process.

andypost’s picture

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")

Berdir’s picture

1. the else is to remove the session key in case the user is no longer active, I think that's useful cleanup so that we don't have to try and load a blocked user on every request.

2. It posts the *value* of the key, which is 0/1. getEntityKey() is an optimization for entity keys that allows to access their values without having to load field definitions and field values without having to go through field objects.

andypost’s picture

FileSize
650 bytes
7.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,727 pass(es). View

I mean that

znerol’s picture

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.

znerol’s picture

FileSize
1.25 KB
1.25 KB
znerol’s picture

FileSize
591 bytes
1.26 KB
1.26 KB

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
Berdir’s picture

Yeah...

My results, your test.php (1000 requests) and frontpage with one node (

HEAD test.php:
Requests per second:    179.89 [#/sec] (mean)
Time per request:       5.559 [ms] (mean)
Time per request:       5.559 [ms] (mean, across all concurrent requests)

Patch test.php:
Requests per second:    121.97 [#/sec] (mean)
Time per request:       8.199 [ms] (mean)
Time per request:       8.199 [ms] (mean, across all concurrent requests)

Head front:
Requests per second:    12.76 [#/sec] (mean)
Time per request:       78.370 [ms] (mean)
Time per request:       78.370 [ms] (mean, across all concurrent requests)

Patch: front:
Requests per second:    12.22 [#/sec] (mean)
Time per request:       81.857 [ms] (mean)
Time per request:       81.857 [ms] (mean, across all concurrent requests)

Did some profiling to figure out what makes it slower. Turns out, we do have a few calls to methods that do have to load fields. I noticed getTimezone(), getLastAccessedTime() and getUsername().

znerol’s picture

FileSize
1.78 KB
169.44 KB
162.94 KB
4.39 KB

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.

catch’s picture

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.

znerol’s picture

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.

znerol’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
FileSize
168.5 KB
169.5 KB
178.69 KB

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.

znerol’s picture

andypost’s picture

Status: Needs work » Needs review
Issue tags: -Needs profiling, -Needs reroll
FileSize
7.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,913 pass(es). View

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

dawehner’s picture

  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?

andypost’s picture

Parent issue: » #2371629: [meta] Finalize Session and User Authentication API
FileSize
7.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,399 pass(es), 4 fail(s), and 0 exception(s). View
1.03 KB
7.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,397 pass(es). View

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

The last submitted patch, 70: current-user-load-entity-2345611-70-nopart.patch, failed testing.

znerol’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

#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.

andypost’s picture

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

andypost’s picture

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

mgifford’s picture

Assigned: Berdir » Unassigned

Unassigning stale issue. Hopefully someone else will pursue this.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Berdir’s picture

Still applies with git apply -3.

The reason for #47 is the same as the answer to make getTimezone() faster. Skipping the slow field item list + field item creation.

#2580551: Optimize getCommentedEntity() is an attempt at making that more generic, but I'm not sure if we can or want to support multiple fields.

Status: Needs review » Needs work

The last submitted patch, 77: current-user-load-entity-2345611-77.patch, failed testing.

Berdir’s picture

Lets try this instead.

Berdir’s picture

That doesn't work, here's a combined patch with getCommentedEntity() and using getFieldValue().

The last submitted patch, 79: current-user-load-entity-2345611-79.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 80: current-user-load-entity-2345611-80.patch, failed testing.

Berdir’s picture

Fixes and cleanup.

Tested this, getFieldDefinitions() for user is no longer called with this on normal pages. Only place it is called is in the comment form. And when viewing the user profile, but only the first time. And both cases are already the case right now.

The last submitted patch, 83: current-user-load-entity-2345611-83.patch, failed testing.

dawehner’s picture

  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?

Berdir’s picture

1. Was thinking about that, not sure.
2. I thought about that too, actually had it at one point. it's a bit strange with the non-delta specific values, but I think we can simply only check those for delta 0.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, 88: current-user-load-entity-2345611-88.patch, failed testing.

jibran’s picture

+++ 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.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

richgerdes’s picture

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.

Berdir’s picture

Status: Needs work » Needs review

Thanks for the reroll, don't forget to set issues to needs review when uploading a patch.

Keep in mind that code still shouldn't rely on the current user being a user entity, but loading the entity now is at least a load from static cache.

Status: Needs review » Needs work

The last submitted patch, 92: 2345611-92.diff, failed testing.

Berdir’s picture

Version: 8.3.x-dev » 8.4.x-dev
Status: Needs work » Needs review
FileSize
11.72 KB
2.79 KB

This removes the comment specific parts from this, lets see how many test fails are left.

jibran’s picture

  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.

Berdir’s picture

Status: Needs review » Needs work

1. Nope because the $delta key might not be set. This is just there to prevent matching it as string index as it's possible that levels could be a string, depending on how the entity is initialized.
2. yep.

richgerdes’s picture

FileSize
11.83 KB
685 bytes

Uploaded the wrong version of the patch the first time.

The last submitted patch, 98: 2345611-98.patch, failed testing.