Part of #1976158: Rename entity storage/list/form/render "controllers" to handlers.

Example TermAccessController -> TermAccessControlHandler

Including:
- Base class and interface
- All subclasses
- Document references
- EntityManager->getAccessController() method,
- EntityType->getAccessClass() method,
- entity_access_controller() (removed, as not used anymore
- variables and property names

Sandbox D8 Field API branch 2154435-entity-access-handler

CommentFileSizeAuthor
#92 2154435-entity-access-control-handler-92-patch-diff.txt922 bytesberdir
#92 2154435-entity-access-control-handler-92.patch98.14 KBberdir
#89 2154435-entity-access-control-handler-89.patch98.15 KBberdir
#83 2154435-entity-access-control-handler-78.patch98.14 KBberdir
#81 2154435-entity-access-handler-78.patch91.63 KBberdir
#78 2154435-entity-access-handler-78-interdiff.txt131.21 KBberdir
#77 2154435-entity-access-handler-77-interdiff.txt11.61 KBberdir
#77 2154435-entity-access-handler-77.patch93.3 KBberdir
#74 2154435-entity-access-handler-74.patch87.86 KBandypost
#74 interdiff.txt21.34 KBandypost
#66 rename-2154435-66.patch86.81 KBmparker17
#63 rename-2154435-62.patch86.58 KBmparker17
#51 2154435-entity-access-handler-51.patch84.86 KBandypost
#51 interdiff.txt8.11 KBandypost
#44 2154435-entity-access-handler-44.patch80.61 KBandypost
#40 2154435-entity-access-handler-40.patch80.71 KBandypost
#40 interdiff.txt798 bytesandypost
#39 2154435-entity-access-handler-39.patch80.68 KBandypost
#36 2154435-entity-access-handler-36.patch80.67 KBandypost
#33 2154435-entity-access-handler-33.patch80.71 KBandypost
#32 2154435-entity-access-handler-32.patch80.71 KBandypost
#27 2154435-entity-access-handler-27.patch81.9 KBandypost
#21 2154435-entity-access-handler-21.patch81.99 KBandypost
#21 interdiff.txt910 bytesandypost
#19 2154435-entity-access-handler-19.patch81.98 KBandypost
#19 interdiff.txt1.59 KBandypost
#18 2154435-entity-access-handler-18.patch81.55 KBandypost
#18 interdiff.txt981 bytesandypost
#16 2154435-entity-access-handler-16.patch81.54 KBandypost
#12 2154435-entity-access-12.patch77.56 KBandypost
#12 interdiff.txt796 bytesandypost
#11 2154435-entity-access-11.patch77.55 KBandypost
#11 interdiff.txt2.82 KBandypost
#8 2154435-entity-access-8.patch79.01 KBandypost
#3 2154435-entity-access-3.patch69.34 KBandypost

Comments

amateescu’s picture

amateescu commented
Status: Active » Closed (duplicate)

Duplicate of #2120871: Rename EntityListController to EntityListBuilder.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Title: Rename EntityListController to EntityList » Rename EntityAccessController to EntityAccess
Issue summary: View changes
Status: Closed (duplicate) » Active

So let's simply rename the issue

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Active » Needs review
StatusFileSize
new 2154435-entity-access-3.patch69.34 KB
xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +beta target, +beta deadline
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

3: 2154435-entity-access-3.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 3: 2154435-entity-access-3.patch, failed testing.

The last submitted patch, 3: 2154435-entity-access-3.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new 2154435-entity-access-8.patch79.01 KB

Status: Needs review » Needs work

The last submitted patch, 8: 2154435-entity-access-8.patch, failed testing.

The last submitted patch, 8: 2154435-entity-access-8.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt2.82 KB
new 2154435-entity-access-11.patch77.55 KB

proper merge

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt796 bytes
new 2154435-entity-access-12.patch77.56 KB

And one more

The last submitted patch, 11: 2154435-entity-access-11.patch, failed testing.

The last submitted patch, 11: 2154435-entity-access-11.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
commented

Finally green and waits for reviews

andypost’s picture

andypost
he/him
Primary language Russian
commented
Title: Rename EntityAccessController to EntityAccess » Rename EntityAccessController to EntityAccessHandler
StatusFileSize
new 2154435-entity-access-handler-16.patch81.54 KB

As was discussed this should be a handler because actually this kind of classes introduce access checker but the name used by routing

Patch also unifies all doc blocks and comments

Status: Needs review » Needs work

The last submitted patch, 16: 2154435-entity-access-handler-16.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt981 bytes
new 2154435-entity-access-handler-18.patch81.55 KB

Fix broken test

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt1.59 KB
new 2154435-entity-access-handler-19.patch81.98 KB

And a bit more comments fixed

Status: Needs review » Needs work

The last submitted patch, 19: 2154435-entity-access-handler-19.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt910 bytes
new 2154435-entity-access-handler-21.patch81.99 KB

Fix last broken test

The last submitted patch, 18: 2154435-entity-access-handler-18.patch, failed testing.

The last submitted patch, 18: 2154435-entity-access-handler-18.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

I actually thought EntityAccess was quite a nice name and more inline with where we're going with EntityStorage, EntityList, etc.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

EntityList has been changed to EntityListBuilder as it's not an entity list, it is something that builds a list of entities. Just like this isn't an entity access, it handles/checks access to entities.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

EntityStorage is different, because it actually represents the storage, so I'm fine with that.

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 2154435-entity-access-handler-27.patch81.9 KB

re-roll

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

All right, I'm fine with it. #25/ #26 is very arguable, though. EntityStorage isn't the storage either, the DB (or whatever) is the storage, EntityStorage is the one that provides access to the storage. So it could be named StorageHandler as well. Also we have a thing called FormInterface but that actually *builds* forms (i.e. FormInterface::buildForm()) but we apparently conceive it as *the form* conceptually. So EntityList could just as well be conceived as *the list*.

Again, don't want to hold this up just wanted to point that out.

effulgentsia’s picture

effulgentsia commented

Just to play devil's advocate, why not leave this one as AccessController, since "access control" is standard software terminology?

andypost’s picture

andypost
he/him
Primary language Russian
commented

because controllers used for different purpose ;) in current core

