follow-up from #1184944: Make entities classed objects, introduce CRUD support. This issue is for migrating the user entity to the new system.
See #1346204: [meta] Drupal 8 Entity API improvements for the general roadmap.

CommentFileSizeAuthor
#122 1361228-interpatch-diff.txt12.09 KBNiklas Fiekas
#118 drupal-1361228-118.patch52.8 KBtim.plunkett
#115 user-entity-1361228-115.patch54.82 KBxjm
#104 1361228-entity-user-104.patch54.84 KBNiklas Fiekas
#104 1361228-entity-user-104-interdiff.txt1.1 KBNiklas Fiekas
#102 1361228-entity-user-102.patch53.91 KBNiklas Fiekas
#102 1361228-entity-user-102-interdiff.txt3.88 KBNiklas Fiekas
#99 d8-entity-user.patch52.87 KBfago
#97 d8-entity-user.patch60.17 KBfago
#93 d8-entity-user.patch64.85 KBfago
#91 1361228.patch55.54 KBRobLoach
#85 user-entity-1361228-85.patch51.29 KBxjm
#85 interdiff.txt11.97 KBxjm
#83 1361228-user-entity-83.patch51.04 KBBerdir
#80 1361228-user-entity-80.patch50.98 KBBerdir
#80 test_fixes_interdiff-do-not-test.patch4.04 KBBerdir
#77 1361228-user-entity-77.patch48.43 KBBerdir
#75 1361228-user-entity-75.patch48.45 KBBerdir
#74 1361228-user-entity-74.patch48.41 KBBerdir
#72 1361228-user-entity-72.patch48.7 KBaspilicious
#70 1361228_70_code_comments.patch47.11 KBcosmicdreams
#60 user-entity-1361228-60.patch48.18 KBduellj
#56 user-entity-1361228-56.patch48.2 KBduellj
#47 remove_debug_code.patch50.81 KBBerdir
#44 fixed_most_testfails.patch51.19 KBBerdir
#35 user-entity-rename-account-to-entity.patch47.89 KBBerdir
#33 user-entity-revert-User.patch47.13 KBBerdir
#30 user-entity-more-tests-fixed.patch58.5 KBBerdir
#28 user-entity-saving-and-hooks.patch56.19 KBBerdir
#22 user-entity.patch12.84 KBmarcingy
#20 user-entity.patch11.69 KBmarcingy
#14 drupal-8-user-entity-classed-object-1361228.patch7.3 KBmarcingy
#11 drupal-8-user-entity-classed-object-1361228-5.patch4.39 KBmarcingy
#9 drupal-8-user-entity-classed-object-1361228-5.patch4.39 KBmarcingy
#8 drupal-8-user-entity-classed-object-1361228-5.patch4.84 KBmarcingy
#6 drupal-8-user-entity-classed-object-1361228-5.patch5.33 KBmarcingy
#4 drupal-8-user-entity-classed-object-1361228-4.patch2.41 KBrlmumford
#1 drupal-8-user-entity-classed-object-1361228-1.patch1.99 KBrlmumford
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rlmumford’s picture

Made a start on this.

Defined all the variables in the Drupal 7 user object as public variables of the User Class.

rlmumford’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-8-user-entity-classed-object-1361228-1.patch, failed testing.

rlmumford’s picture

Status: Needs work » Needs review
FileSize
2.41 KB

Mainly a re-roll, but I've also added the entity class key into the user entity info array.

tstoeckler’s picture

Status: Needs review » Needs work
+++ b/core/modules/user/user.entity.inc
@@ -5,6 +5,125 @@
+   * The timestamp for when the user was create.

create -> created. Also I think "for" can be omitted.

+++ b/core/modules/user/user.entity.inc
@@ -5,6 +5,125 @@
+   * The timestamp for when the user's last login.

Again, I think "for" can be omitted here.

+++ b/core/modules/user/user.entity.inc
@@ -5,6 +5,125 @@
+   * Whether the user is active(1) or blocked(0).

I think the (1) and (0) can be omitted.

+++ b/core/modules/user/user.entity.inc
@@ -5,6 +5,125 @@
+   * The fid of the user's picture.

I don't know if we have any standard for this. On the one hand calling this "fid" makes it clear that

$user->picture === $file->fid;

On the other hand "fid" isn't a real word...

I think this is a really good start (!!!, thx!), but this is definitely not "needs review" yet. user_save() and friends need to be updated for this.

22 days to next Drupal core point release.

marcingy’s picture

Status: Needs work » Needs review
FileSize
5.33 KB

Tidying up comments above and starting to build out the user controller. This will fail but the tests can now create users.

marcingy’s picture

Status: Needs review » Needs work

And back to needs work now this is off to the bot.

marcingy’s picture

marcingy’s picture

marcingy’s picture

Take 3

marcingy’s picture

marcingy’s picture

Status: Needs work » Needs review
marcingy’s picture

Status: Needs review » Needs work
marcingy’s picture

Status: Needs work » Needs review
FileSize
7.3 KB

Some what improved patch

marcingy’s picture

Status: Needs review » Needs work
jbrown’s picture

-    $account = user_save(drupal_anonymous_user(), $edit);
+    $User = entity_create('user', $edit);

variables don't start with a capital letter

fago’s picture

I fear this one is going to be a tough one - who doesn't love user.module?

