Follow-up from #2163203: Remove calls to deprecated global $user wherever possible (outside bootstrap and authentication).
This is how I'm finding them all, FWIW and used the remaining items from #2163203-62: Remove calls to deprecated global $user wherever possible (outside bootstrap and authentication)
grep -R -e "GLOBALS\['user'\]" -e "global .*\$user" *
Excluding bootstrap and session (thought I may throw a patch in to see what happens).
I'll create a number of patches to see how testbot reacts to the changes then consolidate the green ones into a larger patch.
Let's expand the scope of this issue to convert all the trivial references outside bootstrap and session handling, and then work on those tougher issues separately.
Before RTBCing a patch for this issue, confirm that no more references to the global remain outside #2061953: Remove calls to deprecated global $user in core/lib/Drupal/Core/Authentication/AuthenticationManager.php , #2062771: #2062771: Remove calls to deprecated global $user in \Drupal\Core\Session\SessionManager, #2062069: Remove calls to deprecated global $user in core/includes/bootstrap.inc . Edit: Fixing TestBase and its child base classes might also merit a separate issue, but uses in normal test classes themselves should be (edit) either in this patch or in that patch, depending on which makes more sense. But not separately.
Comment | File | Size | Author |
---|---|---|---|
#54 | interdiff.txt | 1.57 KB | dawehner |
#54 | remove_global_user-2213899-54.patch | 2.42 KB | dawehner |
#48 | interdiff.txt | 3.35 KB | joelpittet |
#48 | remove_global_user-2213899-48.patch | 2.43 KB | joelpittet |
#46 | 2213899-global-user-round-2-46.patch | 3.2 KB | joelpittet |
Comments
Comment #1
joelpittetGive me some green testbot!
Comment #18
joelpittetOk here's the combined green. Let's see if they play nice together.
This leaves the following remaining:
Comment #19
tstoecklerAs this script is supposed to run on a D7 site we should leave those calls alone.
Otherwise looks great!
Comment #20
joelpittetThanks for the review @tstoeckler, this removes the d7 change, I knew there was a reason that didn't show up in the previous round.
Not a whole lot now but we be widdl'n.
Comment #21
joelpittetA better version of 2213899-1-DrupalDateTimeTest.patch but it will still fail because it needs the drupal_get_user_timezone() to be changed but that causes a circular reference.
Comment #22
joelpittet1: 2213899-1-all.patch queued for re-testing.
Comment #24
joelpittetRe-roll of #20
Also, grabbed a few from #1 that still applied to see where they are at.
Comment #25
joelpittetRe-roll for #24.
Comment #27
joelpittet1: 2213899-1-user.patch queued for re-testing.
Comment #28
joelpittet1: 2213899-1-user.pages_.patch queued for re-testing.
Comment #29
joelpittet1: 2213899-1-FormBuilder.patch queued for re-testing.
Comment #30
joelpittet1: 2213899-1-FormatDateTest.patch queued for re-testing.
Comment #31
joelpittet1: 2213899-1-EntityAccessTest.patch queued for re-testing.
Comment #32
joelpittet1: 2213899-1-EntityAccessController.patch queued for re-testing.
Comment #33
joelpittet1: 2213899-1-DrupalDateTimeTest.patch queued for re-testing.
Comment #34
joelpittet1: 2213899-1-Cookie.patch queued for re-testing.
Comment #35
joelpittet1: 2213899-1-AuthenticationManager.patch queued for re-testing.
Comment #36
joelpittet1: 2213899-1-AuthenticationEnhancer.patch queued for re-testing.
Comment #46
joelpittetAdding formbuilder patch from #1 because it passes now.
Comment #47
dawehnerTogether this won't really need the two code paths ...
This is part of the session handling and so potentially problematic.
The recommended API is now \Drupal::currentUser()->setCurrentUser($account);
Comment #48
joelpittet@dawehner thanks for the review.
1. Merged the paths
2. Reverted this change
3. Great that looks nice to work with, thanks, changed.
Comment #49
joelpittetaw rookie mistake ... :P
Comment #51
joelpittet@dawehner it seems that the setCurrentUser isn't working as it should in those tests. I'll dig in deeper in a while.
Comment #54
dawehnerLet's just fix the tests.
Comment #55
moshe weitzman CreditAttribution: moshe weitzman commentedLooks sane and modern.
Comment #56
webchickThis changes the logic such that $this->currentUser could end up being NULL. On the one hand, tests are passing, and chx said in IRC he didn't think it was a big deal because FormBuilder should come well after session loading stuff when that ceases to be an issue. On the other hand, it seems to have come from #2112807: Move the form builder functions in form.inc to a form service and doesn't seem to be a copy/paste of anything else that I can find, indicating it was a deliberate change.
Can we look more into this before committing?
Comment #57
dawehnerTechnically the current user is a lazy initialized object. So unless someone calls for example
->id()
or->hasPermission()
on it, the user does not have to be initialized.Afaik the reason why we have this hasService call is that first step of the installer doesn't have a proper container yet.
Comment #58
webchickCool, thanks for the explanation.
Committed and pushed to 8.x. Thanks!