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.
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
Comment | File | Size | Author |
---|---|---|---|
#96 | user-ng-1818570-96.patch | 110.78 KB | effulgentsia |
#96 | interdiff.txt | 988 bytes | effulgentsia |
#93 | user-ng-1818570-93.patch | 109.23 KB | effulgentsia |
#94 | user-ng-1818570-94.patch | 109.66 KB | effulgentsia |
#94 | interdiff.txt | 1.11 KB | effulgentsia |
Comments
Comment #1
chx CreditAttribution: chx commentedComment #2
chx CreditAttribution: chx commentedGLOBALS['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.
Comment #3
BerdirWorking a bit on this.
Comment #4
BerdirThis 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.
Comment #6
BerdirThis should fix some of the failures.
Comment #8
BerdirOk, 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 :(
Comment #9
BerdirI'm done for the day. :)
Comment #11
BerdirThis should fix a lot of user role related tests, still not everything 100% correct there.
Comment #13
BerdirThat node access fix was wrong.
Comment #15
BerdirFor 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
Comment #16
BerdirSome more user role fixes.
Comment #18
BerdirComment #19
BerdirFixed 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.
Comment #20
BerdirRemoved debug message.
Comment #22
BerdirOk, 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.
Comment #24
BerdirThe 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.
Comment #25
BerdirSame 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 :)
Comment #26
Gábor HojtsyNote that #1498664: Refactor user entity properties to multilingual is currently postponed on this one. Tagging for D8MI.
Comment #27
Berdir#25: user-ng-1818570-25.patch queued for re-testing.
Comment #29
andypostjust a merge about current head
Comment #30
andypostThis still waits for #1825332: Introduce an AccountInterface to represent the current user
Comment #32
effulgentsia CreditAttribution: effulgentsia commentedRaising 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.
Comment #33
BerdirRe-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.
Comment #35
BerdirYes, 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.
Comment #37
ParisLiakos CreditAttribution: ParisLiakos commentedthe 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
Comment #38
BerdirThis should be green again. Not sure why the forum integration test suddenly starts to fail here but the fix makes sense anyway.
Comment #40
BerdirThis, then :)
Comment #41
BerdirRemoved some ng/bc switches and other unecessary changes with the interface now. Let's see if this still passes.
Comment #43
BerdirThis should fix the context test.
Comment #44
BerdirThere's an unrelated hunk in there, will remove that in the next re-roll that will be required sooner or later anyway I guess ;)
Comment #46
BerdirLooks like that wasn't a good idea.
Comment #48
BerdirHm, 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.
Comment #50
BerdirAh 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.
Comment #52
BerdirReplace a new User type hint with UserInterface.
Comment #54
dawehnerWhy don't we add the UserInterface in the use statement?
This feels out of scope.
Just in general I'm wondering whether could use get_object_vars to make this generic.
Needs @inheritdoc
Not important: This could also use the container to fetch the manager ...
It would be also possible to use a "use" on this class?
Wouldn't it be better use $entity->id() on both lines?
What happened here?
These could use @inheritdoc
Just wondering why this fix is not part of DatabaseStorageController?
module_invoke_all should be Drupal::moduleHandler()->invokeAll()
Wouldn't it also just be possible to use ->roles[] = $rid?
Just to note: The method doesn't have type hinting yet.
It's good to see that this is easier to read.
Comment #55
BerdirThanks 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.
Comment #56
BerdirFixed the last test fail and the feedback from @dawehner as far as possible :)
Comment #58
BerdirOk, invokeAll() wants an array... We should then actually type hint that?
Comment #60
das-peter CreditAttribution: das-peter commented"getting the roles roles from", I guess we can remove one "roles".
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.
Is there a reason why
$account->roles[] = $rid;
shouldn't work?Further I've found some legacy code:
hook_node_grants_alter()
:if (isset($account->roles[$role_id])) {
_drupal_session_read()
:$user->roles[DRUPAL_AUTHENTICATED_RID] = DRUPAL_AUTHENTICATED_RID;
Comment #61
das-peter CreditAttribution: das-peter commentedAttempt to fix the test fails.
Comment #62
BerdirRe-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.
Comment #64
BerdirMessed up the re-roll.
Comment #66
BerdirThis should fix those tests.
Comment #67
das-peter CreditAttribution: das-peter commentedI think we should use the numeric index here too - just to prevent wrong expectations.
Two backslashes doesn't look right.
For the sake of consistency changed to
->id()
This looks odd,
$index
is never used and the return ofarray_search()
is compared untyped.Let's see if the adjusted patch works.
Comment #69
Berdir#67: user-ng-1818570-67.patch queued for re-testing.
Comment #70
BerdirTo 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.
Comment #71
fagoI carefully reviewed the patch and I must say this is already great! Imo this is close to being ready, just a few remarks:
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.
Misses leading \
Comment #72
fagoComment #73
BerdirRe-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.
Comment #75
BerdirWell, 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.
Comment #76
fagoI 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!
Comment #77
fagoWent through that manually as well, everything worked for me.
Comment #78
fago#75: user-ng-1818570-75.patch queued for re-testing.
Comment #79
alexpottLets get some profiling done on this... viewing a page, viewing a list of users using views etc...
Comment #80
fagoPerformance 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
Comment #81
fagook, 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.
Comment #82
andypostFixed broken tests, seems left from node conversion
Comment #86
BerdirRe-adding tags.
Comment #87
BerdirMoving the original check back into the user storage controller for now.
Comment #89
BerdirForgot the use, this should pass again.
Comment #90
andypostBack to RTBC, there's only merge changes
Comment #91
fago#89: user-ng-1818570-89.patch queued for re-testing.
Comment #92
alexpottSome 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?
Need to fix up the return value here as it no longer returns a User object...
The documented implementation seems to suggest the type hinting in user_user_view should be different...
Comment #93
effulgentsia CreditAttribution: effulgentsia commentedThis 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.
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.
Comment #94
effulgentsia CreditAttribution: effulgentsia commentedFixes for #92.
Comment #96
effulgentsia CreditAttribution: effulgentsia commentedComment #98
effulgentsia CreditAttribution: effulgentsia commented#96: user-ng-1818570-96.patch queued for re-testing.
Comment #99
BerdirRe-roll/updates look good, back RTBC.
Comment #100
alexpottCommitted 88e6a76 and pushed to 8.x. Thanks!
Comment #101
BerdirAdded to https://drupal.org/node/1806650, which will need more work, but we can complete that once all entities are converted.
Comment #102
BerdirRemoving sprint tag. See you all in #2017207: [meta] Complete conversion of users to Entity Field API!
Comment #103.0
(not verified) CreditAttribution: commentedUpdated issue summary.