[meta] Complete conversion of users to Entity Field API

Starting with this.

Adding lots of methods, starting to convert some code...

Ups.

More weird things.

Upgrade path isn't using UserSession yet.

More fixes. The crazy things we put into theme('username')... dblogs?!

The re-roll is fine, the patch just changed uid to id() in all of core. I'm not sure if we want to do that in the initial patch, my idea was to just get the interface and classes in, with a few example conversions in e.g. user.module. Otherwise this patch is going to get insanely big.

Yes, I'm +1 on not replacing everything here/in the first step. The node issue was changed to a meta issue, maybe we should do this here too and then split it up per module or groups of modules? Then we could make novice issues out of probably and get them in in parallel. An alternative would be to split it up per field (uid, status, ...) but I think that has a much higher chance of conflicting.

This is a re-roll of #21.

A lot of additional methods on User, so shuffling things around a bit to get a clean diff there...

Not exactly sure yet what to do with user_form_name(), but for now, I had to add getUsername() and also convert a bunch of cases in node.module and elswhere that still passed non-user thingies into that theme function. That just doesn't fly anymore now that we rely on ->id(), it probably used the node id there or something like that...

#40: user-account-interface-2017207-40.patch queued for re-testing.

Tagging.

Untested attempt to fix those tests.

This should be better :)

That said, that should obviously be an EFQ query, but that's a different story...

Opened #2026347: Adding methods to UserInterface/AccountInterface and re-uploaded the same patch there, changing this to a meta issue.

To get this started, here is a first patch that converts ->uid to ->id() and some isAnonymous()/isAuthenticated().

First patch includes the refeferenced issue above to have it tested.

Better?

So in the first step of the installer, we don't even have a global user object? ... Interesting...

Needs to happen, so critical.

Fixed the roles test and the admin overview, just reverted the change there, there are other patches that clean that function up.

The other issue went in, re-roll and fixing the last test fails, assuming that I didn't make any mistakes.

Opened an issue for the ->uid changes: #2039199: Convert ->uid to ->id(), isAnonymous() and isAuthenticated(), please review and RTBC to get the next piece in.

In the meantime, similar to the node issue, here's a patch that converts more fields and also a version of the patch removes the BC decorator to see how well that's working already.

Edit: Fixed issue reference.

This should be better.

Re-roll and some test fixes after the ->uid patch got in.

Fixed some timezone and access issues.

Ah, found the major problem there, accidentally already had some BC stuff that I dropped in the wrong commit.

Another really stupid mistake ;)

