Replace user_access() calls with $account->hasPermission() in core files

Should use Drupal::currentUser() service

Ran some tests locally, which passed, here is a second attempt.

Re-roll.

Reroolled (I thing)

11: replace_user_access_calls_with_account_hasPermission-2062043-10.patch queued for re-testing.

Still needs some love

core/includes/menu.inc:602:    // As call_user_func_array is quite slow and user_access is a very common 
core/includes/menu.inc:604:    if ($callback == 'user_access') {
core/includes/menu.inc:3047:      $item['access callback'] = 'user_access';
core/includes/menu.inc:3203:    if (user_access('access site in maintenance mode')) {
core/includes/menu.inc:3208:        if (user_access('administer site configuration')) {
core/update.php:104:  if (\Drupal::moduleHandler()->moduleExists('dblog') && user_access('access site reports')) {
core/update.php:250:  // Calls to user_access() might fail during the Drupal 6 to 7 update process,
core/update.php:259:    return user_access('administer software updates');

Just a nitpick

+++ b/core/includes/menu.inc
@@ -599,10 +599,10 @@ function _menu_check_access(&$item, $map) {
+    // As call_user_func_array is quite slow and user_access is a very common ¶

trailing whitespace

I resolve #14 in my patch. If you like, use it:

https://drupal.org/node/2181629

The patch not works for me.

replace_user_access_calls_with_account_hasPermission-2062043-10.patch:44: trailing whitespace.
    // As call_user_func_array is quite slow and user_access is a very common
error: patch failed: core/lib/Drupal/Core/Extension/UpdateModuleHandler.php:46
error: core/lib/Drupal/Core/Extension/UpdateModuleHandler.php: patch does not apply

This is wrong:

-      $item['access'] = (count($arguments) == 1) ? user_access($arguments[0]) : user_access($arguments[0], $arguments[1]);
+      $item['access'] = (count($arguments) == 1) ? Drupal::currentUser()->hasPermission($arguments[0]) : Drupal::currentUser()->hasPermission($arguments[1]);

1.
arguments[1] is an account not permission, use a following:

$item['access'] = (count($arguments) == 1) ? Drupal::currentUser()->hasPermission($arguments[0]) : $arguments[1]->hasPermission($arguments[0]);

2.
_menu_site_is_offline() function contains 2 user_access() call correct it. See my patch: https://drupal.org/files/issues/remove_user_access_menu_inc.patch

I can't reroll a patch because I concentrate only menu.inc now, and have not time to understand all code in this patch. Sorry.

pp also wrote a patch for update.php

#2181615: Remove calls to deprecated user_access() in update.php

$item['access'] = (count($arguments) == 1) ? Drupal::currentUser()->hasPermission($arguments[0]) : Drupal::currentUser()->hasPermission($arguments[1]);;

$arguments[1] is a user object, not a permission string. The correct way is the following:

$arguments[1]->hasPermission($arguments[0]);

I re-rolled the patch (it needed another) because I was running into an issue with user_access within update.php (it could not be found, resulting in a fatal). pp's comment no longer seems relevant, at least to this patch; that code is no longer present in menu.inc.

Looks good in principle, just a few small points:

1. +++ b/core/authorize.php
   @@ -50,7 +50,7 @@
   +  return Settings::get('allow_authorize_operations', TRUE) && Drupal::currentUser()->hasPermission('administer software updates');
   

   We should be using \Drupal:: rather than just Drupal::, even in unnamespaced files. See #2053489: Standardize on \Drupal throughout core.

2. +++ b/core/includes/bootstrap.inc
   @@ -1588,13 +1588,13 @@ function drupal_classloader_register($name, $path) {
   - * function user_access($string, $account = NULL) {
   + * function datetime_default_format_type() {
     *   // Use the advanced drupal_static() pattern, since this is called very often.
     *   static $drupal_static_fast;
     *   if (!isset($drupal_static_fast)) {
   - *     $drupal_static_fast['perm'] = &drupal_static(__FUNCTION__);
   + *     $drupal_static_fast['format_type'] = &drupal_static(__FUNCTION__);
     *   }
   - *   $perm = &$drupal_static_fast['perm'];
   + *   $format_type = &$drupal_static_fast['format_type'];
   

   This is changing documentation, presumably because we plan to remove the user_access() function (unless it's just a merge error).

   Either way, datetime_default_format_type isn't a function, so we'll need to use something else.

3. +++ b/core/update.php
   @@ -255,7 +255,7 @@ function update_access_allowed() {
   -  // Calls to user_access() might fail during the Drupal 6 to 7 update process,
   +  // Calls to Drupal::currentUser()->hasPermission might fail during the Drupal 6 to 7 update process,
   

   Documentation should wrap at 80 characters

Thanks. Here's an updated patch, addressing your points 1 and 3.

I did not change the point about the documentation (yet), because I'm not sure what to change it into and I'm also not sure it's actually needed. It's in #20 as well, so at least its not a merge error. It serves as an example in the code block, and I suppose it makes sense removing user_access as an example if it is deprecated. Other examples from the same comment block are example_list(), examples_admin_overview() and mymodule_log_stream_handle(). user_access() is being used to illustrate the "fast" variant of using drupal_static(). I guess the question becomes, was the fact that user_access is (was) an actual function using the fast drupal_static() pattern an important enough aspect of the example to try and find another "real world" example (and if so, can we actually find another real world example)?

I found two examples in core, drupal_is_front_page(), which already has an RTBC patch to convert it to OO code (no statics, #2301975: Move drupal_is_front_page to PathMatcher service)

The other is template_preprocess(), so we should probably use that as an example, even if it too eventually gets refactored out.

The patch in #25 contains multiple commits. Generally it's preferred to upload a patch with a single commit, with an interdiff to highlight the changes since the last revision of the patch, see https://www.drupal.org/documentation/git/interdiff

#24.2 is not addressed, and I think better to replace the code with some abstract function

@28: Ah, sorry, had not seen that. It's the first time I generated a patch using SourceTree. Here's a replacement. I went back to command line.

Regarding the documentation example, I think I tend to agree with @andypost, especially if all this stuff is up for refactoring, we probably should not go for a real function (or we or others will end up having the same discussion when those get deprecated). If I understand you correctly, this pattern will eventually disappear anyway? That's probably reason enough to not put too much time into this.

Wow, these are indeed the only left places in core.

+++ b/core/includes/bootstrap.inc
@@ -1505,13 +1505,13 @@ function drupal_classloader_register($name, $path) {
  *
  * Example:
  * @code
- * function user_access($string, $account = NULL) {
+ * function datetime_default_format_type() {
  *   // Use the advanced drupal_static() pattern, since this is called very often.
  *   static $drupal_static_fast;
  *   if (!isset($drupal_static_fast)) {
- *     $drupal_static_fast['perm'] = &drupal_static(__FUNCTION__);
+ *     $drupal_static_fast['format_type'] = &drupal_static(__FUNCTION__);
  *   }
- *   $perm = &$drupal_static_fast['perm'];
+ *   $format_type = &$drupal_static_fast['format_type'];
  *   ...
  * }

Instead of relying on actual real life code, should we better come up with an example which actually does not change in the future? Note: there is a longterm goal to remove drupal_static() as well.

Ah.. PHPStorm did not return any results when searching for datetime_default_format_type, so I thought it *was* example code. Sorry for the confusion. Yes, I think we should come up with an example. Maybe it's enough to simply replace the function name, e.g. "example_default_format_type"?

+1 for that suggestion.

Here's a patch implementing that change.

[Sorry, didn't mean to post this comment, double post]

All points have been addressed, patch looks good.

+++ b/core/update.php
@@ -255,15 +255,16 @@ function update_access_allowed() {
-  // Calls to user_access() might fail during the Drupal 6 to 7 update process,
-  // so we fall back on requiring that the user be logged in as user #1.
+  // Calls to \Drupal::currentUser()->hasPermission might fail during the Drupal
+  // 6 to 7 update process, so we fall back on requiring that the user be logged
+  // in as user #1.
   try {
     $module_handler = \Drupal::moduleHandler();
...
     $module_handler->reload();
     $module_filenames = $module_handler->getModuleList();
     \Drupal::service('kernel')->updateModules($module_filenames, $module_filenames);
-    return user_access('administer software updates');
+    return \Drupal::currentUser()->hasPermission('administer software updates');
   }
   catch (\Exception $e) {
     return ($user->id() == 1);

This comment and code is super strange now since update.php can not be used for major version updates. So the whole try catch and adding the user module is not required. And we already have the current user. Personally I think the whole function could be simplified to:

function update_access_allowed() {
  // Allow the global variable in settings.php to override the access check.
  if (Settings::get('update_free_access')) {
    return TRUE;
  }
  return \Drupal::currentUser()->hasPermission('administer software updates');
}

Code further down update.php checks if the system table exists and if it does produces an error. This function will not be called if the code is running against a previous Drupal version db.

Ignore #38, sorry

Patch in comment #39 works fine. Changing to RTBC.

Committed 7d0c579 and pushed to 8.x. Thanks!