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.

Files: 
CommentFileSizeAuthor
#122 1361228-interpatch-diff.txt12.09 KBNiklas Fiekas
#118 drupal-1361228-118.patch52.8 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 35,428 pass(es).
[ View ]
#115 user-entity-1361228-115.patch54.82 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-entity-1361228-115.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#104 1361228-entity-user-104.patch54.84 KBNiklas Fiekas
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1361228-entity-user-104.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#104 1361228-entity-user-104-interdiff.txt1.1 KBNiklas Fiekas
#102 1361228-entity-user-102.patch53.91 KBNiklas Fiekas
FAILED: [[SimpleTest]]: [MySQL] 35,012 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#102 1361228-entity-user-102-interdiff.txt3.88 KBNiklas Fiekas
#99 d8-entity-user.patch52.87 KBfago
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8-entity-user_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#97 d8-entity-user.patch60.17 KBfago
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8-entity-user_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#93 d8-entity-user.patch64.85 KBfago
FAILED: [[SimpleTest]]: [MySQL] 36,229 pass(es), 13 fail(s), and 18 exception(s).
[ View ]
#91 1361228.patch55.54 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 36,251 pass(es).
[ View ]
#85 user-entity-1361228-85.patch51.29 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 36,243 pass(es).
[ View ]
#85 interdiff.txt11.97 KBxjm
#83 1361228-user-entity-83.patch51.04 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 36,168 pass(es).
[ View ]
#80 1361228-user-entity-80.patch50.98 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 36,158 pass(es), 1 fail(s), and 2 exception(s).
[ View ]
#80 test_fixes_interdiff-do-not-test.patch4.04 KBBerdir
#77 1361228-user-entity-77.patch48.43 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 36,124 pass(es), 21 fail(s), and 9 exception(s).
[ View ]
#75 1361228-user-entity-75.patch48.45 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 25,821 pass(es), 9,782 fail(s), and 8,062 exception(s).
[ View ]
#74 1361228-user-entity-74.patch48.41 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#72 1361228-user-entity-72.patch48.7 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#70 1361228_70_code_comments.patch47.11 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#60 user-entity-1361228-60.patch48.18 KBduellj
PASSED: [[SimpleTest]]: [MySQL] 35,729 pass(es).
[ View ]
#56 user-entity-1361228-56.patch48.2 KBduellj
FAILED: [[SimpleTest]]: [MySQL] 35,718 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#47 remove_debug_code.patch50.81 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 34,611 pass(es), 4 fail(s), and 0 exception(es).
[ View ]
#44 fixed_most_testfails.patch51.19 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 34,541 pass(es), 4 fail(s), and 0 exception(es).
[ View ]
#35 user-entity-rename-account-to-entity.patch47.89 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 34,359 pass(es), 31 fail(s), and 0 exception(es).
[ View ]
#33 user-entity-revert-User.patch47.13 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 34,350 pass(es), 35 fail(s), and 192 exception(es).
[ View ]
#30 user-entity-more-tests-fixed.patch58.5 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#28 user-entity-saving-and-hooks.patch56.19 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 34,100 pass(es), 287 fail(s), and 51 exception(es).
[ View ]
#22 user-entity.patch12.84 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] 34,101 pass(es), 103 fail(s), and 61 exception(es).
[ View ]
#20 user-entity.patch11.69 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] 34,095 pass(es), 103 fail(s), and 61 exception(es).
[ View ]
#14 drupal-8-user-entity-classed-object-1361228.patch7.3 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] 23,777 pass(es), 9,539 fail(s), and 3,783 exception(es).
[ View ]
#11 drupal-8-user-entity-classed-object-1361228-5.patch4.39 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] 21,858 pass(es), 12,495 fail(s), and 7,761 exception(es).
[ View ]
#9 drupal-8-user-entity-classed-object-1361228-5.patch4.39 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] 21,883 pass(es), 12,476 fail(s), and 7,743 exception(es).
[ View ]
#8 drupal-8-user-entity-classed-object-1361228-5.patch4.84 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#6 drupal-8-user-entity-classed-object-1361228-5.patch5.33 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#4 drupal-8-user-entity-classed-object-1361228-4.patch2.41 KBrlmumford
FAILED: [[SimpleTest]]: [MySQL] 34,148 pass(es), 72 fail(s), and 26 exception(es).
[ View ]
#1 drupal-8-user-entity-classed-object-1361228-1.patch1.99 KBrlmumford
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/user/user.entity.inc.
[ View ]

Comments

StatusFileSize
new1.99 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/user/user.entity.inc.
[ View ]

Made a start on this.

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

Status:Active» Needs review

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new2.41 KB
FAILED: [[SimpleTest]]: [MySQL] 34,148 pass(es), 72 fail(s), and 26 exception(es).
[ View ]

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

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

<?php
$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.

Status:Needs work» Needs review
StatusFileSize
new5.33 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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

Status:Needs review» Needs work

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