- Fixed missing return for isActive()
- Fixed weird drupalLogin() code (yeah, we're testing this ~10 times, with 4x duplicated code)
- Converted the user password hashing test to DUBT. either that or a phpunit test with mocking, but that felt like a bigger change.

BC Removal interdiff is still trivial, the previous changes should have fixed a lot of those fails too.

Buh.

Another crazy bug in getSignature() of the bc decorator, this should be green, so the next question is, how to get it in/split it up. There are some non-trivial changes in there... I can provide patches per method in the order I have them in my commits, re-ordering would be a bit ugly...

* 021613b - Convert ->access/->timestamp to getLastAccessedTime()
* 8efcc26 - Convert ->timezone to methods
* 767a225 - fix password hashing test
* 09f09a0 - convert ->pass to getPassword()
* 3539cda - Converted user signature methods
* e961ecf - Change ->roles to getRoles()
* 470e34d - Replace ->name with methods
* b52d7bb - ->created to ->getCreatedTime()
* b2441c5 - ->login to getLastLoginTime()
* 106311f - Convert ->mail to getEmail()
* 532fdd0 - converted ->status to methods

Edit: order is inversed, bottom is first.

+++ b/core/includes/session.incundefined
@@ -113,23 +113,23 @@ function _drupal_session_read($sid) {
   elseif ($user) {
     // The user is anonymous or blocked. Only preserve two fields from the
     // {sessions} table.
     $account = drupal_anonymous_user();
-    $account->session = $user->session;
-    $account->timestamp = $user->timestamp;
+    $account->setSessionData($user->getSessionData());
+    $account->setLastAccessedTime($user->getLastAccessedTime());
     $user = $account;
   }
   else {
     // The session has expired.
     $user = drupal_anonymous_user();
-    $user->session = '';
+    $user->setSessionData(array());

I think we should replace drupal_anonoymous_user() with a new UserSession() here. The two set methods aren't part of the AccountInterface method, I had to add them to UserSesssion to be able to write the now protected properties. But drupal_anonymous() user is documented to return AccountInterface..

+++ b/core/includes/session.incundefined
@@ -177,7 +177,7 @@ function _drupal_session_write($sid, $value) {
-    if ($is_changed || !isset($user->timestamp) || REQUEST_TIME - $user->timestamp > settings()->get('session_write_interval', 180)) {
+    if ($is_changed || !$user->getLastAccessedTime() || REQUEST_TIME - $user->getLastAccessedTime() > settings()->get('session_write_interval', 180)) {

@@ -214,7 +214,7 @@ function _drupal_session_write($sid, $value) {
     // Likewise, do not update access time more than once per 180 seconds.
-    if ($user->isAuthenticated() && REQUEST_TIME - $user->access > settings()->get('session_write_interval', 180)) {
+    if ($user->isAuthenticated() && REQUEST_TIME - $user->getLastAccessedTime() > settings()->get('session_write_interval', 180)) {

It looks like global user has two different properties... ->timestamp and ->access, which are basically the same from what I understand, I replaced them with the same logic/method and it *seems* to work fine.

+++ b/core/lib/Drupal/Core/Password/PasswordInterface.phpundefined
@@ -32,13 +34,13 @@ public function hash($password);
-   * @return bolean.
+   * @return boolean.

That "." here is weird, already fixed the typo, will remove that too.

+++ b/core/lib/Drupal/Core/Session/AccountInterface.phpundefined
@@ -114,4 +114,30 @@ public function getPreferredAdminLangcode($default = NULL);
+  public function getEmail();
...
+  public function getTimeZone();
...
+  public function getLastAccessedTime();

These methods were moved from UserInterface to AccountInterface, as they reflect usage in our current code. We could alternatively also don't do this and require the code to do a user_load(), at least for timezone and e-mail, but the access is required.

+++ b/core/modules/system/lib/Drupal/system/Tests/System/PasswordHashingTest.phpundefined
@@ -7,13 +7,21 @@
-class PasswordHashingTest extends UnitTestBase {
+class PasswordHashingTest extends DrupalUnitTestBase {

There's a separate issue to make this a phpunit test, then we could use mocking, but previously, this passed in stdClass objects, so we need to use DUBT here.

+++ b/core/modules/system/system.moduleundefined
@@ -2413,9 +2414,9 @@ function system_form_user_register_form_alter(&$form, &$form_state) {
-function system_user_presave($account) {
+function system_user_presave(UserInterface $account) {
   $config = config('system.timezone');
-  if ($config->get('user.configurable') && empty($account->timezone) && !$config->get('user.default')) {
+  if ($config->get('user.configurable') && !$account->getTimeZone() && !$config->get('user.default')) {
     $account->timezone = $config->get('default');
   }

As pointed out before, system.module altering the user form is just crazy, we should move this in a follow-up issue.

+++ b/core/modules/user/lib/Drupal/user/Plugin/Action/BlockUser.phpundefined
@@ -27,11 +27,11 @@ class BlockUser extends ActionBase {
   public function execute($account = NULL) {
     // Skip blocking user if they are already blocked.
-    if ($account !== FALSE && $account->status->value == 1) {
+    if ($account !== FALSE && $account->isActive()) {

No idea why action plugins can receive NULL and why it has to do a type safe check to FALSE (which will actually never be TRUE as entity_load() now returns NULL.

Seems like a trick to use ExecutableInterface, which doesn't have any arguments?

+++ b/core/modules/user/lib/Drupal/user/Tests/UserCancelTest.phpundefined
@@ -50,10 +50,10 @@ function testUserCancelWithoutPermission() {
-    $this->drupalGet("user/" . $account->id() . "/cancel/confirm/$timestamp/" . user_pass_rehash($account->pass, $timestamp, $account->login));
+    $this->drupalGet("user/" . $account->id() . "/cancel/confirm/$timestamp/" . user_pass_rehash($account->pass, $timestamp, $account->getLastLoginTime()));

Oh, there's still a ->pass here, a few actually in these tests.

Ok, lots of additional method conversions that should fix most of the failing test with the NG patch, updating to use isAuthenticated() for the above, and various additional removed getBCEntity().

Let's see how this looks.

Posted a re-rolled method conversion patch in #2045275: Convert user properties to methods, please review.

Yay and kinda surprised at the NG patch being green. That's the one that also needs manual testing I think, and I'm quite sure that at least the overlay and contact settings are broken now, as the form controller doesn't set arbitrary stuff on the user objects anymore. So I think it makes sense to test and review that separately.

Ok, we're getting close here.

Re-rolled, removed UserBCDecorator class. Wrote tests for the overlay and contact user settings and fixed those form alters by switching to a form submit callback. There would be a fancier way using computed, defined fields based on user.data, but that's a much bigger change...

Ok, passed as expected.

What I think this needs now is a fair amount of manual testing. As visible in the missing test coverage, our tests aren't perfect, so it's quite possible that we're missing stuff.

So test this locally or on simplytest.me and then do stuff with users. Create, edit and make sure all settings, passwording changing and so on works, add fields, mass-operations, create views, that list users, stuff like that.

Thanks for testing. I can't reproduce that error, can you provide more specific steps on what you filled out where exactly and how your role selection looks like?

Thanks, got it. Very ugly bug related to serialization of the entity form controller, we didn't massage the role ids in case of a validation error and re-submit.

Tried, but wasn't able to reproduce the bug with a test so far. Somehow it doesn't break there.

Also removed the new useless clean form state function call, as we only save things which are defined fields anyway.

#87: user-ng-methods-2017207-87-bc-removal.patch queued for re-testing.

Fixed all Overrides that were part of the patch context, removed the incomplete test changes.

Removing sprint tag.