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

Files: 
CommentFileSizeAuthor
#96 user-ng-1818570-96.patch110.78 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 55,002 pass(es). View
#96 interdiff.txt988 byteseffulgentsia
#93 user-ng-1818570-93.patch109.23 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 56,602 pass(es), 6 fail(s), and 105 exception(s). View
#94 user-ng-1818570-94.patch109.66 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 56,765 pass(es), 6 fail(s), and 105 exception(s). View
#94 interdiff.txt1.11 KBeffulgentsia
#89 user-ng-1818570-89.patch109.63 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 56,051 pass(es). View
#89 user-ng-1818570-89-interdiff.txt508 bytesBerdir
#87 user-ng-1818570-87.patch109.58 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 57,522 pass(es), 4 fail(s), and 4 exception(s). View
#87 user-ng-1818570-87-interdiff.txt2.32 KBBerdir
#82 interdiff.txt695 bytesandypost
#82 user-ng-1818570-82.patch110.86 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-ng-1818570-82.patch. Unable to apply patch. See the log in the details link for more information. View
#75 user-ng-1818570-75.patch110.14 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 55,806 pass(es), 0 fail(s), and 5 exception(s). View
#75 user-ng-1818570-75-interdiff.txt2.09 KBBerdir
#73 user-ng-1818570-73.patch110.01 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 55,853 pass(es), 7 fail(s), and 4 exception(s). View
#73 user-ng-1818570-73-interdiff.txt7.19 KBBerdir
#67 user-ng-1818570-67.patch108.37 KBdas-peter
PASSED: [[SimpleTest]]: [MySQL] 55,720 pass(es). View
#67 user-ng-1818570-66-67-interdiff-do-not-test.diff5.59 KBdas-peter
#66 user-ng-1818570-66.patch108.38 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 55,858 pass(es). View
#66 user-ng-1818570-66-interdiff.txt1.63 KBBerdir
#64 user-ng-1818570-64.patch107.59 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 55,993 pass(es), 1 fail(s), and 16 exception(s). View
#62 user-ng-1818570-62.patch110.7 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-ng-1818570-62_2.patch. Unable to apply patch. See the log in the details link for more information. View
#62 user-ng-1818570-62-interdiff.txt4.5 KBBerdir
#61 user-ng-1818570-61.patch109.58 KBdas-peter
PASSED: [[SimpleTest]]: [MySQL] 55,708 pass(es). View
#61 interdiff-user-ng-1818570-58-61-do-not-test.diff760 bytesdas-peter
#58 user-ng-1818570-58.patch108.83 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 55,603 pass(es), 0 fail(s), and 18 exception(s). View
#58 user-ng-1818570-58-interdiff.txt898 bytesBerdir
#56 user-ng-1818570-56.patch108.82 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 56,217 pass(es), 43 fail(s), and 4,589 exception(s). View
#56 user-ng-1818570-56-interdiff.txt7.68 KBBerdir
#52 user-ng-phpunit-1818570-52.patch106.86 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 55,594 pass(es), 3 fail(s), and 0 exception(s). View
#52 user-ng-phpunit-1818570-52-interdiff.txt1 KBBerdir
#50 user-ng-phpunit-1818570-50.patch105.86 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 55,292 pass(es), 118 fail(s), and 3,236 exception(s). View
#50 user-ng-phpunit-1818570-50-interdiff.txt2.89 KBBerdir
#48 user-ng-phpunit-1818570-48.patch103.94 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#48 user-ng-phpunit-1818570-48-interdiff.txt1 KBBerdir
#46 user-ng-context-1818570-46.patch102.94 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
#46 user-ng-context-1818570-46-interdiff.txt592 bytesBerdir
#43 user-ng-context-1818570-43.patch103.51 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#43 user-ng-context-1818570-43-interdiff.txt2.93 KBBerdir
#41 user-ng-slightly-simplified-1818570-41.patch100.58 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 55,376 pass(es), 0 fail(s), and 1 exception(s). View
#41 user-ng-slightly-simplified-1818570-41-interdiff.txt7.11 KBBerdir
#40 user-ng-interface-1818570-40.patch105.01 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 55,563 pass(es). View
#40 user-ng-interface-1818570-40-interdiff.txt1.6 KBBerdir
#38 user-ng-interface-1818570-37.patch104.45 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 55,462 pass(es), 8 fail(s), and 0 exception(s). View
#38 user-ng-interface-1818570-37-interdiff.txt2.66 KBBerdir
#35 user-ng-interface-1818570-35.patch102.64 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 55,383 pass(es), 4 fail(s), and 1 exception(s). View
#35 user-ng-interface-1818570-35-interdiff.txt35.88 KBBerdir
#33 user-ng-1818570-33.patch74.53 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 54,637 pass(es), 550 fail(s), and 63,173 exception(s). View
#33 user-ng-1818570-33-interdiff.txt9.99 KBBerdir
#29 user-ng-1818570-29.patch74.67 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#25 user-ng-1818570-25.patch74.18 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-ng-1818570-25.patch. Unable to apply patch. See the log in the details link for more information. View
#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
FAILED: [[SimpleTest]]: [MySQL] 54,178 pass(es), 2 fail(s), and 0 exception(s). View
#20 user-ng-1818570-20.patch67.59 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 53,910 pass(es), 3 fail(s), and 51 exception(s). View
#20 user-ng-1818570-20-interdiff.txt723 bytesBerdir
#19 user-ng-1818570-19.patch68.3 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#19 user-ng-1818570-19-interdiff.txt4.93 KBBerdir
#16 user-ng-more-role-fixes-1818570-16.patch70.12 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 53,276 pass(es), 50 fail(s), and 8 exception(s). View
#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
FAILED: [[SimpleTest]]: [MySQL] 53,108 pass(es), 91 fail(s), and 38 exception(s). View
#11 user-ng-roles-stuff-1818570-11.patch60.41 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 52,047 pass(es), 177 fail(s), and 38 exception(s). View
#11 user-ng-roles-stuff-1818570-11-interdiff.txt17.48 KBBerdir
#8 user-ng-more-test-fixes-1818570-8.patch44.97 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 52,982 pass(es), 199 fail(s), and 69 exception(s). View
#8 user-ng-more-test-fixes-1818570-8-interdiff.txt21.12 KBBerdir
#6 user-ng-some-fixes-1818570-6.patch25.53 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 52,575 pass(es), 364 fail(s), and 900 exception(s). View
#6 user-ng-some-fixes-1818570-6-interdiff.txt3.1 KBBerdir
#4 user-ng-lets-get-started-1818570-4.patch23.3 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 51,336 pass(es), 1,142 fail(s), and 2,307 exception(s). View

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
FAILED: [[SimpleTest]]: [MySQL] 51,336 pass(es), 1,142 fail(s), and 2,307 exception(s). View

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
FAILED: [[SimpleTest]]: [MySQL] 52,575 pass(es), 364 fail(s), and 900 exception(s). View

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
FAILED: [[SimpleTest]]: [MySQL] 52,982 pass(es), 199 fail(s), and 69 exception(s). View

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
FAILED: [[SimpleTest]]: [MySQL] 52,047 pass(es), 177 fail(s), and 38 exception(s). View

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
FAILED: [[SimpleTest]]: [MySQL] 53,108 pass(es), 91 fail(s), and 38 exception(s). View
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
FAILED: [[SimpleTest]]: [MySQL] 53,276 pass(es), 50 fail(s), and 8 exception(s). View

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
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

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

