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.
Let's start to convert all calls to user_access() with the new AccountInterface::hasPermission() method.
core/authorize.php
core/lib/Drupal/Core/Extension/UpdateModuleHandler.php
core/update.php
core/includes/bootstrap.inc
core/includes/entity.api.php
core/includes/menu.inc
Part of #2048171: [meta] Replace user_access() calls with $account->hasPermission() wherever possible.
Change records for this issue:
Comment | File | Size | Author |
---|---|---|---|
#39 | 2062043-interdiff.txt | 1.22 KB | longwave |
#39 | 2062043-user_access.patch | 3.27 KB | longwave |
Comments
Comment #1
InternetDevels CreditAttribution: InternetDevels commentedSome of tests still fail locally, but here is the first try.
Comment #3
andypostShould use Drupal::currentUser() service
Comment #4
rhm5000 CreditAttribution: rhm5000 commentedComment #6
rhm5000 CreditAttribution: rhm5000 commentedRan some tests locally, which passed, here is a second attempt.
Comment #8
XanoRe-roll.
Comment #9
andypostComment #10
xjmComment #11
alvar0hurtad0Reroolled (I thing)
Comment #13
longwave11: replace_user_access_calls_with_account_hasPermission-2062043-10.patch queued for re-testing.
Comment #14
andypostStill needs some love
Comment #15
andypostJust a nitpick
trailing whitespace
Comment #16
pp CreditAttribution: pp commentedI resolve #14 in my patch. If you like, use it:
https://drupal.org/node/2181629
Comment #17
pp CreditAttribution: pp commentedThe patch not works for me.
This is wrong:
1.
arguments[1] is an account not permission, use a following:
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.
Comment #18
pp CreditAttribution: pp commentedComment #19
ianthomas_ukpp also wrote a patch for update.php
#2181615: Remove calls to deprecated user_access() in update.php
Comment #20
InternetDevels CreditAttribution: InternetDevels commentedPatch rerolled.
Comment #21
XanoComment #22
pp CreditAttribution: pp commented$arguments[1] is a user object, not a permission string. The correct way is the following:
Comment #23
eelkeblokI 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.
Comment #24
ianthomas_ukLooks good in principle, just a few small points:
We should be using \Drupal:: rather than just Drupal::, even in unnamespaced files. See #2053489: Standardize on \Drupal throughout core.
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.Documentation should wrap at 80 characters
Comment #25
eelkeblokThanks. 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)?
Comment #26
eelkeblokComment #27
ianthomas_ukI 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.
Comment #28
ianthomas_ukThe 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
Comment #29
andypost#24.2 is not addressed, and I think better to replace the code with some abstract function
Comment #30
eelkeblok@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.
Comment #31
dawehnerWow, these are indeed the only left places in core.
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.
Comment #32
eelkeblokAh.. 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"?
Comment #33
dawehner+1 for that suggestion.
Comment #34
eelkeblokHere's a patch implementing that change.
Comment #35
eelkeblok[Sorry, didn't mean to post this comment, double post]
Comment #36
ianthomas_ukAll points have been addressed, patch looks good.
Comment #37
alexpottThis 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:
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.
Comment #38
longwaveComment #39
longwaveIgnore #38, sorry
Comment #40
a_thakur CreditAttribution: a_thakur commentedPatch in comment #39 works fine. Changing to RTBC.
Comment #41
alexpottCommitted 7d0c579 and pushed to 8.x. Thanks!