effulgentsia’s picture

effulgentsia commented

"controller" has a well established meaning in MVC terminology and in Symfony routing. But "access control" also has a well established meaning in software systems. So while I get the benefit of renaming "controller" as a generic Drupalism away from the entity system, I'm not yet convinced of the need to remove "AccessController". Can you clarify what the problem is in having the overlap, given that it's not an overlap we're inventing?

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 2154435-entity-access-handler-32.patch80.71 KB

re-roll

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 2154435-entity-access-handler-33.patch80.71 KB

Re-roll, pushed to sandbox/yched/1736366.git branch 2154435-entity-access-handler

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes
andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes
andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 2154435-entity-access-handler-36.patch80.67 KB

re-roll

Status: Needs review » Needs work

The last submitted patch, 36: 2154435-entity-access-handler-36.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review

36: 2154435-entity-access-handler-36.patch queued for re-testing.

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 2154435-entity-access-handler-39.patch80.68 KB

re-roll after #2120871: Rename EntityListController to EntityListBuilder

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt798 bytes
new 2154435-entity-access-handler-40.patch80.71 KB

fixed wrong merge

The last submitted patch, 39: 2154435-entity-access-handler-39.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

New suggestion from @alexpott: EntityAccessControl.

Let's discuss this in person before we continue here.

xjm’s picture

xjm
she/her
Primary language English
commented

I'd prefer either EntityAccessControl or EntityAccessControlHandler.

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 2154435-entity-access-handler-44.patch80.61 KB

EntityAccessControlHandler looks great!
So how to rename EntityManager->getAccess()?

just a test of re-roll

Status: Needs review » Needs work

The last submitted patch, 44: 2154435-entity-access-handler-44.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
commented
Title: Rename EntityAccessController to EntityAccessHandler » Rename EntityAccessController to EntityAccessControl or EntityAccessControlHandler
effulgentsia’s picture

effulgentsia commented

