Let's start to convert all calls to user_access() with the new AccountInterface::hasPermission() method.

Update: These should be rolled into only three patches, combining

Steps:

1. The first patch (comment 1) should just target places where we can easily retrieve the current user. Otherwise we would need to access the Request to retrieve the active account from that which would probably dramatically increase the patch size.

2. Create sub issues for all modules and core directories.

3. Create patches for all sub issues, get RTBC, wait for core commits (view table below).

4. Remove deprecated function user_access.

Needs works issues

Contact module #2061977: Replace user_access() calls with $account->hasPermission() in all core modules except user
User module #2062039: Replace user_access() calls with $account->hasPermission() in user module
Other core files #2062043: Replace user_access() calls with $account->hasPermission() in core files

Committed and pushed to 8.x.

Action module #2061959: Replace user_access() calls with $account->hasPermission() in action module
Aggregator module #2061969: Replace user_access() calls with $account->hasPermission() in aggregator module
Block module #2061971: Replace user_access() calls with $account->hasPermission() in block module
Book module #2061973: Replace user_access() calls with $account->hasPermission() in book module
Contextual module #2061979: Replace user_access() calls with $account->hasPermission() in contextual module
Edit module #2061983: Replace user_access() calls with $account->hasPermission() in edit module
Entity module #2061985: Replace user_access() calls with $account->hasPermission() in entity module
Field module #2061987: Replace user_access() calls with $account->hasPermission() in field module
Language module #2062001: Replace user_access() calls with $account->hasPermission() in language module
Layout module #2062003: Replace user_access() calls with $account->hasPermission() in layout module
Menu module #2062005: Replace user_access() calls with $account->hasPermission() in menu module
Overlay module #2062009: Replace user_access() calls with $account->hasPermission() in overlay module
Path module #2062011: Replace user_access() calls with $account->hasPermission() in path module
Php module #2062013: Replace user_access() calls with $account->hasPermission() in php module
Picture module #2062015: Replace user_access() calls with $account->hasPermission() in picture module
Rdf module #2062017: Replace user_access() calls with $account->hasPermission() in rdf module
Search module #2062019: Replace user_access() calls with $account->hasPermission() in search module
shortcut module #2062021: Replace user_access() calls with $account->hasPermission() in shortcut module
Statistics module #2062023: Replace user_access() calls with $account->hasPermission() in statistics module
Taxonomy module #2062027: Replace user_access() calls with $account->hasPermission() in taxonomy module
Tour module #2062031: Replace user_access() calls with $account->hasPermission() in tour module
Tracker module #2062033: Replace user_access() calls with $account->hasPermission() in tracker module
Tanslation module #2062035: Replace user_access() calls with $account->hasPermission() in translation module
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fubhy’s picture

Status: Active » Needs review
FileSize
44.71 KB

This was done solely with a regex and replaces all occurrences of user_access\((.*), (.*)\) (so, just those with two arguments).

Anything more complex than that most likely needs some extra code except for those cases where we have the Request object available. Will take another look (manually) for those next. But that can be a separate patch.

fubhy’s picture

Issue tags: +Novice

Adding novice tag. This patch is so small that we should consider also covering invocations of user_access() in controllers (where we have $account always available through the request object).

Status: Needs review » Needs work

The last submitted patch, 2048171-1.patch, failed testing.

jibran’s picture

merdekiti’s picture

Issue tags: -Novice +CodeSprintCIS

Good issue for Code Sprint CIS

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -CodeSprintCIS

#1: 2048171-1.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +CodeSprintCIS

The last submitted patch, 2048171-1.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

add sub issues

dawehner’s picture

Title: Replace user_access() calls with $account->hasPermission() wherever possible. » [meta] Replace user_access() calls with $account->hasPermission() wherever possible.

Add tag.

larowlan’s picture

larowlan’s picture

Zemelia’s picture

webchick’s picture

