Convert user SQL queries to the Entity Query API

Hi,
this is my initial work.

Rerolled

1. +++ b/core/modules/user/lib/Drupal/user/UserAutocomplete.php
   @@ -65,9 +65,15 @@ public function getMatches($string, $include_anonymous = FALSE) {
   +      $uids = \Drupal::entityQuery('user')
   +        ->condition('name', db_like($string) . '%', 'LIKE')
   +        ->range(0, 10)
   

   Should we use STARTS_WITH here?

2. +++ b/core/modules/user/lib/Drupal/user/UserAutocomplete.php
   @@ -65,9 +65,15 @@ public function getMatches($string, $include_anonymous = FALSE) {
   +        $matches[$account->getUsername()] = check_plain($account->getUsername());
   

   Go ahead and convert this to String::checkPlain().

3. +++ b/core/modules/user/user.module
   @@ -522,16 +521,14 @@ function user_search_access() {
   +    $group = $query->orConditionGroup()
   +      ->condition('name', '%' . db_like($keys) . '%', 'LIKE')
   +      ->condition('mail', '%' . db_like($keys) . '%', 'LIKE');
   +    $query->condition($group);
   

   Use CONTAINS?

4. +++ b/core/modules/user/user.module
   @@ -1137,10 +1133,7 @@ function user_login_finalize(UserInterface $account) {
   -    ->execute();
   +  $controller = Drupal::entityManager()->getStorageController('user')->save($user);
   

   Why not $user->save()?

Ok, I modified as suggested by @twistor, updated some parts and continued with the work.

I'm successfully applied, and tested this patch.
I'm also not found any coding standard problems, so I change the status to RTBC.

+++ b/core/modules/user/user.module
@@ -1062,10 +1061,7 @@ function user_login_finalize(UserInterface $account) {
-  db_update('users')
-    ->fields(array('login' => $user->getLastLoginTime()))
-    ->condition('uid', $user->id())
-    ->execute();
+  $user->save();

I'm not sure about this change. This very explicitly did not use user save before, doing that is quite an overhead over the old code.

The main issue is about to use Entity API everywhere instead of SQL queries #2068325: [META] Convert entity SQL queries to the Entity Query API, and this means that we should user_save to change this data.
But I'm also thinking this is an overhead, have you any idea how can we resolve this without using db_update or user_save?

Yes, there is an alternative, that would be to add a updateTimestamp($uid) to the user storage controller. But I'm not sure if it's worth it.

I think it worths it.

#11: 2068329_11.patch queued for re-testing.

In the last patch the last login time wasn't set, here is the fix.

I'm also confused of the variable name usage in this function:

Some cases it uses $user, other cases $account, they are the same of course, but is here any reason to using one or other in a context?
I think no, so if you agree, I'll change all usage of $account to $user from the 3. line of the function.

function user_login_finalize(UserInterface $account) {
  global $user;
  $user = $account;
  watchdog('user', 'Session opened for %name.', array('%name' => $user->getUsername()));
  // Update the user table timestamp noting user has logged in.
  // This is also used to invalidate one-time login links.
  $account->setLastLoginTime(REQUEST_TIME);
  \Drupal::entityManager()
    ->getStorageController('user')
    ->updateLastLoginTimestamp($account);

  // Regenerate the session ID to prevent against session fixation attacks.
  // This is called before hook_user in case one of those functions fails
  // or incorrectly does a redirect which would leave the old session in place.
  drupal_session_regenerate();

  \Drupal::moduleHandler()->invokeAll('user_login', array($user));
}

#15: 2068329-convert_user_sql_queries_to_the_entity_query_api-15.patch queued for re-testing.

> Some cases it uses $user, other cases $account, they are the same of course, but is here any reason to using one or other in a context?

I would rather change everything to account; using $user with the global $user being around is bad and dangerous practice. But if that's too much; just go with $user -- the global will be gone soon after all.

Ok I think it remains to change the anonimous and admin user creation on installation; but I do not know if at that stage it's possible to make an entity save.

@chx:

Do we also need to modify queries in the update functions and tests?

> change the anonimous and admin user creation on installation; but I do not know if at that stage it's possible to make an entity save.

I doubt you can; it worths trying; but I think any non-sql database will need to alter the install tasks anyways...

Regarding renames: just rename my $account to $user let's no go off on a tangent and do massive renames.

Regarding updates: absolutely not you can't use the entity system during updates.

I'm agree that use $account is better than the global $user, so here is the new patch, only the variable names was changed in the user_login_finalize() function, and rerolled against the new 8.x HEAD.

I believe that as expected you can not create the entity on installation. I only made ​​one final change to the api file; I think that there is nothing else to do.

+++ b/core/modules/user/lib/Drupal/user/AccountFormController.php
@@ -258,13 +258,12 @@ public function validate(array $form, array &$form_state) {
+        $name_taken = (bool) \Drupal::entityQuery('user')

@@ -275,13 +274,12 @@ public function validate(array $form, array &$form_state) {
+      $mail_taken = (bool) \Drupal::entityQuery('user')

+++ b/core/modules/user/lib/Drupal/user/Plugin/Block/UserNewBlock.php
@@ -65,7 +65,13 @@ public function blockSubmit($form, &$form_state) {
+    $uids = \Drupal::entityQuery('user')

+++ b/core/modules/user/lib/Drupal/user/Plugin/Block/UserOnlineBlock.php
@@ -82,7 +82,10 @@ public function build() {
-    $authenticated_count = db_query("SELECT COUNT(uid) FROM {users} WHERE access >= :timestamp", array(':timestamp' => $interval))->fetchField();
+    $authenticated_count = \Drupal::entityQuery('user')

@@ -92,7 +95,13 @@ public function build() {
+      $uids = \Drupal::entityQuery('user')

+++ b/core/modules/user/lib/Drupal/user/UserAutocomplete.php
@@ -65,9 +66,15 @@ public function getMatches($string, $include_anonymous = FALSE) {
+      $uids = \Drupal::entityQuery('user')
...
+      $controller = \Drupal::entityManager()->getStorageController('user');
+      foreach ($controller->loadMultiple($uids) as $account) {

I guess we need to inject all those entity query instances?

Ok modified as suggested, I hope the right way.

Rerolled with some adjustment.

I hope this is the last time, rerolled.

Rerolled.

Ok modified as required.

I realized that returning a boolean value is cleaner for the purpose of the function.

Sorry I forgot the cast.

36: 2068329-convert_user_sql_queries_to_the_entity_query_api-36.patch queued for re-testing.

Just a simple re-roll

Should fix the issues and also kills what seems to be unused use statements

Rerolled.

48: 2068329-convert_user_sql_queries_to_the_entity_query_api-48.patch queued for re-testing.

48: 2068329-convert_user_sql_queries_to_the_entity_query_api-48.patch queued for re-testing.

reroll

Reroll. Attached diff between patches #48 and this one.

Reroll. Patch no longer applied following #2145007: Convert form_set_error() in FormBase classes to use FormErrorInterface

Another reroll following #2146517: Remove annotation 'use' statements from all core classes

62: 2068329-convert_user_sql_queries_to_the_entity_query_api-62.patch queued for re-testing.

Note that #2164715: Remove user_node_load() will remove the user_node_load() completely which I think is preferred over this, so will need a re-roll if it gets in first and if something else conflicts first, I'd suggest to re-roll without that change.

+++ b/core/modules/user/lib/Drupal/user/AccountFormController.php
@@ -301,13 +313,12 @@ public function validate(array $form, array &$form_state) {
+          ->condition('name', $form_state['values']['name'])

Doesn't this mean the query will no-longer be case insensitive?

That patch isn't in yet though right?

Hmm. Either postpone or make that one critical?

Bumped the other issue to critical, I haven't reviewed the full patch here yet so just leaving RTBC.

73: 2068329-convert_user_sql_queries_to_the_entity_query_api-73.patch queued for re-testing.

This looks great!

Committed 0f704ad and pushed to 8.x. Thanks!

diff --git a/core/modules/user/lib/Drupal/user/UserStorageController.php b/core/modules/user/lib/Drupal/user/UserStorageController.php
index bd86370..6093e7e 100644
--- a/core/modules/user/lib/Drupal/user/UserStorageController.php
+++ b/core/modules/user/lib/Drupal/user/UserStorageController.php
@@ -112,12 +112,12 @@ public function save(EntityInterface $entity) {
   /**
    * {@inheritdoc}
    */
-  public function saveRoles(UserInterface $user) {
+  public function saveRoles(UserInterface $account) {
     $query = $this->database->insert('users_roles')->fields(array('uid', 'rid'));
-    foreach ($user->getRoles() as $rid) {
+    foreach ($account->getRoles() as $rid) {
       if (!in_array($rid, array(DRUPAL_ANONYMOUS_RID, DRUPAL_AUTHENTICATED_RID))) {
         $query->values(array(
-          'uid' => $user->id(),
+          'uid' => $account->id(),
           'rid' => $rid,
         ));
       }
diff --git a/core/modules/user/lib/Drupal/user/UserStorageControllerInterface.php b/core/modules/user/lib/Drupal/user/UserStorageControllerInterface.php
index ec4348d..5718f93 100644
--- a/core/modules/user/lib/Drupal/user/UserStorageControllerInterface.php
+++ b/core/modules/user/lib/Drupal/user/UserStorageControllerInterface.php
@@ -27,7 +27,7 @@ public function addRoles(array $users);
    *
    * @param \Drupal\user\UserInterface $account
    */
-  public function saveRoles(UserInterface $user);
+  public function saveRoles(UserInterface $account);

   /**
    * Remove the roles of a user.

Fixed during commit - there was an inconsistency between the parameter names in the interface and the class.

Hm. Do we actually need a change record here? It looks to me like we are just injecting a couple more dependencies in some constructors, so the only BC breaks would be for someone subclassing these things or using them directly in D8. Maybe we just need to see if there are any references to these classes in existing change records, or verify that their aren't? Or did I miss something?

Alright, thanks @plach!

Tagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release