Having spent some time the past few days with our route access check(er)s in #2222719: Use parameter matching via reflection for access checks instead of pulling variables from request attributes, I'm actually left wondering whether AccessChecker would be the best suffix. I kind of like it. What do you all think?

As a separate issue, I wouldn't mind us renaming all of the routing ones from *AccessCheck to *AccessChecker as well. If we felt a need to disambiguate between route access checkers and entity access checkers, then we could make the route ones *RouteAccessChecker, or the entity ones *EntityAccessChecker, or both, but at the moment, I don't really see a need for that disambiguation as there's other context cues for whether you're dealing with a route or an entity.

Just floating the thought out there. If other than me, there's consensus on EntityAccessControl or EntityAccessControlHandler, go ahead with that. It's hard to please everyone, so I'm willing to go along with whatever people generally like most.

effulgentsia’s picture

effulgentsia commented

Oh, and one more comment on why I think it's ok for route access classes and entity access classes to have the same suffix: it's just like *Storage. We have php storage, keyvalue storage, config storage, entity storage, etc., and classes implementing any of those is simply named *Storage, and IMO, there's no confusion with that. So I think the same would be true for access checking.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Hm, while it is true that we have many storage things, they all do have different names.

Assuming we'd rename both to checker, then we'd have two EntityAccessChecker classes, and one would be using the other to check access. Having two layers of access checks is already pretty confusing, and giving them the same name wouldn't help.

We had this situation with entity lists, where we had a route controller EntityListController that called the other EntityListController to build the list of entities which were then returned, I think that was pretty confusing.

effulgentsia’s picture

effulgentsia commented

In the case of the literal class EntityAccessCheck(er), we could rename that one to EntityRouteAccessCheck(er). And similar for others where the entity type name and the route access type name are too similar. But I think there are very few of those.

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt8.11 KB
new 2154435-entity-access-handler-51.patch84.86 KB

re-roll pushed to sandbox, so can we get consensus here? or better to postpone this one on rename routing to EntityRouteAccessCheck(er)

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

51: 2154435-entity-access-handler-51.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 51: 2154435-entity-access-handler-51.patch, failed testing.

chx’s picture

chx commented
Status: Needs work » Postponed

We are not rearranging the deck chairs on the Titanic. #2204363: [sechole] Returning TRUE from hook_entity_access()/hook_ENTITYTYPE_access() must not bypass EntityAccessController::checkAccess() has priority.

chx’s picture

chx commented
Related issues: +#2204363: [sechole] Returning TRUE from hook_entity_access()/hook_ENTITYTYPE_access() must not bypass EntityAccessController::checkAccess()
xjm’s picture

xjm
she/her
Primary language English
commented

@chx, does this really need to be postponed on that? It seems like #2204363: [sechole] Returning TRUE from hook_entity_access()/hook_ENTITYTYPE_access() must not bypass EntityAccessController::checkAccess() would be pretty easy to reroll for the class renames.

chx’s picture

chx commented

When are we going to stop blowing security holes into Drupal and deprioritizing the fixes?

chx’s picture

chx commented

Of course, it's easy to reroll that but in my opinion (which is quite strong due to that bug still being open and the title XSS ) we do not have our priorities straight when we say "oh it's just a slight inconvenience" yes it is but it's a security issue and we should not inconvenience security fixes. Give not an inch. Thou shalt not suffer a sechole to live. etc.

chx’s picture

chx commented
Status: Postponed » Needs work

At the time, let's move forward with this and re-debate when it gets to commit. I do understand the need to work :/

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue tags: +Needs reroll

The patch in #51 no longer applies to 8.x HEAD.

When I tried to re-roll the patch, I got a bunch of merge conflicts :S

