Now as the new entity field API got committed, we need to convert existing entity types to make use of it. See #1346214: [meta] Unified Entity Field API and the "Entity Field API" tag.

ToDo
#1634280-24: drupal_anonymous_user() should return a User entity

CommentFileSizeAuthor
#96 user-ng-1818570-96.patch110.78 KBeffulgentsia
#96 interdiff.txt988 byteseffulgentsia
#93 user-ng-1818570-93.patch109.23 KBeffulgentsia
#94 user-ng-1818570-94.patch109.66 KBeffulgentsia
#94 interdiff.txt1.11 KBeffulgentsia
#89 user-ng-1818570-89.patch109.63 KBBerdir
#89 user-ng-1818570-89-interdiff.txt508 bytesBerdir
#87 user-ng-1818570-87.patch109.58 KBBerdir
#87 user-ng-1818570-87-interdiff.txt2.32 KBBerdir
#82 interdiff.txt695 bytesandypost
#82 user-ng-1818570-82.patch110.86 KBandypost
#75 user-ng-1818570-75.patch110.14 KBBerdir
#75 user-ng-1818570-75-interdiff.txt2.09 KBBerdir
#73 user-ng-1818570-73.patch110.01 KBBerdir
#73 user-ng-1818570-73-interdiff.txt7.19 KBBerdir
#67 user-ng-1818570-67.patch108.37 KBdas-peter
#67 user-ng-1818570-66-67-interdiff-do-not-test.diff5.59 KBdas-peter
#66 user-ng-1818570-66.patch108.38 KBBerdir
#66 user-ng-1818570-66-interdiff.txt1.63 KBBerdir
#64 user-ng-1818570-64.patch107.59 KBBerdir
#62 user-ng-1818570-62.patch110.7 KBBerdir
#62 user-ng-1818570-62-interdiff.txt4.5 KBBerdir
#61 user-ng-1818570-61.patch109.58 KBdas-peter
#61 interdiff-user-ng-1818570-58-61-do-not-test.diff760 bytesdas-peter
#58 user-ng-1818570-58.patch108.83 KBBerdir
#58 user-ng-1818570-58-interdiff.txt898 bytesBerdir
#56 user-ng-1818570-56.patch108.82 KBBerdir
#56 user-ng-1818570-56-interdiff.txt7.68 KBBerdir
#52 user-ng-phpunit-1818570-52.patch106.86 KBBerdir
#52 user-ng-phpunit-1818570-52-interdiff.txt1 KBBerdir
#50 user-ng-phpunit-1818570-50.patch105.86 KBBerdir
#50 user-ng-phpunit-1818570-50-interdiff.txt2.89 KBBerdir
#48 user-ng-phpunit-1818570-48.patch103.94 KBBerdir
#48 user-ng-phpunit-1818570-48-interdiff.txt1 KBBerdir
#46 user-ng-context-1818570-46.patch102.94 KBBerdir
#46 user-ng-context-1818570-46-interdiff.txt592 bytesBerdir
#43 user-ng-context-1818570-43.patch103.51 KBBerdir
#43 user-ng-context-1818570-43-interdiff.txt2.93 KBBerdir
#41 user-ng-slightly-simplified-1818570-41.patch100.58 KBBerdir
#41 user-ng-slightly-simplified-1818570-41-interdiff.txt7.11 KBBerdir
#40 user-ng-interface-1818570-40.patch105.01 KBBerdir
#40 user-ng-interface-1818570-40-interdiff.txt1.6 KBBerdir
#38 user-ng-interface-1818570-37.patch104.45 KBBerdir
#38 user-ng-interface-1818570-37-interdiff.txt2.66 KBBerdir
#35 user-ng-interface-1818570-35.patch102.64 KBBerdir
#35 user-ng-interface-1818570-35-interdiff.txt35.88 KBBerdir
#33 user-ng-1818570-33.patch74.53 KBBerdir
#33 user-ng-1818570-33-interdiff.txt9.99 KBBerdir
#29 user-ng-1818570-29.patch74.67 KBandypost
#25 user-ng-1818570-25.patch74.18 KBBerdir
#25 user-ng-1818570-25-interdiff.txt3.51 KBBerdir
#22 user-ng-1818570-22-interdiff.txt5.27 KBBerdir
#22 user-ng-1818570-22.patch70.67 KBBerdir
#20 user-ng-1818570-20.patch67.59 KBBerdir
#20 user-ng-1818570-20-interdiff.txt723 bytesBerdir
#19 user-ng-1818570-19.patch68.3 KBBerdir
#19 user-ng-1818570-19-interdiff.txt4.93 KBBerdir
#16 user-ng-more-role-fixes-1818570-16.patch70.12 KBBerdir
#16 user-ng-more-role-fixes-1818570-16-interdiff.txt11.4 KBBerdir
#13 user-ng-node-access-hook-1818570-13-interdiff.txt2.27 KBBerdir
#13 user-ng-node-access-hook-1818570-13.patch59.5 KBBerdir
#11 user-ng-roles-stuff-1818570-11.patch60.41 KBBerdir
#11 user-ng-roles-stuff-1818570-11-interdiff.txt17.48 KBBerdir
#8 user-ng-more-test-fixes-1818570-8.patch44.97 KBBerdir
#8 user-ng-more-test-fixes-1818570-8-interdiff.txt21.12 KBBerdir
#6 user-ng-some-fixes-1818570-6.patch25.53 KBBerdir
#6 user-ng-some-fixes-1818570-6-interdiff.txt3.1 KBBerdir
#4 user-ng-lets-get-started-1818570-4.patch23.3 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

