Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#122 | 1361228-interpatch-diff.txt | 12.09 KB | Niklas Fiekas |
#118 | drupal-1361228-118.patch | 52.8 KB | tim.plunkett |
#115 | user-entity-1361228-115.patch | 54.82 KB | xjm |
#104 | 1361228-entity-user-104.patch | 54.84 KB | Niklas Fiekas |
#104 | 1361228-entity-user-104-interdiff.txt | 1.1 KB | Niklas Fiekas |
Comments
Comment #1
rlmumfordMade a start on this.
Defined all the variables in the Drupal 7 user object as public variables of the User Class.
Comment #2
rlmumfordComment #4
rlmumfordMainly a re-roll, but I've also added the entity class key into the user entity info array.
Comment #5
tstoecklercreate -> created. Also I think "for" can be omitted.
Again, I think "for" can be omitted here.
I think the (1) and (0) can be omitted.
I don't know if we have any standard for this. On the one hand calling this "fid" makes it clear that
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.
Comment #6
marcingy CreditAttribution: marcingy commentedTidying up comments above and starting to build out the user controller. This will fail but the tests can now create users.
Comment #7
marcingy CreditAttribution: marcingy commentedAnd back to needs work now this is off to the bot.
Comment #8
marcingy CreditAttribution: marcingy commentedDebug :(
Comment #9
marcingy CreditAttribution: marcingy commentedTake 3
Comment #10
marcingy CreditAttribution: marcingy commentedTake 3
Comment #11
marcingy CreditAttribution: marcingy commentedComment #12
marcingy CreditAttribution: marcingy commentedComment #13
marcingy CreditAttribution: marcingy commentedComment #14
marcingy CreditAttribution: marcingy commentedSome what improved patch
Comment #15
marcingy CreditAttribution: marcingy commentedComment #16
jbrown CreditAttribution: jbrown commentedvariables don't start with a capital letter
Comment #17
fagoI fear this one is going to be a tough one - who doesn't love user.module?
This function should receive just $account.
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.
Same here.
Missing trailing point.
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.
Comment #18
Gábor HojtsyUnfortunately $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.
Comment #19
marcingy CreditAttribution: marcingy commented@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.
Comment #20
marcingy CreditAttribution: marcingy commentedSome 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.
Comment #21
marcingy CreditAttribution: marcingy commentedBack to needs work as the bot now has this.
Comment #22
marcingy CreditAttribution: marcingy commentedReroll using user entity for site installs
Comment #23
marcingy CreditAttribution: marcingy commentedComment #24
xjmRecategorizing per #1346204-11: [meta] Drupal 8 Entity API improvements.
Comment #25
xjmNote 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.
Comment #26
sunComment #27
aspilicious CreditAttribution: aspilicious commentedI 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 userComment #28
BerdirYes, 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.
Comment #30
BerdirThe attached patch now also creates User instances for anymous user objects and session user objects.
Added a check to remove unselected roles.
Comment #32
BerdirDiscussed 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...
Comment #33
BerdirForgot the patch...
Comment #35
BerdirRenamed $account to $entity in preSave(), copy & paste errors...
Comment #37
BerdirI 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.
Comment #38
David_Rothstein CreditAttribution: David_Rothstein commentedJust skimming through the patch and noticed that sometimes it does things like this:
But other times does things like this:
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()...
Comment #39
BerdirI'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.
Comment #40
Crell CreditAttribution: Crell commentedMy 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().
Comment #41
sun$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.
Comment #42
Berdiruser_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 ;)
Comment #43
Crell CreditAttribution: Crell commentedSage advise. :-) I agree, if we have to change user_save() anyway, go ahead and euthanize it.
Comment #44
BerdirOk, 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.
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...
Comment #45
BerdirComment #47
BerdirRemoved some debug code.
Comment #49
sunI'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.
Comment #50
aspilicious CreditAttribution: aspilicious commentedSome points for the next reroll
Newline needed
Needs an override docblock
docblock needed
same
Comment #51
aspilicious CreditAttribution: aspilicious commented#47: remove_debug_code.patch queued for re-testing.
Comment #53
aspilicious CreditAttribution: aspilicious commentedOh yeah.. taxonomy isn't in yet..
Comment #54
cosmicdreams CreditAttribution: cosmicdreams commentedI'll see if I can reroll this tomorrow.
Comment #55
Gábor HojtsyUser got its language propety spawned into langcode and preferred_langcode since then FYI.
Comment #56
duellj CreditAttribution: duellj commentedUpdated 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.
Comment #58
duellj CreditAttribution: duellj commented#56: user-entity-1361228-56.patch queued for re-testing.
Comment #60
duellj CreditAttribution: duellj commentedTests should pass now.
Comment #61
tim.plunkettWhen casting, a space is needed.
(int) $account->uid
. Also, why the cast?See above
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.
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 withentity_create('user', array());
Comment #62
tim.plunkettAlso, 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.
Comment #63
duellj CreditAttribution: duellj commentedThanks 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).
Looks like this was added in the patch from #44:
Maybe there's a better way to do this?
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.
Comment #64
BerdirUsing 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.
Comment #65
duellj CreditAttribution: duellj commentedOk, just reread the comment from #32:
So holding off on everything from #63 until there's more discussion.
Comment #66
BerdirThere'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.
Comment #67
BerdirWrote a proof of concept patch for the suggested changed and opened #1512564: Remove entity info from the Entity classes, feedback welcome.
Comment #68
BerdirAnd 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.
Comment #69
fagoSounds good to me.
Responded there.
Unrelated changes. Git merge issues?
No need to define defaulting to NULL, that's the case anyway. It's the same for other properties.
last logged in.
Is 0 a sensible default? Shuouldn't it be NULL too? 0 is actually a valid timestamp.
Should default to LANGUAGE_NOT_SPECIFIED.
Same here.
Again, wouldn't be NULL a better default or does the code rely on it being 0?
Should become UserStorageController.
It's NULL! ;)
is_new handling got improved recently with the taxonomy patch. Use enforceIsNew() to set it.
!? We are we not using auto-increments as everywhere else!? Should be probably a follow-up though.
So it aborts preSave()? What should be aborted? Maybe throw an exception instead?
Let's use consistently NULL instead of 0 here?
We don't care about this else, so let's don't care for users either.
This is applying changes, thus should be postSave().
Actual changes should be done in postSave() so module-driven changes during user_presave take affect.
Again, applying changes.
Let's initialize it during create() and then just save it.
Sounds like a case for create() too.
Again - this is applying changes, thus should be postSave().
Why introduce a cast here? If so missing space after the cast.
misses use
should use enforceIsNew()
Comment #70
cosmicdreams CreditAttribution: cosmicdreams commentedI cleaned up everything I could understand. I couldn't find the postSave function fago talks about. Hope this helps.
Comment #71
aspilicious CreditAttribution: aspilicious commentedI'll do the post save stuff
Comment #72
aspilicious CreditAttribution: aspilicious commentedEverything 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...
Comment #74
BerdirEverything 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.
Comment #75
BerdirNow also renamed UserController to UserStorageController in entity info.
Comment #77
BerdirThis now uses enforceIsNew() in the overriden save() method and $update instead of isNew() in postSave(). Let's see where this gets us.
Comment #79
BerdirCloser 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.
Comment #80
BerdirOk, 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.
Comment #81
aspilicious CreditAttribution: aspilicious commentedI think there should be a space after "(int)" when casting, like this:
Comment #83
BerdirUhm, casting NULL to int makes no sense :p. Also fixed the missing whitespace.
Comment #84
xjmLooking great! Working on some nitpicky cleanups.
Comment #85
xjmAlright, 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:
I didn't know what to make of this at first but Berdir pointed me to #1512564: Remove entity info from the Entity classes.
Edit: this is apparently also related to the above.
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?Why not
drupalLogin(User $user)
? Just to avoid having to provide the namespace?Why are we removing the form array from
hook_user_presave()
but nothook_user_login()
? (Also, this API change should probably go in its own, separate change notification once this issue is committed.)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
?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?
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.)
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:
For now I have wrapped the condition to multiple lines so that I can actually read the query.
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.
It's not immediately obvious to me--why do we need to reload the user entity here?
Comment #86
xjmAlrighty, so Berdir went through #85 with me in IRC.
We can revert the
hook_user_login()
change and instead open a followup issue to fix it properly.This would need to be another followup issue because the entity properties correspond to their schemae.
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
toelse { if { } }
but that's OK.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 outI'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. :)
Comment #87
tstoecklerMinor, so leaving at needs review: We usually don't abbreviate, so "doesn't" -> "does not"
Comment #88
xjmAh, good catch. I'll fix that too (or whoever rerolls can).
Fixing crosspost.
Comment #89
tstoecklerCrosspost, re-adding tag
Comment #90
RobLoachShould we update the user.api.php documentation for hook_user_presave()?
Love how much nicer this is.
Do we have a follow up issue for this @todo?
Doesn't really look like hook_user_insert() ;-) .
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.
Comment #91
RobLoachHere it is with PSR-0 for User and UserStorageController. Made a note of the sister issue: #1495024: Convert the entity system to PSR-0.
Comment #92
BerdirAh, 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 ;)
Comment #93
fagoYep, it makes not much sense to pass $form_state here. I've just removed it as for the others.
Yep, I've worked over the docs updated them and streamlined them with the docs of other entities (comments).
Yep, fixed.
@creation:
I've moved the stuff from __construct() - which runs always - to the storage controllers create() method + reworked it. (see git history)
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)
Comment #94
BerdirDiscussed 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...
Comment #96
xjmThanks @Rob Loach; that should save time.
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:
$user->access
and$user->login
to something more self-explanatory.Add inline comment explaining this change: The $user global is not a complete user entity, so load the full entity.
Let's revert this change, since it's out of scope.
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.
Still need to change this to "does not" as well (as pointed out by @tstoeckler above).
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. :)
Comment #97
fagoAgreed. Reverted related changes.
Open-Id tests still fail, not sure why. No time left to look at it right now :/
@xjm: Great summary, thanks!
Comment #98
BerdirSee 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().
Comment #99
fagoBut 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.
Comment #100
Niklas Fiekas CreditAttribution: Niklas Fiekas commented#99: d8-entity-user.patch queued for re-testing.
Comment #102
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedRerolling 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
Comment #104
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedOh, and #189544: Allow administrator to edit account without email address (regression: accounts can be created without email addresses also) introduced new
user_save()
's.Comment #105
aspilicious CreditAttribution: aspilicious commentedWhy do we do this (srry if it is a stupid question)
Isn't the user_load useless in that case?
Comment #106
cosmicdreams CreditAttribution: cosmicdreams commented#2 and #3 are satisfied by these two issues:
#1537434: Add type-hinting to user entities
#1537442: Rename $user->access and $user->login
Comment #107
Niklas Fiekas CreditAttribution: Niklas Fiekas commented@aspilicious (#105): That one is just a testcase that ensures the relevant hooks are executed (in the right order?).
Comment #108
xjmRegarding #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.
Comment #109
sun#104: 1361228-entity-user-104.patch queued for re-testing.
Comment #111
xjmRerolling.
Comment #115
xjmRebased, two merge conflicts with changed docblocks. It's all my fault. :(
Comment #116
sun#115: user-entity-1361228-115.patch queued for re-testing.
Comment #118
tim.plunkettParts 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
Comment #119
Jelle_SSomehow 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:
There might be a reason why we're doing this, and if there is, I missed it, so please feel free to correct me ;-)
Comment #120
BerdirNo, 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.
Comment #121
fago@serial-field:
Yep, that's odd but there is a reason. See #74:
Anyway, I think that should be fixed in a follow-up.
Update: cross-post, berdir was faster :)
Comment #122
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedI 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.
Comment #123
Jelle_S@Berdir, @fago
oh, ok, thanks for clearing that up for me :-)
Comment #124
Jelle_Sstatus mix-up
Comment #125
xjm:)
Comment #126
catchOK 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!
Comment #127
cosmicdreams CreditAttribution: cosmicdreams commentedHOT DOG! AWESOME!
Comment #128
BerdirAt 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.
Comment #129
xjmWe should additionally update the other change notification:
http://drupal.org/node/1400186
Comment #130
aspilicious CreditAttribution: aspilicious commentedI added the needed info in http://drupal.org/node/1400186
Comment #131
xjmI 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.
Comment #132
RobLoachCould we get a PSR-0 issue for this? Just tagging PSR-0 for now so we can track the follow up later on.
Comment #133
RobLoachAh, we already did PSR-0 this. Just wasn't tagged, no need for a follow-up then! Carry on :-) .
Comment #134
xjmOopsie.
Comment #135
BerdirMaybe 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 :)
Comment #136
Berdiroh, crosspost^2.
Comment #137
xjmRe: #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.
Comment #138
nevergone CreditAttribution: nevergone commentedDrupal 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! :)
Comment #139
BerdirUpdated http://drupal.org/node/1400186, this will still need a separate change record for the user_save() and user hook changes, see #128.
Comment #140
BerdirOk, here we go: http://drupal.org/node/1554986
No example yet for the hooks.
Comment #141
aspilicious CreditAttribution: aspilicious commentedAdded hook example. We are done now :)
Comment #142
aspilicious CreditAttribution: aspilicious commentedComment #144
tim.plunketthttp://drupal.org/node/1554986 is the change notice for this issue, but it doesn't mention that user_save was removed completely.
Comment #145
BerdirThe 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.
Comment #146
tim.plunkettI split these change notices up, it was too confusing to have them combined. See http://drupal.org/node/1688430
Comment #148
benjifisherIn case anyone is trying to track down the right commit, it is here: 93c20fd. Unless there is more than one ...