CONFLICT (rename/rename): Rename "core/modules/views/lib/Drupal/views/ViewAccessController.php"->"core/modules/views/src/ViewAccessController.php" in branch "HEAD" rename "core/modules/views/lib/Drupal/views/ViewAccessController.php"->"core/modules/views/lib/Drupal/views/ViewAccessHandler.php" in "2154435-51"
CONFLICT (rename/rename): Rename "core/modules/user/lib/Drupal/user/UserAccessController.php"->"core/modules/user/src/UserAccessController.php" in branch "HEAD" rename "core/modules/user/lib/Drupal/user/UserAccessController.php"->"core/modules/user/lib/Drupal/user/UserAccessHandler.php" in "2154435-51"
CONFLICT (rename/rename): Rename "core/modules/user/lib/Drupal/user/RoleAccessController.php"->"core/modules/user/src/RoleAccessController.php" in branch "HEAD" rename "core/modules/user/lib/Drupal/user/RoleAccessController.php"->"core/modules/user/lib/Drupal/user/RoleAccessHandler.php" in "2154435-51"
CONFLICT (rename/rename): Rename "core/modules/taxonomy/lib/Drupal/taxonomy/TermAccessController.php"->"core/modules/taxonomy/src/TermAccessController.php" in branch "HEAD" rename "core/modules/taxonomy/lib/Drupal/taxonomy/TermAccessController.php"->"core/modules/taxonomy/lib/Drupal/taxonomy/TermAccessHandler.php" in "2154435-51"
CONFLICT (rename/rename): Rename "core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestAccessController.php"->"core/modules/system/tests/modules/entity_test/src/EntityTestAccessController.php" in branch "HEAD" rename "core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestAccessController.php"->"core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestAccessHandler.php" in "2154435-51"
CONFLICT (rename/rename): Rename "core/modules/system/lib/Drupal/system/MenuAccessController.php"->"core/modules/system/src/MenuAccessController.php" in branch "HEAD" rename "core/modules/system/lib/Drupal/system/MenuAccessController.php"->"core/modules/system/lib/Drupal/system/MenuAccessHandler.php" in "2154435-51"
CONFLICT (rename/rename): Rename "core/modules/system/lib/Drupal/system/DateFormatAccessController.php"->"core/modules/system/src/DateFormatAccessController.php" in branch "HEAD" rename "core/modules/system/lib/Drupal/system/DateFormatAccessController.php"->"core/modules/system/lib/Drupal/system/DateFormatAccessHandler.php" in "2154435-51"
CONFLICT (rename/rename): Rename "core/modules/search/lib/Drupal/search/SearchPageAccessController.php"->"core/modules/search/src/SearchPageAccessController.php" in branch "HEAD" rename "core/modules/search/lib/Drupal/search/SearchPageAccessController.php"->"core/modules/search/lib/Drupal/search/SearchPageAccessHandler.php" in "2154435-51"
CONFLICT (rename/rename): Rename "core/modules/node/lib/Drupal/node/NodeTypeAccessController.php"->"core/modules/node/src/NodeTypeAccessController.php" in branch "HEAD" rename "core/modules/node/lib/Drupal/node/NodeTypeAccessController.php"->"core/modules/node/lib/Drupal/node/NodeTypeAccessHandler.php" in "2154435-51"
CONFLICT (rename/rename): Rename "core/modules/node/lib/Drupal/node/NodeAccessControllerInterface.php"->"core/modules/node/src/NodeAccessControllerInterface.php" in branch "HEAD" rename "core/modules/node/lib/Drupal/node/NodeAccessControllerInterface.php"->"core/modules/node/lib/Drupal/node/NodeAccessInterface.php" in "2154435-51"
CONFLICT (rename/rename): Rename "core/modules/node/lib/Drupal/node/NodeAccessController.php"->"core/modules/node/src/NodeAccessController.php" in branch "HEAD" rename "core/modules/node/lib/Drupal/node/NodeAccessController.php"->"core/modules/node/lib/Drupal/node/NodeAccessHandler.php" in "2154435-51"
CONFLICT (modify/delete): core/modules/node/lib/Drupal/node/Controller/NodeController.php deleted in HEAD and modified in 2154435-51. Version 2154435-51 of core/modules/node/lib/Drupal/node/Controller/NodeController.php left in tree.
CONFLICT (modify/delete): core/modules/node/lib/Drupal/node/Access/NodeAddAccessCheck.php deleted in HEAD and modified in 2154435-51. Version 2154435-51 of core/modules/node/lib/Drupal/node/Access/NodeAddAccessCheck.php left in tree.
CONFLICT (rename/rename): Rename "core/modules/menu_link/lib/Drupal/menu_link/MenuLinkAccessController.php"->"core/modules/menu_link/src/MenuLinkAccessController.php" in branch "HEAD" rename "core/modules/menu_link/lib/Drupal/menu_link/MenuLinkAccessController.php"->"core/modules/menu_link/lib/Drupal/menu_link/MenuLinkAccessHandler.php" in "2154435-51"
CONFLICT (rename/rename): Rename "core/modules/language/lib/Drupal/language/LanguageAccessController.php"->"core/modules/language/src/LanguageAccessController.php" in branch "HEAD" rename "core/modules/language/lib/Drupal/language/LanguageAccessController.php"->"core/modules/language/lib/Drupal/language/LanguageAccessHandler.php" in "2154435-51"
CONFLICT (content): Merge conflict in core/modules/forum/forum.module
CONFLICT (rename/rename): Rename "core/modules/filter/lib/Drupal/filter/FilterFormatAccess.php"->"core/modules/filter/src/FilterFormatAccess.php" in branch "HEAD" rename "core/modules/filter/lib/Drupal/filter/FilterFormatAccess.php"->"core/modules/filter/lib/Drupal/filter/FilterFormatAccessHandler.php" in "2154435-51"
CONFLICT (rename/rename): Rename "core/modules/field/lib/Drupal/field/FieldInstanceConfigAccessController.php"->"core/modules/field/src/FieldInstanceConfigAccessController.php" in branch "HEAD" rename "core/modules/field/lib/Drupal/field/FieldInstanceConfigAccessController.php"->"core/modules/field/lib/Drupal/field/FieldInstanceConfigAccessHandler.php" in "2154435-51"
CONFLICT (rename/rename): Rename "core/modules/contact/lib/Drupal/contact/CategoryAccessController.php"->"core/modules/contact/src/CategoryAccessController.php" in branch "HEAD" rename "core/modules/contact/lib/Drupal/contact/CategoryAccessController.php"->"core/modules/contact/lib/Drupal/contact/CategoryAccessHandler.php" in "2154435-51"
CONFLICT (rename/rename): Rename "core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestAccessController.php"->"core/modules/config/tests/config_test/src/ConfigTestAccessController.php" in branch "HEAD" rename "core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestAccessController.php"->"core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestAccessHandler.php" in "2154435-51"
CONFLICT (rename/rename): Rename "core/modules/comment/lib/Drupal/comment/CommentAccessController.php"->"core/modules/comment/src/CommentAccessController.php" in branch "HEAD" rename "core/modules/comment/lib/Drupal/comment/CommentAccessController.php"->"core/modules/comment/lib/Drupal/comment/CommentAccessHandler.php" in "2154435-51"
CONFLICT (content): Merge conflict in core/modules/block_content/src/Entity/BlockContent.php
CONFLICT (rename/rename): Rename "core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockAccessController.php"->"core/modules/block_content/src/BlockContentAccessController.php" in branch "HEAD" rename "core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockAccessController.php"->"core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockAccessHandler.php" in "2154435-51"
CONFLICT (rename/delete): core/modules/block/lib/Drupal/block/BlockAccessHandler.php deleted in HEAD and renamed in 2154435-51. Version 2154435-51 of core/modules/block/lib/Drupal/block/BlockAccessHandler.php left in tree.
CONFLICT (content): Merge conflict in core/modules/block/block.api.php
CONFLICT (content): Merge conflict in core/lib/Drupal/Core/Entity/EntityType.php
CONFLICT (content): Merge conflict in core/lib/Drupal/Core/Entity/EntityManagerInterface.php
CONFLICT (content): Merge conflict in core/lib/Drupal/Core/Entity/Entity.php
CONFLICT (content): Merge conflict in core/includes/entity.inc
xjm’s picture