Assigned: Unassigned » chx
chx’s picture

Assigned: chx » Unassigned

GLOBALS['user']. Nope. This is too big for me. I can take this back on but need guidance on what to do with that. Alternatively, someone else who knows what to do with it can do it.

Berdir’s picture

Assigned: Unassigned » Berdir

Working a bit on this.

Berdir’s picture

Status: Active » Needs review
FileSize
23.3 KB

This will probably not get through the installer but I have quite a few tests passing.

Had to add a few hacks, drupal_anonyomous_user() now returns a User entity that's now too early for something like that. Using a stdClass for global $user (it was actually even messier than before because the loaded user was a stdClass but the anon global user object is User.

Status: Needs review » Needs work

The last submitted patch, user-ng-lets-get-started-1818570-4.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
3.1 KB
25.53 KB

This should fix some of the failures.

Status: Needs review » Needs work

The last submitted patch, user-ng-some-fixes-1818570-6.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
21.12 KB
44.97 KB

Ok, this should fix a lot of tests. Some parts of this are a bit ugly, e.g. due to User type hints but I really think we shouldn't replace them with EntityInterface, at least not everywhere where they are access related. We really need to replace that with something else :(

Berdir’s picture

Assigned: Berdir » Unassigned

I'm done for the day. :)

Status: Needs review » Needs work

The last submitted patch, user-ng-more-test-fixes-1818570-8.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
17.48 KB
60.41 KB

This should fix a lot of user role related tests, still not everything 100% correct there.

Status: Needs review » Needs work

The last submitted patch, user-ng-roles-stuff-1818570-11.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
59.5 KB
2.27 KB

That node access fix was wrong.

Status: Needs review » Needs work

The last submitted patch, user-ng-node-access-hook-1818570-13.patch, failed testing.

Berdir’s picture

For anyone reading here, I'm working on a UserSessionInterface that both the global $user object and User would implement, that should make a lot of the changes here less hackish: #1825332: Introduce an AccountInterface to represent the current user

Berdir’s picture

Status: Needs work » Needs review
FileSize
11.4 KB
70.12 KB

Some more user role fixes.

Status: Needs review » Needs work

The last submitted patch, user-ng-more-role-fixes-1818570-16.patch, failed testing.

Berdir’s picture

Issue tags: +sprint
Berdir’s picture

Status: Needs work » Needs review
FileSize
4.93 KB
68.3 KB

Fixed the permission page default values and reverted drupal_anonoymous_user() return to an stdClass, removing the session.inc changes. I really need #1825332: Introduce an AccountInterface to represent the current user :(

This should fix most of the remaining fails.

Berdir’s picture

Removed debug message.

Status: Needs review » Needs work

The last submitted patch, user-ng-1818570-20.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
70.67 KB
5.27 KB

Ok, fixed most of the remaining test fails. User registration is related to the array(NULL) bug I think, for which there already is an issue. Also removed some left-over debug()'s.

Status: Needs review » Needs work

The last submitted patch, user-ng-1818570-22.patch, failed testing.

Berdir’s picture