+++ b/core/modules/block/block.module
@@ -631,9 +631,9 @@ function block_form_user_profile_form_alter(&$form, &$form_state) {
+function block_user_presave($edit) {

This function should receive just $account.

+++ b/core/modules/block/block.module
@@ -631,9 +631,9 @@ function block_form_user_profile_form_alter(&$form, &$form_state) {
+  if (isset($edit->block)) {
+    $edit->data['block'] = $edit->block;

uhm? I guess $edit->block stems from the form, so it should be read from the form values and be written into $account in an #entity_builder. See entity_form_submit_build_entity().

Of course, it should be only written into the right position inside of $account.

+++ b/core/modules/overlay/overlay.module
@@ -102,9 +102,9 @@ function overlay_form_user_profile_form_alter(&$form, &$form_state) {
+  if (isset($edit->overlay)) {
+    $edit->data['overlay'] = $edit->overlay;
   }

Same here.

+++ b/core/modules/user/user.entity.inc
@@ -5,12 +5,131 @@
+ * Defines the user entity class

Missing trailing point.

+++ b/core/modules/user/user.module
@@ -1184,23 +1184,23 @@ function user_account_form_validate($form, &$form_state) {
+function user_user_presave($edit) {

User module specific presave stuff can go into the controllers preSave() method. But again, it shouldn't deal with cleaning up form values. That belongs to the form code.

Gábor Hojtsy’s picture

Unfortunately $language on the user is not really the language of the user like on comment or node entities and this introduces inconsistencies with the OOP solution. For user entities, we don't have a base language for the entity. We do have a "preferred language" that the user set. But its more like the timezone value. It is used elsewhere to work with the entity, not to designate the language of the entity. We should ideally introduce a real language property on the entity with the goal of pairing up the functionality with nodes and comments and rename the current language property to "preferred_language" or somesuch.That would clean up the inconsistency in the API.

marcingy’s picture

@fago both the block and overlay code is from user save not from form code so I assume that the issue is we are passing in $edit and not $account. This is indeed going to be fun. I'll try and attempt a round 2 later.

marcingy’s picture

Status: Needs work » Needs review
FileSize
11.69 KB

Some more work, still need to deal with the form remenants but this new patch now allows for tests to create users etc with success for the most part. The next step is likely to be to start attempting to switch out internal calls to user_save now that the framework in general form seems to be somewhat robust.

marcingy’s picture

Status: Needs review » Needs work

Back to needs work as the bot now has this.

marcingy’s picture

Status: Needs work » Needs review
FileSize
12.84 KB

Reroll using user entity for site installs

marcingy’s picture

Status: Needs review » Needs work
xjm’s picture

Category: task » feature
xjm’s picture

Note that #1346032: Ordering of hook_entity_delete() is inconsistent will need to go in before this issue, and the patch here be rerolled for it, taking the predelete hook into account.

sun’s picture

Issue tags: +Entity system
aspilicious’s picture

I looked at this and this is going to be a biggy.

1) User_save, user_presave, ... all use and $edit array. That is plain wrong with the new CRUD api. Every function should change the $user before calling the save function.

2) In bootstrap.inc we sometimes create an anonymous user, this happens by making a user stdObject. With user being a real class we should call "new User()" instead of "new stdClass". But I'm not sure we can do this at this stage in bootstrap.
This is a caused by the cache function in bootstrap. (I hope someone prooves me wrong so we don't have to change this)

3) I'm always confused by $account vs $user. If we create a class named "User" we shouldn't call it $account in all those functions. That is stupid and confusing. If you want to use "account" you should call the class "Account". People could disagree but seriously it is confusing. EDIT: chx explained the difference to me, user is the current object account any user

Berdir’s picture

Status: Needs work » Needs review
FileSize
56.19 KB

Yes, this is a biggie :)

Attached patch converts all user_save() calls, user hooks, fixes some bugs (e.g. user picture upload). Not finished yet, but should be relatively close..

One issue I wasn't able to fix yet was something related to current_pass/password changes.

And yes, creating/accessing User objects in early bootstrap is a problem. Just like in #1361226: Make the file entity a classed object, this happens when you store User objects with variable_set() for example. This is for example done when saving sent mails including their context during test runs. To make this work, I made the entity_get_info() call conditional for now and moved drupal_map_assoc() to bootstrap.inc.

Status: Needs review » Needs work

The last submitted patch, user-entity-saving-and-hooks.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
58.5 KB

The attached patch now also creates User instances for anymous user objects and session user objects.

Added a check to remove unselected roles.

Status: Needs review » Needs work

The last submitted patch, user-entity-more-tests-fixed.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -Entity system

Discussed this with fago, reverted the User enforcement in the hooks and back to a stdClass for session user objects and drupal_anonymous_user().

Both need more discussion/work...

Berdir’s picture

Forgot the patch...

Status: Needs review » Needs work

The last submitted patch, user-entity-revert-User.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
47.89 KB

Renamed $account to $entity in preSave(), copy & paste errors...

Status: Needs review » Needs work

The last submitted patch, user-entity-rename-account-to-entity.patch, failed testing.

Berdir’s picture

I think most of the remaining fails come down to two issues:

- user:created tokens don't work. That's why all the trigger tests fail and obviously the token replacement tests
- current_password confirmation to change mail/password does not work, the current_password value isn't accepted.

David_Rothstein’s picture

Just skimming through the patch and noticed that sometimes it does things like this:

   $account = user_load(1);
+  $account->save();

But other times does things like this:

+    $account = user_load($user->uid);
+    user_save($account);

Even though they do the same thing, we probably want to standardize on one of those save methods. I'm guessing user_save(), for consistency with user_load()...

Berdir’s picture

I've been wondering that myself. The ->save() calls were from earlier patches then mine and at that point, user_save() hadn't been ported yet.

I'm not sure if the _save() functions are meant to stay or if they're just a BC layer. But if it is, we could just as well drop it right now because the function is changing anyway.

Crell’s picture

My understanding is that we want to move to $entity->save(), which internally just calls $this->controller->save($this). (That part doesn't work yet, IIRC.)

user_load() et al are already just convenience wrappers around $controller->load().

sun’s picture

$entity->save() already works for other entity types. If it doesn't work for user entities, we need to fix that in this patch.

The procedural wrappers user_save() are kept in these patches. Completely removing them and converting all existing code is a follow-up task on its own. Therefore, it also doesn't really matter whether this patch is consistently using one or the other.

Berdir’s picture

user_save() is already being changed in this patch to work like other something_save() functions, meaning user_save($account) and not user_save($account, $array_with_stuff_that_changes).

There is absolutely no way around that, because e.g. the user hooks are now generic and don't use $edit anymore, as they are called by the generic save() implementation.

So, if the goal is to remove user_save(), we can as well do it right now. It's pointless to introduce a BC compatibility layer that is not backwards compatible ;)

Crell’s picture

It's pointless to introduce a BC compatibility layer that is not backwards compatible ;)

Sage advise. :-) I agree, if we have to change user_save() anyway, go ahead and euthanize it.

Berdir’s picture

FileSize
51.19 KB

Ok, this should fix most fails except the hashing thing.

- Removed user_save().

- The openid tests failed because $form['#user'] had uid set to NULL, which caused the query to user name duplicate query to behave unexpectecly. Fixed in a hackish way, not sure what's the best way here.

--- a/core/modules/user/user.module
+++ b/core/modules/user/user.module
@@ -929,7 +929,7 @@ function user_account_form_validate($form, &$form_state) {
     if ($error = user_validate_name($form_state['values']['name'])) {
       form_set_error('name', $error);
     }
-    elseif ((bool) db_select('users')->fields('users', array('uid'))->condition('uid', $account->uid, '<>')->condition('name', db_like($form_state['values']['name']), 'LIKE')->range(0,
+    elseif ((bool) db_select('users')->fields('users', array('uid'))->condition('uid', (int)$account->uid, '<>')->condition('name', db_like($form_state['values']['name']), 'LIKE')->ran
       form_set_error('name', t('The name %name is already taken.', array('%name' => $form_state['values']['name'])));
     }
   }
@@ -943,7 +943,7 @@ function user_account_form_validate($form, &$form_state) {
   if ($error = user_validate_mail($form_state['values']['mail'])) {
     form_set_error('mail', $error);
   }
-  elseif ((bool) db_select('users')->fields('users', array('uid'))->condition('uid', $account->uid, '<>')->condition('mail', db_like($form_state['values']['mail']), 'LIKE')->range(0, 1
+  elseif ((bool) db_select('users')->fields('users', array('uid'))->condition('uid', (int)$account->uid, '<>')->condition('mail', db_like($form_state['values']['mail']), 'LIKE')->range
     // Format error message dependent on whether the user is logged in or not.
     if ($GLOBALS['user']->uid) {
       form_set_error('mail', t('The e-mail address %email is already taken.', array('%email' => $form_state['values']['mail'])));

The created issues were because the default value of created was set to 0. This caused the !isset($entity->created) check to return FALSE so created was saved as 0. Changed default value to NULL.

- The password hashing is a tricky one. One problem is that an empty $entity->pass resulted in a re-hashing of the password. But the tests still fail, so there must be something else. I'm really out of clues at the moment...

--- a/core/modules/user/user.entity.inc
+++ b/core/modules/user/user.entity.inc
@@ -182,7 +182,7 @@ class UserController extends EntityDatabaseStorageController {
 
   protected function preSave(EntityInterface $entity) {
     // Update the user password if it has changed.
-    if ($entity->is_new || $entity->pass != $entity->original->pass) {
+    if ($entity->isNew() || (!empty($entity->pass) && $entity->pass != $entity->original->pass)) {
       // Allow alternate password hashing schemes.
       require_once DRUPAL_ROOT . '/' . variable_get('password_inc', 'core/includes/password.inc');
       $entity->pass = user_hash_password(trim($entity->pass));
Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, fixed_most_testfails.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
50.81 KB

Removed some debug code.

Status: Needs review » Needs work

The last submitted patch, remove_debug_code.patch, failed testing.

sun’s picture

I'm not quite sure whether or how this + other entity conversion patches can be made to pass tests without the essential base system and Entity class fixes in #1361232-29: Make the taxonomy entities classed objects -- based on what I see there, those changes need to get in first.

aspilicious’s picture

Some points for the next reroll

+++ b/core/includes/bootstrap.incundefined
@@ -3408,3 +3408,30 @@ function _drupal_shutdown_function() {
\ No newline at end of file

Newline needed

+++ b/core/modules/user/user.entity.incundefined
@@ -5,12 +5,134 @@
+  public function __construct(array $values = array(), $entity_type = NULL) {

Needs an override docblock

+++ b/core/modules/user/user.entity.incundefined
@@ -49,4 +171,143 @@ class UserController extends DrupalDefaultEntityController {
+  public function save(EntityInterface $entity) {

docblock needed

+++ b/core/modules/user/user.entity.incundefined
@@ -49,4 +171,143 @@ class UserController extends DrupalDefaultEntityController {
+  protected function preSave(EntityInterface $entity) {

same

aspilicious’s picture

Status: Needs work » Needs review

#47: remove_debug_code.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, remove_debug_code.patch, failed testing.

aspilicious’s picture

Oh yeah.. taxonomy isn't in yet..

cosmicdreams’s picture

I'll see if I can reroll this tomorrow.

Gábor Hojtsy’s picture

User got its language propety spawned into langcode and preferred_langcode since then FYI.

duellj’s picture

Status: Needs work » Needs review
FileSize
48.2 KB

Updated patch to apply against HEAD, as well as including the langcode and preferred_langcode changes from #55 and changes from #50.

There was also a bug where the user's password would be clobbered if editing the user form without changing the password or email (i.e. not providing a verification password). That should fix the simpletest errors from #44.

Given the direction of the other entity patches, this patch should be updated to include type hinting. I'll see if I can't roll that into the next patch, if all tests pass.

Status: Needs review » Needs work

The last submitted patch, user-entity-1361228-56.patch, failed testing.

duellj’s picture

Status: Needs work » Needs review

#56: user-entity-1361228-56.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, user-entity-1361228-56.patch, failed testing.

duellj’s picture

Status: Needs work » Needs review
FileSize
48.18 KB

Tests should pass now.

tim.plunkett’s picture

Status: Needs review » Needs work
+++ b/core/modules/user/user.moduleundefined
@@ -1126,14 +908,14 @@ function user_account_form_validate($form, &$form_state) {
+    elseif ((bool) db_select('users')->fields('users', array('uid'))->condition('uid', (int)$account->uid, '<>')->condition('name', db_like($form_state['values']['name']), 'LIKE')->range(0, 1)->execute()->fetchField()) {

When casting, a space is needed. (int) $account->uid. Also, why the cast?

+++ b/core/modules/user/user.moduleundefined
@@ -1126,14 +908,14 @@ function user_account_form_validate($form, &$form_state) {
+  if ((bool) db_select('users')->fields('users', array('uid'))->condition('uid', (int)$account->uid, '<>')->condition('mail', db_like($mail), 'LIKE')->range(0, 1)->execute()->fetchField()) {

See above

+++ b/core/modules/user/user.testundefined
@@ -1572,14 +1572,14 @@ class UserBlocksUnitTests extends DrupalWebTestCase {
+ * Test case to test account saving behaviour.

s/behaviour/behavior. It was wrong before, maybe doesn't need to be changed as part of this, but the line is being changed anyway.

+++ b/core/modules/user/user.testundefined
@@ -1661,7 +1660,7 @@ class UserCreateTestCase extends DrupalWebTestCase {
+ * Test case to test account saving behaviour.

Same.

Is it the responsibility of this patch to also clean up drupal_anonymous_user() and its usage? I see a couple hunks where its replaced with entity_create('user', array());

tim.plunkett’s picture

Also, it seems that #1361234: Make the node entity a classed object and #1361226: Make the file entity a classed object are adding type-hinting, see #1480866: Add type-hinting and parameter type docmentation for comment objects. This is pretty close so it could be a follow-up, but wanted to point that out.

duellj’s picture

Thanks tim. I'm working on adding type hinting into this patch, since it does seem to be the trend of the rest of the entity related patches (and makes sense to be rolled into this issue).

When casting, a space is needed. (int) $account->uid. Also, why the cast?

Looks like this was added in the patch from #44:

The openid tests failed because $form['#user'] had uid set to NULL, which caused the query to user name duplicate query to behave unexpectecly. Fixed in a hackish way, not sure what's the best way here.

Maybe there's a better way to do this?

Is it the responsibility of this patch to also clean up drupal_anonymous_user() and its usage?

Good catch. I think that drupal_anonymous_user() should absolutely return a User object instead of a stdClass. This will be especially relevant when type hint gets included.

I'm working on rolling all of these changes into a new patch.

Berdir’s picture

Using User in that function doesn't work because it's called too early.. notably in http://api.drupal.org/api/drupal/includes%21session.inc/function/drupal_....

global $user is actually not really a user object and making the distinction here might actually be a good idea. User is a loaded entity, global $user is just whatever happens to be in the user table, without fields and things like that.

duellj’s picture

Ok, just reread the comment from #32:

Discussed this with fago, reverted the User enforcement in the hooks and back to a stdClass for session user objects and drupal_anonymous_user().

Both need more discussion/work...

So holding off on everything from #63 until there's more discussion.

Berdir’s picture

+++ b/core/modules/entity/entity.class.inc
@@ -163,7 +163,11 @@ class Entity implements EntityInterface {
-    $this->entityInfo = entity_get_info($this->entityType);
+    // @todo: entity_get_info() is not yet defined if this function is called
+    // too early. This happens e.g. when saving entities with variable_set().
+    if (function_exists('entity_get_info')) {
+      $this->entityInfo = entity_get_info($this->entityType);
+    }

There's also this part to be dealt with.

My suggestion would actually be to stop loading that information all the time, as we're just unecessary moving arrays around and instead add messages for getInfo(), getIdKey() and getBundleKey() or something like that. Then we can load it on demand when we really need it and don't need to pass around these possibly large arrays (not so much for users, but the node entity info can be quite large if you have, say, 20 bundles).

That should probably be done in a separate issue and this to be put on old until we agreed on it.

The current hack is just ugly and was only added to get around the crazyness and fix the tests.

Berdir’s picture

Wrote a proof of concept patch for the suggested changed and opened #1512564: Remove entity info from the Entity classes, feedback welcome.

Berdir’s picture

And in regards to #65, global $user will hopefully soon be removed in favor of a dependency injected proper user object, which means that it will be a properly loaded, real entity that is requested on demand when necessary and when the system is fully booted (maybe with a different way for simple "logged in or not" checks. That will help with the drupal_anonymous_user() stuff.

So I'd vote to keep the status quo there and maybe add a @todo to revisit that once the whole session/user context has been improved.

fago’s picture

So I'd vote to keep the status quo there and maybe add a @todo to revisit that once the whole session/user context has been improved.

Sounds good to me.

Wrote a proof of concept patch for the suggested changed and opened #1512564: Add methods to lazy load entity information instead of prefetching it into properties, feedback welcome.

Responded there.

+++ b/core/includes/bootstrap.inc
@@ -3190,3 +3190,30 @@ function _drupal_shutdown_function() {
+ *
+ * @return
+ *   An associative array.
+ */
+function drupal_map_assoc($array, $function = NULL) {

Unrelated changes. Git merge issues?

+++ b/core/modules/user/user.entity.inc
@@ -5,13 +5,149 @@
+   */
+  public $created = NULL;

No need to define defaulting to NULL, that's the case anyway. It's the same for other properties.

+++ b/core/modules/user/user.entity.inc
@@ -5,13 +5,149 @@
+   * The timestamp when the user lasted logged in.

last logged in.

+++ b/core/modules/user/user.entity.inc
@@ -5,13 +5,149 @@
+   * The timestamp when the user lasted logged in.
+   *
+   * @var integer
+   */
+  public $login = 0;

Is 0 a sensible default? Shuouldn't it be NULL too? 0 is actually a valid timestamp.

+++ b/core/modules/user/user.entity.inc
@@ -5,13 +5,149 @@
+  public $langcode = '';

Should default to LANGUAGE_NOT_SPECIFIED.

+++ b/core/modules/user/user.entity.inc
@@ -5,13 +5,149 @@
+  public $preferred_langcode = '';

Same here.

+++ b/core/modules/user/user.entity.inc
@@ -5,13 +5,149 @@
+  public $picture = 0;

Again, wouldn't be NULL a better default or does the code rely on it being 0?

+++ b/core/modules/user/user.entity.inc
@@ -5,13 +5,149 @@
+class UserController extends EntityDatabaseStorageController {

Should become UserStorageController.

+++ b/core/modules/user/user.entity.inc
@@ -36,12 +172,12 @@ class UserController extends DrupalDefaultEntityController {
-          $account->picture = NULL;
+          $entity->picture = NULL;

It's NULL! ;)

+++ b/core/modules/user/user.entity.inc
@@ -49,4 +185,157 @@ class UserController extends DrupalDefaultEntityController {
+    $entity->is_new = $entity->isNew();

is_new handling got improved recently with the taxonomy patch. Use enforceIsNew() to set it.

+++ b/core/modules/user/user.entity.inc
@@ -49,4 +185,157 @@ class UserController extends DrupalDefaultEntityController {
+    if (empty($entity->uid)) {
+      $entity->uid = db_next_id(db_query('SELECT MAX(uid) FROM {users}')->fetchField());
+    }

!? We are we not using auto-increments as everywhere else!? Should be probably a follow-up though.

+++ b/core/modules/user/user.entity.inc
@@ -49,4 +185,157 @@ class UserController extends DrupalDefaultEntityController {
+      // Abort if the hashing failed and returned FALSE.
+      if (!$entity->pass) {
+        return FALSE;
+      }

So it aborts preSave()? What should be aborted? Maybe throw an exception instead?

+++ b/core/modules/user/user.entity.inc
@@ -49,4 +185,157 @@ class UserController extends DrupalDefaultEntityController {
+      $entity->picture = empty($entity->picture->fid) ? 0 : $entity->picture->fid;

Let's use consistently NULL instead of 0 here?

+++ b/core/modules/user/user.entity.inc
@@ -49,4 +185,157 @@ class UserController extends DrupalDefaultEntityController {
+      // Do not allow 'uid' to be changed.
+      $entity->uid = $entity->original->uid;

We don't care about this else, so let's don't care for users either.

+++ b/core/modules/user/user.entity.inc
@@ -49,4 +185,157 @@ class UserController extends DrupalDefaultEntityController {
+      if ($entity->pass != $entity->original->pass) {
+        drupal_session_destroy_uid($entity->uid);
+        if ($entity->uid == $GLOBALS['user']->uid) {
+          drupal_session_regenerate();
+        }
+      }

This is applying changes, thus should be postSave().

+++ b/core/modules/user/user.entity.inc
@@ -49,4 +185,157 @@ class UserController extends DrupalDefaultEntityController {
+      // Remove not enabled roles.
+      $entity->roles = array_filter($entity->roles);
+
+      // Reload user roles if provided.
+      if ($entity->roles != $entity->original->roles) {
+        db_delete('users_roles')
+          ->condition('uid', $entity->uid)
+          ->execute();
+
+        $query = db_insert('users_roles')->fields(array('uid', 'rid'));
+        foreach (array_keys($entity->roles) as $rid) {
+          if (!in_array($rid, array(DRUPAL_ANONYMOUS_RID, DRUPAL_AUTHENTICATED_RID))) {
+            $query->values(array(
+              'uid' => $entity->uid,
+              'rid' => $rid,
+            ));
+          }
+        }
+        $query->execute();
+      }

Actual changes should be done in postSave() so module-driven changes during user_presave take affect.

+++ b/core/modules/user/user.entity.inc
@@ -49,4 +185,157 @@ class UserController extends DrupalDefaultEntityController {
+      // Delete a blocked user's sessions to kick them if they are online.
+      if ($entity->original->status != $entity->status && $entity->status == 0) {
+        drupal_session_destroy_uid($entity->uid);
+      }
+
+      // Send emails after we have the new user object.
+      if ($entity->status != $entity->original->status) {
+        // The user's status is changing; conditionally send notification email.
+        $op = $entity->status == 1 ? 'status_activated' : 'status_blocked';
+        _user_mail_notify($op, $entity);
+      }

Again, applying changes.

+++ b/core/modules/user/user.entity.inc
@@ -49,4 +185,157 @@ class UserController extends DrupalDefaultEntityController {
+      // Allow 'created' to be set by the caller.
+      if (!isset($entity->created)) {
+        $entity->created = REQUEST_TIME;
+      }

Let's initialize it during create() and then just save it.

+++ b/core/modules/user/user.entity.inc
@@ -49,4 +185,157 @@ class UserController extends DrupalDefaultEntityController {
+      // Make sure $entity is properly initialized.
+      $entity->roles[DRUPAL_AUTHENTICATED_RID] = 'authenticated user';

Sounds like a case for create() too.

+++ b/core/modules/user/user.entity.inc
@@ -49,4 +185,157 @@ class UserController extends DrupalDefaultEntityController {
+      // Save user roles.
+      if (count($entity->roles) > 1) {
+        $query = db_insert('users_roles')->fields(array('uid', 'rid'));
+        foreach (array_keys($entity->roles) as $rid) {
+          if (!in_array($rid, array(DRUPAL_ANONYMOUS_RID, DRUPAL_AUTHENTICATED_RID))) {
+            $query->values(array(
+              'uid' => $entity->uid,
+              'rid' => $rid,
+            ));
+          }
+        }
+        $query->execute();

Again - this is applying changes, thus should be postSave().

+++ b/core/modules/user/user.module
@@ -1126,14 +908,14 @@ function user_account_form_validate($form, &$form_state) {
+    elseif ((bool) db_select('users')->fields('users', array('uid'))->condition('uid', (int)$account->uid, '<>')->condition('name', db_like($form_state['values']['name']), 'LIKE')->range(0, 1)->execute()->fetchField()) {

Why introduce a cast here? If so missing space after the cast.

+++ b/core/modules/user/user.test
@@ -501,7 +501,7 @@ class UserCancelTestCase extends DrupalWebTestCase {
-    // We cannot use user_save() here or the password would be hashed again.
+    // We cannot $account->save() here or the password would be hashed again.

misses use

+++ b/core/modules/user/user.test
@@ -1594,16 +1594,15 @@ class UserSaveTestCase extends DrupalWebTestCase {
       'is_new' => TRUE,

should use enforceIsNew()

cosmicdreams’s picture

I cleaned up everything I could understand. I couldn't find the postSave function fago talks about. Hope this helps.

aspilicious’s picture

Assigned: Unassigned » aspilicious

I'll do the post save stuff

aspilicious’s picture

Assigned: aspilicious » Unassigned
Status: Needs work » Needs review
FileSize
48.7 KB

Everything done except for the create() stuff (because I didn't understand it) and the last part about "should use enforceIsNew()" in the test case. The entity is not created yet so I don't think we should enforceIsNew() there as it is not possible to call a method on a not created object :)

Let's see what i broke with this patch...

Status: Needs review » Needs work

The last submitted patch, 1361228-user-entity-72.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
48.41 KB

Everything it seems :p You deleted drupal_map_assoc() completely ;)

Also, we're turning in circles :)

- The move of drupal_map_assoc() is intentional. It's now used before common.inc is included, so it needs to be in bootstrap.inc for now. *For now*, because it's used in __sleep() which means that this can be reverted once #1512564: Remove entity info from the Entity classes is commited. So, please review that one! Now! :)

- The users table does not use auto_increment because it inserts uid 0 user. Unless that is changed, it can't be auto_increment.

- The cast in the username validation query is necessary as uid is sometimes set to NULL which causes the query to behave weird (as it always does with you try to do comparisons with NULL in SQL).

- I assume you meant __construct() and not create(), because that is what I did (move the initialization stuff in the __construct() method.)

Let's see how this one behaves.

Berdir’s picture

FileSize
48.45 KB

Now also renamed UserController to UserStorageController in entity info.

Status: Needs review » Needs work

The last submitted patch, 1361228-user-entity-75.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
48.43 KB

This now uses enforceIsNew() in the overriden save() method and $update instead of isNew() in postSave(). Let's see where this gets us.

Status: Needs review » Needs work

The last submitted patch, 1361228-user-entity-77.patch, failed testing.

Berdir’s picture

Assigned: Unassigned » Berdir

Closer again.

Will continue with this one tomorrow.

The UserSave tests are fixed by replacing is_new with a enforceIsNew() call, something wicked is happening with user pictures once again (or still?), they seem to be set to the string '0'. OpenID tests I have yet to look into.

Berdir’s picture

Status: Needs work » Needs review
FileSize
4.04 KB
50.98 KB

Ok, this should pass. The test failures were caused by:

- The already mentioned wrong is_new stuff
- $user->login being '0', which causes if ($account->login) to equal TRUE. Forced it to an integer. I'm not sure why this is necessary now and wasn't before.
- Similar weird behavior for $user->picture, now also enforced to an integer. I also noticed that there are file_load_multiple() with fid = 0 if users do not have a picture but pictures are enabled. Fixed this, this might be something to be backported to 7.x if this bug exists there as well.
- A bunch of failing language and preferred_language tests that failed because we changed the empty default value to LANGUAGE_NOT_SPECIFIED. I'm not sure if this should be reverted and attempted in a separate patch.

See test_fixes_interdiff-do-not-test.patch for the changes I made in this last patch.

This one should be green.

aspilicious’s picture

I think there should be a space after "(int)" when casting, like this:

+    // Make sure picture is NULL if not set.
+    if (empty($this->picture)) {
+      $this->picture = (int) NULL;
+    }
+
+    // Make sure login is an integer.
+    $this->login = (int) $this->login;

Status: Needs review » Needs work

The last submitted patch, 1361228-user-entity-80.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
51.04 KB

Uhm, casting NULL to int makes no sense :p. Also fixed the missing whitespace.

xjm’s picture

Assigned: Berdir » xjm

Looking great! Working on some nitpicky cleanups.

xjm’s picture

Assigned: xjm » Unassigned
FileSize
11.97 KB
51.29 KB

Alright, I reviewed the whole patch (yay!) and cleaned up a number of minor formatting and documentation issues. Attached interdiff shows the changes in my revision of the patch.

A few other questions that came up while I was reviewing:

  1. +++ b/core/includes/bootstrap.incundefined
    @@ -3194,3 +3194,30 @@ function _drupal_shutdown_function() {
    +function drupal_map_assoc($array, $function = NULL) {
    
    +++ b/core/includes/common.incundefined
    @@ -2704,33 +2704,6 @@ function drupal_exit($destination = NULL) {
    -function drupal_map_assoc($array, $function = NULL) {
    

    I didn't know what to make of this at first but Berdir pointed me to #1512564: Remove entity info from the Entity classes.

  2. +++ b/core/modules/entity/entity.class.incundefined
    @@ -189,7 +189,11 @@ class Entity implements EntityInterface {
    -    $this->entityInfo = entity_get_info($this->entityType);
    +    // @todo: entity_get_info() is not yet defined if this function is called
    +    // too early. This happens e.g. when saving entities with variable_set().
    +    if (function_exists('entity_get_info')) {
    +      $this->entityInfo = entity_get_info($this->entityType);
    +    }
    

    Edit: this is apparently also related to the above.

  3. +++ b/core/modules/entity/tests/entity_crud_hook_test.testundefined
    @@ -375,7 +374,7 @@ class EntityCrudHookTestCase extends DrupalWebTestCase {
    -    $account = user_load($account->uid);
    +    user_load($account->uid);
    

    At first I did not understand why we were not storing this value anymore, since we do not change user_load() anywhere in this patch. However, EntityCrudHookTestCase::testUserHooks() is only testing whether the hooks are invoked in the proper order, so there's no need to store the account object again, I guess? Can anyone clarify?

  4. +++ b/core/modules/simpletest/drupal_web_test_case.phpundefined
    @@ -1230,7 +1231,7 @@ class DrupalWebTestCase extends DrupalTestCase {
    -  protected function drupalLogin(stdClass $user) {
    +  protected function drupalLogin($user) {
    

    Why not drupalLogin(User $user)? Just to avoid having to provide the namespace?

  5. +++ b/core/modules/system/system.moduleundefined
    @@ -2011,7 +2011,7 @@ function system_form_user_register_form_alter(&$form, &$form_state) {
    -function system_user_presave(&$edit, $account) {
    +function system_user_presave($account) {
    
    @@ -2021,7 +2021,7 @@ function system_user_presave(&$edit, $account) {
    -function system_user_login(&$edit, $account) {
    +function system_user_login(&$form_state, $account) {
    

    Why are we removing the form array from hook_user_presave() but not hook_user_login()? (Also, this API change should probably go in its own, separate change notification once this issue is committed.)

  6. +++ b/core/modules/user/user.entity.incundefined
    @@ -5,18 +5,171 @@
    +  /**
    +   * The timestamp when the user last accessed the site.
    +   *
    +   * @var integer
    +   */
    +  public $access;
    +
    +  /**
    +   * The timestamp when the user last logged in.
    +   *
    +   * @var integer
    +   */
    +  public $login;
    

    The names for these variables are confusing with many possible meanings and I had trouble understanding them inline. I assumed at first they were Booleans (whether the user had access; whether the user was logged in). Could we instead name them something more explicit like $last_login and $last_access?

  7. +++ b/core/modules/user/user.entity.incundefined
    @@ -36,12 +189,9 @@ class UserController extends DrupalDefaultEntityController {
    -        else {
    -          $account->picture = NULL;
    

    Are we removing this case just so that it is more consistent with the existing core pattern (leaving a property unset rather than setting it to NULL)? Edit: Or does it relate to the fact that we're setting this NULL in the constructor?

  8. +++ b/core/modules/user/user.entity.incundefined
    @@ -49,4 +199,153 @@ class UserController extends DrupalDefaultEntityController {
    +        throw new EntityMalformedException("The entity doesn't have a password");
    

    The fact that this is not translated made me do a double-take. It seems core currently translates some (but not all) exceptions. Berdir, Crell, Gábor, and I had a nice discussion about it in IRC. The consensus is that the exception should be stored and passed around as an untranslated string. References: #522746: Drupal Exception Wrapper Class, #839884: Exceptions and t()

    (However, the string should be a complete sentence with a period, so I've cleaned that up.)

  9. +++ b/core/modules/user/user.moduleundefined
    @@ -1126,14 +908,16 @@ function user_account_form_validate($form, &$form_state) {
    +    elseif ((bool) db_select('users')->fields('users', array('uid'))->condition('uid', (int) $account->uid, '<>')->condition('name', db_like($form_state['values']['name']), 'LIKE')->range(0, 1)->execute()->fetchField()) {
    ...
    +  if ((bool) db_select('users')->fields('users', array('uid'))->condition('uid', (int) $account->uid, '<>')->condition('mail', db_like($mail), 'LIKE')->range(0, 1)->execute()->fetchField()) {
    

    I realize this formatting predates the patch, but this is very hard to read. I'd prefer to chain the methods on separate lines, either within the condition here or as a separate flag. On the fence as to the extent to which the former would violate this point from our coding standards:

    Conditions should not be wrapped into multiple lines.

    For now I have wrapped the condition to multiple lines so that I can actually read the query.

  10. +++ b/core/modules/user/user.entity.incundefined
    @@ -49,4 +199,153 @@ class UserController extends DrupalDefaultEntityController {
    +      // Process picture uploads.
    

    This is totally an out-of-scope question, but have we considered making user pictures a field, maybe one that comes with the standard profile? Edit: I found #1292470: Convert user pictures to Image Field.

  11. +++ b/core/modules/user/user.moduleundefined
    @@ -3289,7 +3053,7 @@ function user_multiple_cancel_confirm_submit($form, &$form_state) {
    -        $admin_form_state['values']['_account'] = $user;
    +        $admin_form_state['values']['_account'] = user_load($user->uid);
    

    It's not immediately obvious to me--why do we need to reload the user entity here?

xjm’s picture

Alrighty, so Berdir went through #85 with me in IRC.

Why are we removing the form array from hook_user_presave() but not hook_user_login()? (Also, this API change should probably go in its own, separate change notification once this issue is committed.)

We can revert the hook_user_login() change and instead open a followup issue to fix it properly.

they were Booleans (whether the user had access; whether the user was logged in). Could we instead name them something more explicit like $last_login and $last_access?

This would need to be another followup issue because the entity properties correspond to their schemae.

+++ b/core/modules/user/user.moduleundefined
@@ -1126,14 +908,16 @@ function user_account_form_validate($form, &$form_state) {
+    elseif ((bool) db_select('users')->fields('users', array('uid'))->condition('uid', (int) $account->uid, '<>')->condition('name', db_like($form_state['values']['name']), 'LIKE')->range(0, 1)->execute()->fetchField()) {
...
+  if ((bool) db_select('users')->fields('users', array('uid'))->condition('uid', (int) $account->uid, '<>')->condition('mail', db_like($mail), 'LIKE')->range(0, 1)->execute()->fetchField()) {

We decided rewrite this to run the query first and keep the condition on one line (so more of a change than the reformatting I did in the patch above). This means changing the elseif to else { if { } } but that's OK.

It's not immediately obvious to me--why do we need to reload the user entity here?

Answer: The $user global is not a full user entity, which can lead to stuff like #1272830: User module hooks are not always invoked with a full $account object, which can cause user pictures to be deleted on save. Edit: It would be good to add an inline comment pointing this out

I'll reroll the patch, update the summary, and open some followup issues when I have time (and assuming I don't forget) unless someone else gets to these tasks first. :)

tstoeckler’s picture

+++ b/core/modules/user/user.entity.inc
@@ -222,14 +221,15 @@ class UserStorageController extends EntityDatabaseStorageController {
+        throw new EntityMalformedException("The entity doesn't have a password.");

Minor, so leaving at needs review: We usually don't abbreviate, so "doesn't" -> "does not"

xjm’s picture

Ah, good catch. I'll fix that too (or whoever rerolls can).

Fixing crosspost.

tstoeckler’s picture

Crosspost, re-adding tag

RobLoach’s picture

Status: Needs review » Needs work
+++ b/core/modules/block/block.moduleundefined
@@ -628,9 +628,9 @@ function block_form_user_profile_form_alter(&$form, &$form_state) {
  * Implements hook_user_presave().
  */
-function block_user_presave(&$edit, $account) {
-  if (isset($edit['block'])) {
-    $edit['data']['block'] = $edit['block'];
+function block_user_presave($account) {
+  if (isset($account->block)) {
+    $account->data['block'] = $account->block;
   }

Should we update the user.api.php documentation for hook_user_presave()?

+++ b/core/modules/block/block.testundefined
@@ -542,8 +542,8 @@ class BlockCacheTestCase extends DrupalWebTestCase {
     // the same permission sets.
-    user_save($this->normal_user_alt, array('roles' => $this->normal_user->roles));
     $this->normal_user_alt->roles = $this->normal_user->roles;
+    $this->normal_user_alt->save();

Love how much nicer this is.

+++ b/core/modules/entity/entity.class.incundefined
@@ -189,7 +189,11 @@ class Entity implements EntityInterface {
-    $this->entityInfo = entity_get_info($this->entityType);
+    // @todo: entity_get_info() is not yet defined if this function is called
+    // too early. This happens e.g. when saving entities with variable_set().
+    if (function_exists('entity_get_info')) {
+      $this->entityInfo = entity_get_info($this->entityType);

Do we have a follow up issue for this @todo?

+++ b/core/modules/system/system.moduleundefined
@@ -2011,7 +2011,7 @@ function system_form_user_register_form_alter(&$form, &$form_state) {
 /**
  * Implements hook_user_insert().
  */
-function system_user_presave(&$edit, $account) {
+function system_user_presave($account) {
   if (variable_get('configurable_timezones', 1) && empty($account->timezone) && !variable_get('user_default_timezone', DRUPAL_USER_TIMEZONE_DEFAULT)) {

Doesn't really look like hook_user_insert() ;-) .

+++ b/core/modules/user/user.entity.incundefined
+++ b/core/modules/user/user.entity.incundefined
@@ -5,18 +5,170 @@

@@ -5,18 +5,170 @@
  */
 
 /**
+ * Defines the user entity class.
+ */

Could we make a follow up issue to move this to PSR-0? modules/user/lib/Drupal/user/User.php, etc.

-8 days to next Drupal core point release.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
55.54 KB

Here it is with PSR-0 for User and UserStorageController. Made a note of the sister issue: #1495024: Convert the entity system to PSR-0.

Berdir’s picture

Ah, now I see why you were so fast. Unlike the Node patch, this one doesn't add the User type-hint yet. Which I think is perfectly ok to do in a follow-up task because that will probably affect a billion files or so and each will need the use statement ;)

fago’s picture

FileSize
64.85 KB

Why are we removing the form array from hook_user_presave() but not hook_user_login()?

Yep, it makes not much sense to pass $form_state here. I've just removed it as for the others.

Should we update the user.api.php documentation for hook_user_presave()?

Yep, I've worked over the docs updated them and streamlined them with the docs of other entities (comments).

Doesn't really look like hook_user_insert() ;-) .

Yep, fixed.

@creation:
I've moved the stuff from __construct() - which runs always - to the storage controllers create() method + reworked it. (see git history)

+++ b/core/modules/user/user.entity.inc
@@ -5,13 +5,149 @@
+ public $picture = 0;

Again, wouldn't be NULL a better default or does the code rely on it being 0?

Realized the db schema already has the default of 0, so let's better stay with that to avoid unnecessary schema changes. Thus, I've changed the default back to 0 and fixed the code to *always* go with 0 instead of sometimes using NULL for "no-picture".

@entity-info & drupal_map_assoc().
Let's do away with the workarounds of this patch, but base it upon the latest patch/branch from #1512564: Remove entity info from the Entity classes. I've added the patch to Git and done so + applied all the other changes there. See the entity-user branch in http://drupal.org/sandbox/fago/1497344.

Please also make use of the Git repository for further follow-ups to ease further rerolls. I've granted you access - or if you need access please contact me.

Updated patch attached (includes #1512564: Remove entity info from the Entity classes)

Berdir’s picture

Discussed hook_user_login() and we agreed that this should be tackled in a separate issue, it has nothing to do with this conversion.

As mentioned before, the additional argument there is important as it allows to do proper redirects on user login. drupal_goto() there is just as wrong as doing drupal_goto() in a submit callback because we break other hook_user_login() implementations.

Let's ignore that hook for now and tackle this later...

Status: Needs review » Needs work

The last submitted patch, d8-entity-user.patch, failed testing.

xjm’s picture

Thanks @Rob Loach; that should save time.

Do we have a follow up issue for this @todo?

Aye, that's #1512564: Remove entity info from the Entity classes. And yes, let's please do the type hinting in a followup if possible.

Remaining tasks:

  1. Fix #1512564: Remove entity info from the Entity classes and then fix the @todo in this issue.
  2. Open a followup to add type hinting for user entities.
  3. Open a followup to rename $user->access and $user->login to something more self-explanatory.
  4. +++ b/core/modules/user/user.moduleundefined
    @@ -3289,7 +3065,7 @@ function user_multiple_cancel_confirm_submit($form, &$form_state) {
    -        $admin_form_state['values']['_account'] = $user;
    +        $admin_form_state['values']['_account'] = user_load($user->uid);
    

    Add inline comment explaining this change: The $user global is not a complete user entity, so load the full entity.

  5. +++ b/core/modules/system/system.moduleundefined
    @@ -2021,7 +2021,7 @@ function system_user_presave(&$edit, $account) {
    -function system_user_login(&$edit, $account) {
    +function system_user_login(&$form_state, $account) {
    

    Let's revert this change, since it's out of scope.

  6. +++ b/core/modules/user/user.moduleundefined
    @@ -1126,14 +908,28 @@ function user_account_form_validate($form, &$form_state) {
    +    elseif ((bool) db_select('users')
    +      ->fields('users', array('uid'))
    +      ->condition('uid', (int) $account->uid, '<>')
    +      ->condition('name', db_like($form_state['values']['name']), 'LIKE')
    +      ->range(0, 1)
    +      ->execute()
    +      ->fetchField()) {
    ...
    +  if ((bool) db_select('users')
    +    ->fields('users', array('uid'))
    +    ->condition('uid', (int) $account->uid, '<>')
    +    ->condition('mail', db_like($mail), 'LIKE')
    +    ->range(0, 1)
    +    ->execute()
    +    ->fetchField()) {
    

    Rewrite this so that the query result is calculated first (so that the condition can be on one line), which means also changing elseif to a separate else and if so we don't run the query needlessly.

  7. +++ b/core/modules/user/lib/Drupal/user/UserStorageController.phpundefined
    @@ -0,0 +1,216 @@
    +        throw new EntityMalformedException("The entity doesn't have a password.");
    

    Still need to change this to "does not" as well (as pointed out by @tstoeckler above).

    +++ b/core/modules/system/system.moduleundefined
    @@ -2011,7 +2011,7 @@ function system_form_user_register_form_alter(&$form, &$form_state) {
     /**
      * Implements hook_user_insert().
      */
    -function system_user_presave(&$edit, $account) {
    +function system_user_presave($account) {
    

    This docblock was incorrect before, but per the documentation gate we should still fix it. Nice catch Rob. ;)

I have to go to a meeting but I'll try to add a summary for this issue later. :)

fago’s picture

Status: Needs work » Needs review
FileSize
60.17 KB

Discussed hook_user_login() and we agreed that this should be tackled in a separate issue, it has nothing to do with this conversion.

Agreed. Reverted related changes.

Open-Id tests still fail, not sure why. No time left to look at it right now :/

@xjm: Great summary, thanks!

Berdir’s picture

See comment #80 for the reason why openid tests probably fail, which is because $account->login is probably '0', which equals TRUE (PHP--).

There is a reason why I had the (int) cast for that in __construct() and not create().

fago’s picture

FileSize
52.87 KB

But a string of '0' evaluates to FALSE too. I had another look at it and realized the problem is the changed default 0 <-> NULL. So the password hash was created for a login time 0 vs NULL. The cast just converted NULL back to 0 :D

Thus, I've fixed the login and access properties to follow the schema and default to 0. As mentioned above this is weird as 0 is actually a valid timestamp. Still, that can be fixed in a follow-up, so I've just commented it for now.

#1512564: Remove entity info from the Entity classes got committed :-) - rerolled and updated patch attached. Points
2,3,4,6,7 of #96 are still todo.

Niklas Fiekas’s picture

#99: d8-entity-user.patch queued for re-testing.

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

The last submitted patch, d8-entity-user.patch, failed testing.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
3.88 KB
53.91 KB

Rerolling because of #1299424: Allow one module per directory and move system tests to core/modules/system and #189544: Allow administrator to edit account without email address (regression: accounts can be created without email addresses also).

Also:
2: Still needs a follow-up
3: Still needs a follow-up
4: See interdiff
6: See interdiff

7: Converted doesn't, found the doc change already done

Status: Needs review » Needs work

The last submitted patch, 1361228-entity-user-102.patch, failed testing.

Niklas Fiekas’s picture

aspilicious’s picture

Why do we do this (srry if it is a stupid question)

-    $account = user_load($account->uid);
+    user_load($account->uid);

Isn't the user_load useless in that case?

cosmicdreams’s picture

Niklas Fiekas’s picture

@aspilicious (#105): That one is just a testcase that ensures the relevant hooks are executed (in the right order?).

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Regarding #105, I asked the same thing in IRC. :P It's fine though.

Alright. Going to take the leap... I think this is RTBC. :)

Edit: Also mentioned the followups on the meta.

sun’s picture

#104: 1361228-entity-user-104.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

The last submitted patch, 1361228-entity-user-104.patch, failed testing.

xjm’s picture

Assigned: Unassigned » xjm

Rerolling.

xjm’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
54.82 KB

Rebased, two merge conflicts with changed docblocks. It's all my fault. :(

sun’s picture

#115: user-entity-1361228-115.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

The last submitted patch, user-entity-1361228-115.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
52.8 KB

Parts of this patch were addressed in #1519942: Decouple locale module from user module, which is why the diffstat is smaller (and why it failed).

EDITED to correct the conflicting issue

Jelle_S’s picture

Status: Needs review » Needs work
+++ b/core/modules/user/lib/Drupal/user/UserStorageController.phpundefined
@@ -0,0 +1,227 @@
+  public function save(EntityInterface $entity) {
+    if (empty($entity->uid)) {
+      $entity->uid = db_next_id(db_query('SELECT MAX(uid) FROM {users}')->fetchField());
+      $entity->enforceIsNew();
+    }
+    parent::save($entity);

Somehow this seems weird to me. Correct me if I'm wrong, but isn't the uid column in the users table a serial field?
From the db_next_id() documentation:

Use this function if for some reason you can't use a serial field. Using a serial field is preferred, and InsertQuery::execute() returns the value of the last ID inserted.

There might be a reason why we're doing this, and if there is, I missed it, so please feel free to correct me ;-)

Berdir’s picture

Status: Needs work » Needs review

No, it's not a serial field. It can't be, to properly support our anonymous user uid 0 and being able to make sure that uid 1 is uid 1. Everything else requires crazy hacks to work across different databases.

It is also nothing that is changed in here.

fago’s picture

@serial-field:
Yep, that's odd but there is a reason. See #74:

The users table does not use auto_increment because it inserts uid 0 user. Unless that is changed, it can't be auto_increment.

Anyway, I think that should be fixed in a follow-up.

Update: cross-post, berdir was faster :)

Niklas Fiekas’s picture

FileSize
12.09 KB

I diffed the patch in #118 against the one #115. The result of that is attached. The few differences that are actually in code look good. So thank you and back to RTBC.

Edit: Wow, lots of new posts (regarding more than the interdiff) in the meantime. RTBC still seems to be justified.

Jelle_S’s picture

@Berdir, @fago

oh, ok, thanks for clearing that up for me :-)

Jelle_S’s picture

Status: Needs review » Reviewed & tested by the community

status mix-up

xjm’s picture

Assigned: xjm » Unassigned

:)

catch’s picture

Status: Reviewed & tested by the community » Fixed

OK this looks fine. The user picture hunks, even though just moved around, are horrifying, but we have #1292470: Convert user pictures to Image Field for that. I opened #1548204: Remove user signature and move it to contrib for signatures since I couldn't find an existing issue.

Committed/pushed to 8.x, thanks all!

cosmicdreams’s picture

HOT DOG! AWESOME!

Berdir’s picture

Title: Make the user entity a classed object » Change notification for: Make the user entity a classed object
Priority: Major » Critical
Status: Fixed » Active

At least the hook_user_*() changes deserve a separate change notification I think. List the affected hooks and how their parameters changed and maybe include a before/after example, this patch should have enough of them.

xjm’s picture

We should additionally update the other change notification:
http://drupal.org/node/1400186

aspilicious’s picture

I added the needed info in http://drupal.org/node/1400186

xjm’s picture

I think we also need to document the PSR-0 namespacing of these classes under a subheader, preferably in a nice pretty table like we have for http://drupal.org/node/1479568. This applies to the other entity conversions as well. Edit: Only Node and User are namespaced so far.

RobLoach’s picture

Issue tags: +PSR-0

Could we get a PSR-0 issue for this? Just tagging PSR-0 for now so we can track the follow up later on.

RobLoach’s picture

Ah, we already did PSR-0 this. Just wasn't tagged, no need for a follow-up then! Carry on :-) .

xjm’s picture

Category: feature » task

Oopsie.

Berdir’s picture

Category: task » feature
Issue tags: -PSR-0

Maybe we could instead update the existing entities change record, make that a table with old/new name and use the fully qualified PSR-0 name there? Having two separate change records would mean that you had to read both of them to be able to follow the renames. I guess that's a problem that we will have in other places as well, imagine we decide to change Drupal\$module_name to Drupal\$ModuleName in 4 month :)

Berdir’s picture

Category: feature » task
Issue tags: +PSR-0

oh, crosspost^2.

xjm’s picture

Re: #135, that's what I suggested in #131. :) Edit: I guess we should probably mention it in http://drupal.org/node/1400186 as a short reference to http://drupal.org/node/1479568. I retitled the latter to redefine its scope to include core modules.

nevergone’s picture

Drupal 7 cross-post, please help: #1399798: Anonymous user properties are hardcoded
If anonymous user properties are not hardcoded, possible that user entity uses bundles.
Thanks for all! :)

Berdir’s picture

Updated http://drupal.org/node/1400186, this will still need a separate change record for the user_save() and user hook changes, see #128.

Berdir’s picture

Status: Active » Needs review

Ok, here we go: http://drupal.org/node/1554986

No example yet for the hooks.

aspilicious’s picture

Status: Needs review » Fixed

Added hook example. We are done now :)

aspilicious’s picture

Title: Change notification for: Make the user entity a classed object » Make the user entity a classed object
Priority: Critical » Major

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

tim.plunkett’s picture

Title: Make the user entity a classed object » Change notification: Make the user entity a classed object
Priority: Major » Critical
Status: Closed (fixed) » Active

http://drupal.org/node/1554986 is the change notice for this issue, but it doesn't mention that user_save was removed completely.

Berdir’s picture

The code example is actually correct, we just need to rewrite "Additionally, user_save() is now a deprecated wrapper of the $account->save() method and will likely be removed at a later point." and the title.

tim.plunkett’s picture

Title: Change notification: Make the user entity a classed object » Make the user entity a classed object
Priority: Critical » Major
Status: Active » Fixed

I split these change notices up, it was too confusing to have them combined. See http://drupal.org/node/1688430

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

benjifisher’s picture

In case anyone is trying to track down the right commit, it is here: 93c20fd. Unless there is more than one ...