xjm
she/her
Primary language English
commented

I think we should just go with FooAccessControlHandler since it's explicit and less likely to be confused, so that we can go forward here and get this in.

@mparker17 is working on a reroll now. :)

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Status: Needs work » Needs review
StatusFileSize
new rename-2154435-62.patch86.58 KB

Note that the patch in #51 did not rename the "access" key to either "access_control" or "access_control_handler" as discussed in #1976158: Rename entity storage/list/form/render "controllers" to handlers, so this re-roll doesn't either.

Because there are so many changes to 8.x HEAD since the last patch, because re-rolling has so many merge conflicts, because it's difficult to generate an interdiff, and because I'm a n00b, I'm going to post the steps I took to re-roll the path. I'd appreciate it if @andypost could review these to make sure I didn't mess up somewhere.

Using PHPStorm 7.1.3:

  1. Refactor/rename \Drupal\Core\Entity\EntityAccessControllerInterface to \Drupal\Core\Entity\EntityAccessHandlerInterface, using the following options:
    • Search in comments and strings = TRUE
    • Search for text occurrences = TRUE
    • Rename inheritors = TRUE
    • Rename containing file = TRUE
    • When asked if I want to rename the containing file from EntityAccessControllerInterface.php to EntityAccessHandlerInterface.php, I say "yes".
    • When asked if I want to rename inheritors from EntityAccessController to EntityAccessHandler, I say "yes".
  2. Refactor/rename \Drupal\Core\Entity\EntityManager::getAccessController() to \Drupal\Core\Entity\EntityManager::getAccessHandler(), using the following options:
    • Search in comments and strings = TRUE
    • Search for text occurrences = TRUE
  3. Replace any calls to get('entity.manager')->getAccessController() with get('entity.manager')->getAccessHandler().
  4. Rename each sub-class of \Drupal\Core\Entity\EntityAccessHandler from "Controller" to "Handler".
    Note \Drupal\filter\FilterFormatAccess should be re-named to \Drupal\filter\FilterFormatAccessHandler.
    At time-of-writing, here are the subclasses of EntityAccessHandler I knew about:
    1. Rename \Drupal\node\NodeAccessController to \Drupal\node\NodeAccessHandler.
    2. Rename \Drupal\language\LanguageAccessController to \Drupal\language\LanguageAccessHandler.
    3. Rename \Drupal\system\DateFormatAccessController to \Drupal\system\DateFormatAccessHandler.
    4. Rename \Drupal\filter\FilterFormatAccess to \Drupal\filter\FilterFormatAccessHandler.
    5. Rename \Drupal\contact\CategoryAccessController to \Drupal\contact\CategoryAccessHandler.
    6. Rename \Drupal\field\FieldInstanceConfigAccessController to \Drupal\field\Handler.
    7. Rename \Drupal\views\ViewAccessController to \Drupal\views\ViewAccessHandler.
    8. Rename \Drupal\comment\CommentAccessController to \Drupal\comment\CommentAccessHandler.
    9. Rename \Drupal\block\BlockAccessController to \Drupal\block\BlockAccessHandler.
    10. Rename \Drupal\config_test\ConfigTestAccessController to \Drupal\config_test\ConfigTestAccessHandler.
    11. Rename \Drupal\node\NodeTypeAccessController to \Drupal\node\NodeTypeAccessHandler.
    12. Rename \Drupal\menu_link\MenuLinkAccessController to \Drupal\menu_link\MenuLinkAccessHandler.
    13. Rename \Drupal\system\MenuAccessController to \Drupal\system\MenuAccessHandler.
    14. Rename \Drupal\taxonomy\TermAccessController to \Drupal\taxonomy\TermAccessHandler.
    15. Rename \Drupal\user\UserAccessController to \Drupal\user\UserAccessHandler.
    16. Rename \Drupal\block_content\BlockContentAccessController to \Drupal\block_content\BlockContentAccessHandler.
    17. Rename \Drupal\shortcut\ShortcutSetAccessController to \Drupal\shortcut\ShortcutSetAccessHandler.
    18. Rename \Drupal\user\RoleAccessController to \Drupal\user\RoleAccessHandler.
    19. Rename \Drupal\search\SearchPageAccessController to \Drupal\search\SearchPageAccessHandler.
    20. Rename \Drupal\entity_test\EntityTestAccessController to \Drupal\entity_test\EntityTestAccessHandler.
    21. Rename \Drupal\shortcut\ShortcutAccessController to \Drupal\shortcut\ShortcutAccessHandler.
  5. Rename \Drupal\system\Tests\Entity\EntityAccessTest to \Drupal\system\Tests\Entity\EntityAccessHandlerTest.
  6. Where appropriate, edit prose comments about "[access] controller[s]" so they talk about "[access] handler[s]" instead in any file you modified above.
  7. Change core/modules/system/core.api.php's entity_api and user_api sections so they refer to "access handlers" instead of "access controllers"
  8. Change \Drupal\entity_test\Entity\EntityTestDefaultAccess so it refers to "access handlers" instead of "access controllers".
  9. Ensure each access handler references the class that uses it as it's access handler with an @see line that does not end with a period.

