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.
#1696660: Add an entity access API for single entity access got commited, but now we need to actually implement the API for all entity types.
- Implement an entity access controller for users
- Convert access checks to use
$entity->access()
.
Comment | File | Size | Author |
---|---|---|---|
#44 | user-access-1862752.44.interdiff.txt | 1 KB | larowlan |
#44 | user-access-1862752.44.patch | 10.6 KB | larowlan |
#41 | user-access-1862752.40.interdiff.txt | 680 bytes | larowlan |
#40 | user-access-1862752.40.interdiff.txt | 0 bytes | larowlan |
#40 | user-access-1862752.40.patch | 9.6 KB | larowlan |
Comments
Comment #1
larowlanI'll tackle this one.
Comment #2
larowlanFirst crack
Comment #4
larowlan#2: user-access-1862752.2.patch queued for re-testing.
Comment #6
larowlanneeded a merge against 8.x
Comment #8
larowlancredit for jthorson for setting me straight on this
Comment #10
larowlanthese two tests pass locally, kicking off again.
Comment #11
larowlan#8: user-access-1862752.8.patch queued for re-testing.
Comment #12
Berdir$account should be passed to these user_access() calls. Same for a few more below. Some do it correctly.
Comment #13
larowlanFixes issues identified by @Berdir and also an incorrect use of $account->uid > 0 which should have been $entity->uid.
Comment #15
larowlan#13: user-access-1862752.13.patch queued for re-testing.
Comment #17
BerdirThere is no custom render controller for users anymore, I guess that's why this failed.
Comment #18
larowlan@Berdir, thanks you're a legend!
Comment #20
BerdirLooks like a random test failure. I've seen a few of those already related to translation tests. Something strange is going on there...
Comment #21
Berdir#18: user-access-1862752.18.patch queued for re-testing.
Comment #22
BerdirShould be \Drupal\...
I haven't found a call in core that was still calling that with a user id.
I think we can safely drop that "feature" and type hint $account here accordingly.
Same for the other functions below.
And I would suggest to only keep using this functions for access callbacks and change direct calls to ->access('view').
Comment #23
larowlanThanks, attached fixes issues at #22 as well as fixing some doc blocks now that $account is type-hinted.
Comment #25
larowlanmeh two use Drupal\user\Plugin\Core\Entity\User;
Comment #26
BerdirMagic re-appearance of the render controller class? :)
We can replace this with a entity-generic implementation in system or entity module as soon as all entities are converted. We might even be able to kill this hook completely and just keep the alter. I've already started with a patch for that in the issue about taxonomy terms not implementing this.
It's a nice example of what the unified access API allows us to do :)
Comment #28
larowlanSun morning fail.
Definitely committed to my local branch now.
Comment #29
BerdirOk, I think this is ready to go. It has the same problems with User vs. anoymous user vs. $GLOBALS['user'] which are all different things but it's an existing problem and not blocking us here.
Comment #30
larowlanThis will need a change notice as the new preferred approach is eg $entity->access('view') over user_view_access() except for access callbacks
Comment #31
BerdirI guess we can update the existing access change notice, add all the conversion issues and a list of removed/deprecated functions. Speaking of that...
Wondering if we want to introduce a entity_page_access(Entity $entity, $operation = 'view') to unify those three access callbacks here.
Comment #32
larowlanMakes sense to me. Want a follow up?
Comment #33
BerdirEither that or update the patch here, I'll try to RTBC either asap :)
Comment #34
larowlanWhere abouts?
entity.module?
Comment #35
BerdirLet's add it to entity.inc for now, where everything else is. We haven't yet decided what exactly should happen with entity.module vs. entity.inc. Right now, the module is just the owner for the display entity type.
Comment #36
larowlanLet's see how this goes.
Comment #37
larowlanDef needs a change notice now we've dropped user_view_access, user_edit_access and user_cancel_access
Comment #38
BerdirYes, let's update http://drupal.org/node/1862420 with entity_page_access() (make sure to state that it should only be used for access callbacks) and the list of removed functions (including the operation that should be used for them now)
Comment #40
larowlanha, 'edit' != 'update'
Comment #41
larowlanWrong interdiff
Comment #43
larowlanhmm these fails look valid, menu data cached that references the old access callbacks for the upgrade tests and open id issues with same removed functions
Comment #44
larowlanMissed some refs in openid.module
Comment #45
BerdirThis is good to go if it passes the tests.
Comment #46
Dries CreditAttribution: Dries commentedI think this should be 'hasAccess()' instead of 'access()'.
I think it would be better to name this 'hasDeleteAccess()' instead of 'deleteAccess()'. For most people, 'deleteAccess()' means we'll delete access.
I understand none of these two things are introduced in this patch, but I wanted to share my thoughts nonetheless. Maybe we can create another issue for this, if not already? Small changes like this can avoid many WTFs. :-)
Comment #47
Dries CreditAttribution: Dries commentedOther than these API suggestions, I think this looks good. Committed to 8.x. Thanks.
Comment #48
Dave ReidhasAccess() sounds weird, like we're checking if the entity itself has access to something. Then again I can't really think of a better alternative besides leaving it 'access'.
Comment #50
xjmThis was marked for a change notice update but I don't see the change notice referenced in the summary. Can we make sure the change notice got updated and add this issue to its issue list? Thanks!
Comment #51
BerdirAdded this to https://drupal.org/node/1862420, but this was just a conversion issue, none of the others were added either.
Comment #52
larowlan