The mentioned issue is #1957888: Exception when a field is empty on programmatic creation, need to check if that fixes the registration fails.

EDIT: Yes, can confirm that this fixes the test fail.

Berdir’s picture

Status: Needs work » Needs review
FileSize
3.51 KB
74.18 KB

Same fix as in the node type issue, needs a clear cache as something is now triggering a field info cache build before it's changed. Also added the patch from the linked issue so that I can see this getting green :)

Gábor Hojtsy’s picture

Note that #1498664: Refactor user entity properties to multilingual is currently postponed on this one. Tagging for D8MI.

Berdir’s picture

Status: Needs review » Needs work
Issue tags: +D8MI, +sprint, +language-content, +Entity Field API, +language-content-property

The last submitted patch, user-ng-1818570-25.patch, failed testing.

andypost’s picture

FileSize
74.67 KB

just a merge about current head

andypost’s picture

Status: Needs work » Needs review
+++ b/core/includes/bootstrap.incundefined
@@ -2002,14 +2003,13 @@ drupal_anonymous_user() {
-  $values = array(
+  return (object) array(
...
-  return new User($values, 'user');

This still waits for #1825332: Introduce an AccountInterface to represent the current user

Status: Needs review » Needs work

The last submitted patch, user-ng-1818570-29.patch, failed testing.

effulgentsia’s picture

Priority: Normal » Major

Raising to major because blocking #1983554: Remove BC-mode from EntityNG . Given that plus #26, let's move forward with this independent of #1825332: Introduce an AccountInterface to represent the current user if possible, and clean up hacks in a follow up. If that's not possible, please mark this issue postponed, and update the issue summary to explain why that issue is a blocker.

Berdir’s picture

Status: Needs work » Needs review
FileSize
9.99 KB
74.53 KB

Re-roll, getOriginalEntity() is now getNGEntity().

@effulgentsia: I'm... not sure. I can make this patch pass the tests without the other issue, but I'm not if it can get in like that, there are very ugly cases in regards to some type hints and e.g. the roles handling. The type hints can now probably be solved by changing them to UserInterface and have a subclassed UserBCDecorator that implements UserInterface as well.

Status: Needs review » Needs work

The last submitted patch, user-ng-1818570-33.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
35.88 KB
102.64 KB

Yes, the interface allows me to do something like this. Defined UserBCDecorator that extends EntityBCDecorator and implements UserInterface, changing most type hints of User to UserInterface. Let's see how the testbot likes this.

If this works then I should be able to revert quite a few getBC/NGEntity() calls as I no longer need to sneak around type hints anymore.

Status: Needs review » Needs work

The last submitted patch, user-ng-interface-1818570-35.patch, failed testing.

ParisLiakos’s picture

the forum fail is the same as on #1957888: Exception when a field is empty on programmatic creation
it started happening when forum views integration went in

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.66 KB
104.45 KB

This should be green again. Not sure why the forum integration test suddenly starts to fail here but the fix makes sense anyway.

Status: Needs review » Needs work

The last submitted patch, user-ng-interface-1818570-37.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.6 KB
105.01 KB

This, then :)

Berdir’s picture

Removed some ng/bc switches and other unecessary changes with the interface now. Let's see if this still passes.

Status: Needs review » Needs work

The last submitted patch, user-ng-slightly-simplified-1818570-41.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.93 KB
103.51 KB

This should fix the context test.

Berdir’s picture

There's an unrelated hunk in there, will remove that in the next re-roll that will be required sooner or later anyway I guess ;)

Status: Needs review » Needs work

The last submitted patch, user-ng-context-1818570-43.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
592 bytes
102.94 KB

Looks like that wasn't a good idea.

Status: Needs review » Needs work

The last submitted patch, user-ng-context-1818570-46.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1 KB
103.94 KB

Hm, this is a problem.

Figured out that failure, opened #1986528: Fatal error: Call to undefined method PHPUnit_Framework_Warning::getInfo() when there is an error in tests.

But that just reveals the real problem here, as visible the fatal errors in the previous patch. As User is EntityNG, which is not yet using injected dependencies and calls Drupal::entityManager(), among many other things that will then follow. i don't really see a way to fix that other than #1825332: Introduce an AccountInterface to represent the current user and use UserSession instead of User for those tests.

Status: Needs review » Needs work

The last submitted patch, user-ng-phpunit-1818570-48.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.89 KB
105.86 KB

Ah stupid me. Let's just use stdClass for those tests, that's what they are working with in case of global user anyway. We can switch to the AccountInterface/UserSessionInterface when we have it.

Status: Needs review » Needs work

The last submitted patch, user-ng-phpunit-1818570-50.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1 KB
106.86 KB

Replace a new User type hint with UserInterface.

Status: Needs review » Needs work

The last submitted patch, user-ng-phpunit-1818570-52.patch, failed testing.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Entity/Entity.phpundefined
@@ -257,7 +257,7 @@ public function getIterator() {
+  public function access($operation = 'view', \Drupal\user\UserInterface $account = NULL) {

Why don't we add the UserInterface in the use statement?

+++ b/core/modules/contact/lib/Drupal/contact/MessageFormController.phpundefined
--- a/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceItemTest.php
+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceItemTest.phpundefined

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceItemTest.phpundefined
@@ -97,5 +97,12 @@ public function testEntityReferenceItem() {
+    // Delete terms so we have nothing to reference and try again
+    $term->delete();
+    $term2->delete();
+    $entity = entity_create('entity_test', array('name' => $this->randomName()));
+    $entity->save();

This feels out of scope.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/FieldAccessTest.phpundefined
@@ -70,12 +70,12 @@ function testFieldAccess() {
+    $this->assertTrue($entity->field_test_text->access('view', $account->getNGEntity()), 'Access to the field was granted.');

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Entity/User.phpundefined
@@ -142,40 +143,91 @@ class User extends Entity implements UserInterface {
+  protected function init() {

Just in general I'm wondering whether could use get_object_vars to make this generic.

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Entity/User.phpundefined
@@ -142,40 +143,91 @@ class User extends Entity implements UserInterface {
+  public function getBCEntity() {

Needs @inheritdoc

+++ b/core/modules/user/lib/Drupal/user/Tests/UserTranslationUITest.phpundefined
@@ -39,6 +39,8 @@ function setUp() {
+    entity_get_controller('user')->resetCache();

Not important: This could also use the container to fetch the manager ...

+++ b/core/modules/user/lib/Drupal/user/UserStorageController.phpundefined
@@ -52,19 +52,25 @@ public function create(array $values) {
+    if ($entity->original instanceof \Drupal\Core\Entity\EntityBCDecorator) {

It would be also possible to use a "use" on this class?

+++ b/core/modules/user/lib/Drupal/user/UserStorageController.phpundefined
@@ -111,28 +112,28 @@ protected function postSave(EntityInterface $entity, $update) {
+        drupal_session_destroy_uid($entity->id());
+        if ($entity->uid->value == $GLOBALS['user']->uid) {

Wouldn't it be better use $entity->id() on both lines?

+++ b/core/modules/user/lib/Drupal/user/UserStorageController.phpundefined
@@ -111,28 +112,28 @@ protected function postSave(EntityInterface $entity, $update) {
       // Remove roles that are no longer enabled for the user.
-      $entity->roles = array_filter($entity->roles);
+      //$entity->roles = array_filter($entity->roles);

What happened here?

+++ b/core/modules/user/lib/Drupal/user/UserStorageController.phpundefined
@@ -177,4 +178,123 @@ protected function postDelete($entities) {
+   * Overrides \Drupal\Core\Entity\DataBaseStorageControllerNG::invokeHook().
...
+   * Overrides \Drupal\Core\Entity\DataBaseStorageControllerNG::baseFieldDefinitions().

These could use @inheritdoc

+++ b/core/modules/user/lib/Drupal/user/UserStorageController.phpundefined
@@ -177,4 +178,123 @@ protected function postDelete($entities) {
+    // @todo: field_attach_delete_revision() is named the wrong way round,
+    // consider renaming it.
+    if ($function == 'field_attach_revision_delete') {
+      $function = 'field_attach_delete_revision';
+    }

Just wondering why this fix is not part of DatabaseStorageController?

+++ b/core/modules/user/lib/Drupal/user/UserStorageController.phpundefined
@@ -177,4 +178,123 @@ protected function postDelete($entities) {
+    module_invoke_all($this->entityType . '_' . $hook, $entity->getBCEntity());
...
+    module_invoke_all('entity_' . $hook, $entity->getBCEntity(), $this->entityType);

module_invoke_all should be Drupal::moduleHandler()->invokeAll()

+++ b/core/modules/user/user.moduleundefined
@@ -2135,12 +2147,11 @@ function user_multiple_role_edit($accounts, $operation, $rid) {
+          $account->roles[count($account->roles)] = $rid;

Wouldn't it also just be possible to use ->roles[] = $rid?

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/access/AccessPluginBase.phpundefined
@@ -56,7 +56,7 @@ public function summaryTitle() {
+   * @param Drupal\user\UserInterface $account

Just to note: The method doesn't have type hinting yet.

+++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.phpundefined
index f171580..c0d383c 100644
--- a/core/tests/Drupal/Tests/Core/Route/RoleAccessCheckTest.php

It's good to see that this is easier to read.

Berdir’s picture

Thanks for the review.

- The missing use is because it was like that before, will change it.
- "This feels out of scope." These are tests of #1957888: Exception when a field is empty on programmatic creation, which I merged into this issue. that issue is currently blocking this anyway.
- Looks like one of the review blocks ("$this->assertTrue($entity->field_test_text->access('view', $account->getNGEntity()), 'Access to the field was granted.');"), is cut of, has no comment below it. The getNGEntity() part should no longer be necessary there.
- We might remove init() as we are switching to interfaces where the public properties on he class are useless anyway. Until then, I'd like to keep this consistent with other conversions.
- The array_filter() can be removed, that happens in the form controller now. Will remove it.
- The field_attach_revision_delete stuff is there because we duplicate the whole method so that we can pass the BC decorator to the hook implementations.
- "Wouldn't it also just be possible to use ->roles[] = $rid?" Right now it's not because the Field class wants a numerical index. But I agree that it should be possible, just felt out of scope to change it here.

Berdir’s picture

Status: Needs work » Needs review
FileSize
7.68 KB
108.82 KB

Fixed the last test fail and the feedback from @dawehner as far as possible :)

Status: Needs review » Needs work

The last submitted patch, user-ng-1818570-56.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
898 bytes
108.83 KB

Ok, invokeAll() wants an array... We should then actually type hint that?

Status: Needs review » Needs work

The last submitted patch, user-ng-1818570-58.patch, failed testing.

das-peter’s picture

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUninstallTest.php
@@ -62,6 +62,12 @@ function testUninstallProcess() {
+    // @todo: If the global user is an EntityBCDecorator, getting the roles
+    // roles from it within LocaleLookup results in a loop that invokes

"getting the roles roles from", I guess we can remove one "roles".

+++ b/core/modules/search/lib/Drupal/search/Tests/SearchCommentTest.php
@@ -125,9 +125,7 @@ function testSearchResultsComment() {
-    $this->admin_role = key($this->admin_role);
+    $this->admin_role = $this->admin_user->roles[0];

Relying on an "generic" index to fetch a certain roles is a bit odd. Before the conversion "roles" was an associative array, and thus more reliable. Similar stuff happens in other places too.

+++ b/core/modules/user/user.module
@@ -2099,12 +2111,11 @@ function user_multiple_role_edit($accounts, $operation, $rid) {
-          $account->roles = $roles;
+          $account->roles[count($account->roles)] = $rid;

Is there a reason why $account->roles[] = $rid; shouldn't work?

Further I've found some legacy code:

  • In hook_node_grants_alter():
    if (isset($account->roles[$role_id])) {
  • In _drupal_session_read():
    $user->roles[DRUPAL_AUTHENTICATED_RID] = DRUPAL_AUTHENTICATED_RID;
das-peter’s picture

Status: Needs work » Needs review
FileSize
760 bytes
109.58 KB

Attempt to fix the test fails.

Berdir’s picture

Re-roll after the clean values issue went in. Some small clean-up based on the review from @das-peter. Did not yet change the [0] thing.

Status: Needs review » Needs work

The last submitted patch, user-ng-1818570-62.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
107.59 KB

Messed up the re-roll.

Status: Needs review » Needs work

The last submitted patch, user-ng-1818570-64.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.63 KB
108.38 KB

This should fix those tests.

das-peter’s picture

+++ b/core/includes/bootstrap.inc
@@ -1843,14 +1844,13 @@ function drupal_set_title($title = NULL, $output = CHECK_PLAIN) {
     'roles' => array(
       DRUPAL_ANONYMOUS_RID => DRUPAL_ANONYMOUS_RID,
     ),

I think we should use the numeric index here too - just to prevent wrong expectations.

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/PluginTestBase.php
@@ -74,7 +74,7 @@ public function setUp() {
+          'user' => array('class' => 'Drupal\user\\UserInterface')

@@ -88,7 +88,7 @@ public function setUp() {
+          'user' => array('class' => 'Drupal\user\\UserInterface'),

Two backslashes doesn't look right.

+++ b/core/modules/user/lib/Drupal/user/UserAccessController.php
@@ -32,14 +32,14 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, U
+        return (($account->uid == $entity->uid->value) || user_access('administer users', $account)) && $entity->uid->value > 0;
...
+        return ((($account->uid == $entity->uid->value) && user_access('cancel account', $account)) || user_access('administer users', $account)) && $entity->uid->value > 0;

For the sake of consistency changed to ->id()

+++ b/core/modules/user/user.module
@@ -2102,12 +2114,11 @@ function user_multiple_role_edit($accounts, $operation, $rid) {
+        if ($account !== FALSE && !array_search($rid, $account->roles)) {

@@ -2116,8 +2127,8 @@ function user_multiple_role_edit($accounts, $operation, $rid) {
+        if ($account !== FALSE && $index = array_search($rid, $account->roles)) {

This looks odd, $index is never used and the return of array_search() is compared untyped.

Let's see if the adjusted patch works.

Status: Needs review » Needs work
Issue tags: -D8MI, -sprint, -language-content, -Entity Field API, -language-content-property

The last submitted patch, user-ng-1818570-67.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +D8MI, +sprint, +language-content, +Entity Field API, +language-content-property

#67: user-ng-1818570-67.patch queued for re-testing.

Berdir’s picture

Issue tags: +Needs manual testing

To test this manually, apply the patch and then verify that fields can be added to users and are correctly displayed, check different field types, e.g. images, lists, references. Make sure that you can then edit and create users and the values are correctly saved, displayed.

Make sure roles operations work, e.g. removing and re-adding roles and also additional settings like the overlay setting.

fago’s picture

I carefully reviewed the patch and I must say this is already great! Imo this is close to being ready, just a few remarks:

+++ b/core/modules/user/lib/Drupal/user/AccountFormController.php
@@ -225,6 +225,19 @@ public function form(array $form, array &$form_state) {
+    // Change the roles array to a list of enabled roles.
+    // @todo: Move this to an value callback on the form element.
+    if (empty($this->roles_filtered)) {
+      $form_state['values']['roles'] = array_keys(array_filter($form_state['values']['roles']));
+      $this->roles_filtered = TRUE;

The regular workflow is to extract from form values and write to an entity field here. So this is what it should do here instead of changing the form values. roles_filtered should go away then also.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/access/AccessPluginBase.php
@@ -56,7 +56,7 @@ public function summaryTitle() {
-   * @param Drupal\user\User $account
+   * @param Drupal\user\UserInterface $account

Misses leading \

fago’s picture

Status: Needs review » Needs work
Berdir’s picture

Status: Needs work » Needs review
FileSize
7.19 KB
110.01 KB

Re-roll.

- As discussed, we can't re-assign it to the entity as it blows up on the initial assignment. Changed it so that we use a copy of $form_state, which is a tiny bit less ugly
- Added the missing \
- Update the @var things. As discussed, we'll remove them when we add methods on UserInterface, I will already do that for file but it doesn't help much here anyway.

Status: Needs review » Needs work

The last submitted patch, user-ng-1818570-73.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.09 KB
110.14 KB

Well, that doesn't work, because it looks like we need the form state changes.... Also reverted an accidental change in the access tests during re-roll.

fago’s picture

Status: Needs review » Reviewed & tested by the community

I see, so that's the best option we have right now. We can work on cleaning this up in a follow-up while moving it to form-NG.

I think this is good to go then!

fago’s picture

Issue tags: -Needs manual testing

To test this manually, apply the patch and then verify that fields can be added to users and are correctly displayed, check different field types, e.g. images, lists, references. Make sure that you can then edit and create users and the values are correctly saved, displayed.

Make sure roles operations work, e.g. removing and re-adding roles and also additional settings like the overlay setting.

Went through that manually as well, everything worked for me.

fago’s picture

#75: user-ng-1818570-75.patch queued for re-testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +needs profiling

Lets get some profiling done on this... viewing a page, viewing a list of users using views etc...

fago’s picture

Performance will be worse, e.g. NodeNG has been 25% worse. I assume it will be about the same for this one, but let's measure.

However, as discussed during the node conversion lots of that regression comes from the use of the currently enabled BC-mode here, so we are profiling something that will go away again. So any further performance work is going to have to wait until after we drop the BC mode. See #1983554: Remove BC-mode from EntityNG

Related node discussion:
http://drupal.org/node/1818556#comment-7160984

fago’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -needs profiling

ok, I've added a couple of fields to users, made a view that renders users and added 10 users that are shown on this page. With the patch applied page generation time is ~23% slower. As said, that's partly due to the BC-mode - see comment above.

andypost’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
110.86 KB
695 bytes

Fixed broken tests, seems left from node conversion

Status: Needs review » Needs work
Issue tags: -sprint, -language-content, -language-content-property

The last submitted patch, user-ng-1818570-82.patch, failed testing.

The last submitted patch, user-ng-1818570-84.patch, failed testing.

Berdir’s picture

Re-adding tags.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.32 KB
109.58 KB

Moving the original check back into the user storage controller for now.

Status: Needs review » Needs work

The last submitted patch, user-ng-1818570-87.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
508 bytes
109.63 KB

Forgot the use, this should pass again.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC, there's only merge changes

fago’s picture

#89: user-ng-1818570-89.patch queued for re-testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Some minor nits... plus I'm also wondering why we chose to change the structure of the roles array stored on the user here? Was it necessary for the conversion?

/**
 * Generates a default anonymous $user object.
 *
 * @return Drupal\user\Plugin\Core\Entity\User
 *   The user object.
 */
function drupal_anonymous_user() {

Need to fix up the return value here as it no longer returns a User object...

+++ b/core/modules/user/user.api.phpundefined
@@ -359,7 +359,7 @@ function hook_user_logout($account) {
-function hook_user_view(\Drupal\user\Plugin\Core\Entity\User $account, \Drupal\entity\Plugin\Core\Entity\EntityDisplay $display, $view_mode, $langcode) {
+function hook_user_view(\Drupal\user\UserInterface $account, \Drupal\entity\Plugin\Core\Entity\EntityDisplay $display, $view_mode, $langcode) {

+++ b/core/modules/user/user.moduleundefined
@@ -601,7 +610,7 @@ function user_search_execute($keys = NULL, $conditions = NULL) {
-function user_user_view(User $account, EntityDisplay $display) {
+function user_user_view(EntityInterface $account, EntityDisplay $display) {

The documented implementation seems to suggest the type hinting in user_user_view should be different...

effulgentsia’s picture

FileSize
109.23 KB
+++ b/core/modules/contact/lib/Drupal/contact/MessageFormController.php
@@ -64,7 +64,7 @@ public function form(array $form, array &$form_state) {
-    if ($message->recipient instanceof User) {
+    if ($message->recipient instanceof UserInterface) {

This hunk no longer applies due to #1856556: Convert user contact form into a contact category/bundle. This patch is just a straight reroll that removes this hunk, because the new check in HEAD is $message->isPersonal() which needs no change by this issue.

I'm also wondering why we chose to change the structure of the roles array stored on the user here

Because in EntityNG, all properties/fields are expected to follow the Field API data model, where multivalued fields are numerically indexed.

I'll post the fixes for #92 shortly.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
1.11 KB
109.66 KB

Fixes for #92.

Status: Needs review » Needs work

The last submitted patch, user-ng-1818570-94.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
988 bytes
110.78 KB

Status: Needs review » Needs work
Issue tags: -D8MI, -sprint, -language-content, -Entity Field API, -language-content-property

The last submitted patch, user-ng-1818570-96.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
Issue tags: +D8MI, +sprint, +language-content, +Entity Field API, +language-content-property

#96: user-ng-1818570-96.patch queued for re-testing.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Re-roll/updates look good, back RTBC.

alexpott’s picture

Title: Convert users to the new Entity Field API » Change notice: Convert users to the new Entity Field API
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed 88e6a76 and pushed to 8.x. Thanks!

Berdir’s picture

Title: Change notice: Convert users to the new Entity Field API » Convert users to the new Entity Field API
Status: Active » Fixed
Issue tags: -Needs change record

Added to https://drupal.org/node/1806650, which will need more work, but we can complete that once all entities are converted.

Berdir’s picture

Issue tags: -sprint

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.