Status: Needs review » Needs work

The last submitted patch, 63: rename-2154435-62.patch, failed testing.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

Note that the patch + instructions-for-rerolling in #63 don't include @xjm's suggestion in #62. And testbot doesn't like it either :S

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue tags: -Needs reroll +Needs issue summary update
StatusFileSize
new rename-2154435-66.patch86.81 KB

Oops, apparently I didn't rename the file containing EntityAccessHandler. Hopefully testbot will like it now.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Status: Needs work » Needs review

Oops, I need to set it to "needs review" in order for Testbot to pay attention to it. :P

andypost’s picture

andypost
he/him
Primary language Russian
commented 
+++ b/core/modules/block/src/BlockAccessHandler.php
@@ -2,19 +2,21 @@
- * Provides a Block access controller.
+ * Defines the access handler for the block entity type.

Also the doc blocks should be updated to new name

Status: Needs review » Needs work

The last submitted patch, 66: rename-2154435-66.patch, failed testing.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Status: Needs work » Needs review

Umm... I don't see how any of those test-run errors are related to the changes made in this patch.

mparker17 queued 66: rename-2154435-66.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 66: rename-2154435-66.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Those errors mean that unit tests are failing, run them locally with cd core; ./vendor/bin/phpunit to see the actual fails, testbot is not able to report fatal errors from unit tests properly.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt21.34 KB
new 2154435-entity-access-handler-74.patch87.86 KB