FileSize
723 bytes
67.59 KB
FAILED: [[SimpleTest]]: [MySQL] 53,910 pass(es), 3 fail(s), and 51 exception(s). View

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
FAILED: [[SimpleTest]]: [MySQL] 54,178 pass(es), 2 fail(s), and 0 exception(s). View
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
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-ng-1818570-25.patch. Unable to apply patch. See the log in the details link for more information. View

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
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

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
FAILED: [[SimpleTest]]: [MySQL] 54,637 pass(es), 550 fail(s), and 63,173 exception(s). View

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
FAILED: [[SimpleTest]]: [MySQL] 55,383 pass(es), 4 fail(s), and 1 exception(s). View

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
FAILED: [[SimpleTest]]: [MySQL] 55,462 pass(es), 8 fail(s), and 0 exception(s). View

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
PASSED: [[SimpleTest]]: [MySQL] 55,563 pass(es). View

This, then :)

Berdir’s picture

FileSize
7.11 KB
100.58 KB
FAILED: [[SimpleTest]]: [MySQL] 55,376 pass(es), 0 fail(s), and 1 exception(s). View

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
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

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
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

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
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

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
FAILED: [[SimpleTest]]: [MySQL] 55,292 pass(es), 118 fail(s), and 3,236 exception(s). View

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
FAILED: [[SimpleTest]]: [MySQL] 55,594 pass(es), 3 fail(s), and 0 exception(s). View

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
FAILED: [[SimpleTest]]: [MySQL] 56,217 pass(es), 43 fail(s), and 4,589 exception(s). View

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
FAILED: [[SimpleTest]]: [MySQL] 55,603 pass(es), 0 fail(s), and 18 exception(s). View

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
PASSED: [[SimpleTest]]: [MySQL] 55,708 pass(es). View

Attempt to fix the test fails.

Berdir’s picture

FileSize
4.5 KB
110.7 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-ng-1818570-62_2.patch. Unable to apply patch. See the log in the details link for more information. View

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
FAILED: [[SimpleTest]]: [MySQL] 55,993 pass(es), 1 fail(s), and 16 exception(s). View

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
PASSED: [[SimpleTest]]: [MySQL] 55,858 pass(es). View

This should fix those tests.

das-peter’s picture

FileSize
5.59 KB
108.37 KB
PASSED: [[SimpleTest]]: [MySQL] 55,720 pass(es). View
+++ 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
FAILED: [[SimpleTest]]: [MySQL] 55,853 pass(es), 7 fail(s), and 4 exception(s). View

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
FAILED: [[SimpleTest]]: [MySQL] 55,806 pass(es), 0 fail(s), and 5 exception(s). View

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
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-ng-1818570-82.patch. Unable to apply patch. See the log in the details link for more information. View
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
FAILED: [[SimpleTest]]: [MySQL] 57,522 pass(es), 4 fail(s), and 4 exception(s). View

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
PASSED: [[SimpleTest]]: [MySQL] 56,051 pass(es). View

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
FAILED: [[SimpleTest]]: [MySQL] 56,602 pass(es), 6 fail(s), and 105 exception(s). View
+++ 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
FAILED: [[SimpleTest]]: [MySQL] 56,765 pass(es), 6 fail(s), and 105 exception(s). View

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
PASSED: [[SimpleTest]]: [MySQL] 55,002 pass(es). View

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.