StatusFileSize
new4.84 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Debug :(

StatusFileSize
new4.39 KB
FAILED: [[SimpleTest]]: [MySQL] 21,883 pass(es), 12,476 fail(s), and 7,743 exception(es).
[ View ]

Take 3

Take 3

StatusFileSize
new4.39 KB
FAILED: [[SimpleTest]]: [MySQL] 21,858 pass(es), 12,495 fail(s), and 7,761 exception(es).
[ View ]

Status:Needs work» Needs review

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new7.3 KB
FAILED: [[SimpleTest]]: [MySQL] 23,777 pass(es), 9,539 fail(s), and 3,783 exception(es).
[ View ]

Some what improved patch

Status:Needs review» Needs work

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

variables don't start with a capital letter

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.

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.

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

Status:Needs work» Needs review
StatusFileSize
new11.69 KB
FAILED: [[SimpleTest]]: [MySQL] 34,095 pass(es), 103 fail(s), and 61 exception(es).
[ View ]

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.

Status:Needs review» Needs work

Back to needs work as the bot now has this.

Status:Needs work» Needs review
StatusFileSize
new12.84 KB
FAILED: [[SimpleTest]]: [MySQL] 34,101 pass(es), 103 fail(s), and 61 exception(es).
[ View ]

Reroll using user entity for site installs

Status:Needs review» Needs work

Category:task» feature

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.

Issue tags:+Entity system

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

Status:Needs work» Needs review
StatusFileSize
new56.19 KB
FAILED: [[SimpleTest]]: [MySQL] 34,100 pass(es), 287 fail(s), and 51 exception(es).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new58.5 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

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

StatusFileSize
new47.13 KB
FAILED: [[SimpleTest]]: [MySQL] 34,350 pass(es), 35 fail(s), and 192 exception(es).
[ View ]

Forgot the patch...

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new47.89 KB
FAILED: [[SimpleTest]]: [MySQL] 34,359 pass(es), 31 fail(s), and 0 exception(es).
[ View ]

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.

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.

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

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.

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

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

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

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.

StatusFileSize
new51.19 KB
FAILED: [[SimpleTest]]: [MySQL] 34,541 pass(es), 4 fail(s), and 0 exception(es).
[ View ]

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

Status:Needs work» Needs review

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new50.81 KB
FAILED: [[SimpleTest]]: [MySQL] 34,611 pass(es), 4 fail(s), and 0 exception(es).
[ View ]

Removed some debug code.

Status:Needs review» Needs work

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

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.

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

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.

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

I'll see if I can reroll this tomorrow.

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

Status:Needs work» Needs review
StatusFileSize
new48.2 KB
FAILED: [[SimpleTest]]: [MySQL] 35,718 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new48.18 KB
PASSED: [[SimpleTest]]: [MySQL] 35,729 pass(es).
[ View ]

Tests should pass now.

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

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.

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.

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.

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.

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

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

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.

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

StatusFileSize
new47.11 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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

Assigned:Unassigned» aspilicious

I'll do the post save stuff

Assigned:aspilicious» Unassigned
Status:Needs work» Needs review
StatusFileSize
new48.7 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new48.41 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

StatusFileSize
new48.45 KB
FAILED: [[SimpleTest]]: [MySQL] 25,821 pass(es), 9,782 fail(s), and 8,062 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new48.43 KB
FAILED: [[SimpleTest]]: [MySQL] 36,124 pass(es), 21 fail(s), and 9 exception(s).
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new4.04 KB
new50.98 KB
FAILED: [[SimpleTest]]: [MySQL] 36,158 pass(es), 1 fail(s), and 2 exception(s).
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new51.04 KB
PASSED: [[SimpleTest]]: [MySQL] 36,168 pass(es).
[ View ]

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

Assigned:Berdir» xjm

Looking great! Working on some nitpicky cleanups.

Assigned:xjm» Unassigned
StatusFileSize
new11.97 KB
new51.29 KB
PASSED: [[SimpleTest]]: [MySQL] 36,243 pass(es).
[ View ]

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?

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

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

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

Fixing crosspost.

Crosspost, re-adding tag

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.

Status:Needs work» Needs review
StatusFileSize
new55.54 KB
PASSED: [[SimpleTest]]: [MySQL] 36,251 pass(es).
[ View ]

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.

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

StatusFileSize
new64.85 KB
FAILED: [[SimpleTest]]: [MySQL] 36,229 pass(es), 13 fail(s), and 18 exception(s).
[ View ]

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)

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.

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

Status:Needs work» Needs review
StatusFileSize
new60.17 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8-entity-user_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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!

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

StatusFileSize
new52.87 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8-entity-user_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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

Status:Needs work» Needs review
StatusFileSize
new3.88 KB
new53.91 KB
FAILED: [[SimpleTest]]: [MySQL] 35,012 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new1.1 KB
new54.84 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1361228-entity-user-104.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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?

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

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.

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

Assigned:Unassigned» xjm

Rerolling.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new54.82 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-entity-1361228-115.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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

Status:Needs work» Needs review
StatusFileSize
new52.8 KB
PASSED: [[SimpleTest]]: [MySQL] 35,428 pass(es).
[ View ]

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

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

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.

@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 :)

StatusFileSize
new12.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.

@Berdir, @fago

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

Status:Needs review» Reviewed & tested by the community

status mix-up

Assigned:xjm» Unassigned

:)

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: Convert user signature into a normal field for signatures since I couldn't find an existing issue.

Committed/pushed to 8.x, thanks all!

HOT DOG! AWESOME!

Title:Make the user entity a classed objectChange 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.

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

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

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.

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.

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

Category:feature» task

Oopsie.

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

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

oh, crosspost^2.

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.

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! :)

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

Status:Active» Needs review

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

No example yet for the hooks.

Status:Needs review» Fixed

Added hook example. We are done now :)

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

Status:Fixed» Closed (fixed)

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

Title:Make the user entity a classed objectChange 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.

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.

Title:Change notification: Make the user entity a classed objectMake 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.

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