Fixed re-roll

andypost’s picture

andypost
he/him
Primary language Russian
commented

go with FooAccessControlHandler since it's explicit and less likely to be confused

Now patch is green, so I pushed the code to sandbox and will try to rename all this again

andypost’s picture

andypost
he/him
Primary language Russian
commented 
+++ b/core/lib/Drupal/Core/Entity/EntityManagerInterface.php
@@ -92,15 +92,15 @@ public function getFieldStorageDefinitions($entity_type_id);
-  public function getAccessController($entity_type);
+  public function getAccessHandler($entity_type);

Should this named as getAccessControlHandler?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new 2154435-entity-access-handler-77.patch93.3 KB
new 2154435-entity-access-handler-77-interdiff.txt11.61 KB

Re-rolled this based on the sandbox, pushed updated branch.

Did not yet rename it, do we have an agreement on AccessControlHandler now? If so, then I'm going to do the rename...

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new 2154435-entity-access-handler-78.patch91.63 KB
new 2154435-entity-access-handler-78-interdiff.txt131.21 KB

Bad timing, menu_link is gone now, so another reroll.

dawehner’s picture

dawehner
Primary language German
commented

Great patch! Will RTBC this one when the question is answered.

  1. +++ b/core/modules/node/src/Access/NodeRevisionAccessCheck.php