So I am currently spending about 50-60% of my core committing time just keeping up on these patches. :(

Since these seem to be all find/replace, with the exception of introducing a $user = Drupal::currentUser() if it's not already there, could we take the approach of one massive patch that just does them all in one go?

andypost’s picture

@webchick only 9 was commited, most of all other issues depends on #2048223: Add $account argument to AccessCheckInterface::access() method and use the current_user service or require to inject current_user service so each one could be big for example #2061971-5: Replace user_access() calls with $account->hasPermission() in block module

I'd like have sandbox but after sprint random people file patches so I have no connection to them, I'm just reviewing patches

andypost’s picture

Issue summary: View changes

add steps to description

andypost’s picture

bohart’s picture

Issue summary: View changes

Issue summary updated.
Sub-issues was grouped in two tables (that already done and that needs work).

andypost’s picture

Issue summary: View changes
xjm’s picture

Thanks for all your work on this so far! Looking over the RTBC queue, these patches are very tiny, way too small to merit one per module. It's a burden on core maintainer time. Let's combine them. If you really want to break them down into separate patches I'd suggest one for user.module, one for all modules except user.module, one for everything else. I'll mark the existing issues as duplicates of one of the outstanding ones.

core/authorize.php
core/includes/bootstrap.inc
core/includes/menu.inc
core/lib/Drupal/Core/Extension/UpdateModuleHandler.php
core/modules/comment/comment.module
core/modules/comment/lib/Drupal/comment/CommentAccessController.php
core/modules/comment/lib/Drupal/comment/Plugin/entity_reference/selection/CommentSelection.php
core/modules/comment/lib/Drupal/comment/Tests/CommentLinksTest.php
core/modules/contact/lib/Drupal/contact/MessageFormController.php
core/modules/content_translation/content_translation.admin.inc
core/modules/content_translation/content_translation.module
core/modules/content_translation/lib/Drupal/content_translation/ContentTranslationController.php
core/modules/field_ui/field_ui.module
core/modules/file/file.module
core/modules/filter/filter.module
core/modules/filter/lib/Drupal/filter/Tests/FilterFormatAccessTest.php
core/modules/node/lib/Drupal/node/NodeAccessController.php
core/modules/node/lib/Drupal/node/NodeFormController.php
core/modules/node/lib/Drupal/node/Plugin/entity_reference/selection/NodeSelection.php
core/modules/node/lib/Drupal/node/Tests/NodeRevisionPermissionsTest.php
core/modules/node/node.api.php
core/modules/node/node.module
core/modules/node/node.pages.inc
core/modules/node/node.views_execution.inc
core/modules/node/tests/modules/node_access_test/node_access_test.module
core/modules/system/entity.api.php
core/modules/system/lib/Drupal/system/DateFormatAccessController.php
core/modules/system/lib/Drupal/system/Form/ModulesListForm.php
core/modules/system/system.api.php
core/modules/system/system.module
core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestAccessController.php
core/modules/system/tests/modules/form_test/form_test.module
core/modules/system/theme.api.php
core/modules/toolbar/toolbar.module
core/modules/tracker/tracker.module
core/modules/update/update.module
core/modules/user/lib/Drupal/user/AccountFormController.php
core/modules/user/lib/Drupal/user/EventSubscriber/MaintenanceModeSubscriber.php
core/modules/user/lib/Drupal/user/Plugin/entity_reference/selection/UserSelection.php
core/modules/user/lib/Drupal/user/RegisterFormController.php
core/modules/user/lib/Drupal/user/Tests/UserPermissionsTest.php
core/modules/user/lib/Drupal/user/UserAccessController.php
core/modules/user/user.api.php
core/modules/user/user.module
core/modules/views/views.api.php
core/modules/views/views.module
core/update.php
xjm’s picture

Issue summary: View changes

Rescoping to three child issues.

andypost’s picture

Needs to compile a commit message from all closed issues to give commit credit to all contributors

webchick’s picture

That's fine; just add it to the issue summary, the core committers can copy/paste it from there.

InternetDevels’s picture

Status: Needs work » Needs review
FileSize
74.09 KB
andypost’s picture

@Internetdevels please use sub issues for 3 patches.

The compiled commit message for modules except user in #2061977-10: Replace user_access() calls with $account->hasPermission() in all core modules except user

ianthomas_uk’s picture

I've just marked 15 issues as duplicates of the two sub issues of this. They all had patches though, so if we find we've missed some cases there may already be patches ready. See https://drupal.org/project/issues/search/drupal?version[]=8.x&issue_tags...

ianthomas_uk’s picture

Status: Needs review » Fixed

All child issues are fixed, and there is an RTBC issue to remove the function: #2306429: Remove user_access()

Status: Fixed » Closed (fixed)

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