@@ -27,9 +27,9 @@ class NodeRevisionAccessCheck implements AccessInterface {
+   * The node access handler.
...
+   * @var \Drupal\Core\Entity\EntityAccessHandlerInterface
    */
   protected $nodeAccess;

+++ b/core/modules/node/src/NodeAccessHandler.php
@@ -20,9 +20,11 @@
+class NodeAccessHandler extends EntityAccessHandler implements NodeAccessHandlerInterface, EntityControllerInterface {

    Given that there is a node specific access interface wouldn't make it sense to typehint here? 

  2. +++ b/core/modules/views/src/ViewAccessHandler.php
@@ -2,19 +2,21 @@
diff --git a/core/scripts/switch-psr4.sh b/core/scripts/switch-psr4.sh

diff --git a/core/scripts/switch-psr4.sh b/core/scripts/switch-psr4.sh
index eb1a480..dcffcaa 100644

index eb1a480..dcffcaa 100644
--- a/core/scripts/switch-psr4.sh

--- a/core/scripts/switch-psr4.sh
+++ b/core/scripts/switch-psr4.sh

+++ b/core/scripts/switch-psr4.sh
+++ b/core/scripts/switch-psr4.sh
@@ -8,7 +8,7 @@

@@ -8,7 +8,7 @@
  * Moves module-provided class files to their PSR-4 location.

    OT: do we have a followup to drop tis script again?

xjm’s picture

xjm
she/her
Primary language English
commented

@dawehener, re: point 2: yep, #2277739: Remove core/scripts/switch-psr4.sh is "revisit before release candidate".

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new 2154435-entity-access-handler-78.patch91.63 KB

Re-uploading the file, crosspost killed it.

xjm’s picture

xjm
she/her
Primary language English
commented

We discussed yesterday with @effulgentsia, @berdir, @yched, @timplunkett, and myself that we will go with EntityAccessControlHandler for the name.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new 2154435-entity-access-control-handler-78.patch98.14 KB

Yep. Leads to some pretty long names, but otherwise, this should be OK. No traces of "AccessHandler", "AccessController", "entity access controller", "entity access handler", "access_handler" or "access_controller" left in core/.

Interdiff was as big as the actual patch, so leaving that out...

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Title: Rename EntityAccessController to EntityAccessControl or EntityAccessControlHandler » Rename EntityAccessController to EntityAccessControlHandler
Issue summary: View changes
Issue tags: -Needs issue summary update

Updated issue summary a bit.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Title: Rename EntityAccessController to EntityAccessControl or EntityAccessControlHandler » Rename EntityAccessController to EntityAccessControlHandler
Issue summary: View changes
Issue tags: -Needs issue summary update

Updated issue summary a bit.

xjm’s picture

xjm
she/her
Primary language English
commented

Skimming the changes in this patch, I definitely think this is the right way to go. Everything is automatically less confusing in code and documentation alike with that terminology.

Berdir queued 83: 2154435-entity-access-control-handler-78.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 83: 2154435-entity-access-control-handler-78.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new 2154435-entity-access-control-handler-89.patch98.15 KB

Re-roll.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

Let's get this in and finish renames

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll

git ac https://www.drupal.org/files/issues/2154435-entity-access-control-handler-89.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   98k  100   98k    0     0  89954      0  0:00:01  0:00:01 --:--:--  131k
error: patch failed: core/lib/Drupal/Core/Entity/EntityManagerInterface.php:92
error: core/lib/Drupal/Core/Entity/EntityManagerInterface.php: patch does not apply
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
StatusFileSize
new 2154435-entity-access-control-handler-92.patch98.14 KB
new 2154435-entity-access-control-handler-92-patch-diff.txt922 bytes

Trivial context conflict, sometimes I'm wondering why git isn't smarter about some of those conflicts, patch apply I can understand but git merge hat the same problem. The method above getAccessControlHandler() is now getFieldMapByFieldType() instead of getFieldMap(), that's all that changed, see manual patch diff. So keeping at RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs change record updates

Committed 258856a and pushed to 8.0.x. Thanks!

Let's update all these CRs https://www.drupal.org/list-changes/published/drupal?keywords_descriptio...

  • alexpott committed 258856a on 8.0.x 
    Issue #2154435 by andypost, Berdir, mparker17: Rename...
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Finally!

Working on the change records.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Ok, all traces of access controller and AccessController updated, only mention left is in https://www.drupal.org/node/2200867, as the old name.

Status: Fixed » Closed (fixed)

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

kerby70’s picture

kerby70 commented

Note for related searches:

change EntityAccessControllerInterface to EntityAccessControlHandlerInterface