#2062151: Create a current user service to ensure that current account is always available added a current user service so that we can guarantee that there's a user when it's requested. This should replace existing uses of _account from request attributes, and we can then remove it from there altogether.
Comment | File | Size | Author |
---|---|---|---|
#32 | account-2073531-32.patch | 25.88 KB | tim.plunkett |
#32 | interdiff.txt | 598 bytes | tim.plunkett |
Comments
Comment #1
dawehner#2048223: Add $account argument to AccessCheckInterface::access() method and use the current_user service contains the first bit of the change.
Comment #2
m1r1k CreditAttribution: m1r1k commentedOther related tasks:
#2095667: Use current_user service instead of _account attribute to get user object in user.module
#2095689: Use current_user service instead of _account attribute to get user object in overlay module
#2095641: Use current_user service instead of _account attribute to get user object
#2095709: Use current_user service instead of _account attribute to get user object in system module
#2095627: Use current_user service instead of _account attribute of request object in FormBase class
Comment #3
tim.plunkettLet's just do these all here, since some of them are interdependent. Please give m1r1k credit as well on this.
Comment #5
dawehnerAll this test failures also appeared in #2004086: The Request service must be synthetic
One major problem is that user_access() relies on global $user, but other ones are for example that a test did relied
on WebTestBase->drupalLogin() not changing $GLOBALS['user'], so that global $user is actually user 1(insert a big WTF here).
Comment #6
tim.plunkettTracked down the comment one. Posting a patch before a meeting :) will follow-up more later
Comment #8
tim.plunkettTrying more things...
Comment #11
tim.plunkettI don't even understand what's happening anymore.
Without replacing every single global $user/$GLOBALS['user'], I can't be sure of what's triggering it.
Also, the theme callback stuff is triggering this pretty early, so that'd be nice to get out of the way...
Comment #13
dawehnerAfter literal hours of debugging I realized that $this->web_user is overridden in the CommentNodeAccessTest file. This causes
to save the wrong node author. You might ask how this ever worked? No idea! (beside maybe admin was always the current user).
Next one: Drupal\comment\Tests\CommentTranslationUITest The admin user did not had 'skip comment approval', so the comment got never published.
PS: Small tip: if you run tests via the UI run them at anonymous user, given that this catches some of the bugs that just appear on run-tests.sh
Comment #14
dawehner.
Comment #16
dawehnerThe failure in SearchCommentCountToggleTest.php is caused by a different bug: For some reasons uid = 0 results in a NULL value of $node->get('uid')->value
Comment #17
dawehner.
Comment #19
tim.plunkettComment #20
dawehnerYeah I could not find anything after a total of 10hrs.
Comment #21
tim.plunkettBecause the remaining failures are in content_translation, tagging so someone will help...
Comment #22
xjmComment #23
BerdirHum, the retranslate flag fails, I've seen those way too often.
I'll try to have a look, @plach is usually good at figuring out what's going on with the translation UI tests, they're nasty.
Comment #24
plachWorking on those failures.
Comment #25
BerdirOk, don't completely understand what's going on, but here's the problem:
The drupalPostForm() on line 145 results in a validation error about "Anonymous" not being a valid user.
As a workaround, I added a check for uid 0, which probably should be there anyway, but that doesn't really explain why it was like it was before. But the node author form does it in a similar way so I assume it's necessary.
Oh, and if you want to know. Figuring this out had me write 150 lines of debug output in a file and then *diff* a passing and failing run, there I noticed a few missing lines that I could track down a form submit not happening.
Comment #26
BerdirOk. The 0 in there is coming from content_translation_entity_insert(), which runs within the test code. But as I said, my change IMHO makes sense anyway, as 0 *is* a valid value.
Comment #27
BerdirOk, here's a patch that fixes that function, also removed a now apparently useless rebuildContainer() call that reset the current user service. Tested that it also works without the change in the content translation controller, but keeping that change.
Comment #28
plachDefinitely :)
Comment #30
BerdirOh, looks like the rebuildContainer() removal there *did* cause some fails? That's.. not nice.
Comment #31
dawehnerThis is a really awesome technique!
Comment #32
tim.plunkettI'm leaving the rebuild in. It's really not our problem to fix it here.
The standard profile test fix was really stupid.
When creating the dummy content, the auth user was having input with trimmed whitespace, and anon wasn't. It came down to a global $user in filter_default_format().
This means that in EVERY OTHER USAGE of global $user, our tests aren't good enough. They should all have failed. But that's out of scope.
We need to get this in ASAP.
Comment #34
tim.plunkett32: account-2073531-32.patch queued for re-testing.
Comment #35
dawehnerI remember back in D6 that writing views API tests was hard, as you had to switch the global user. It is great to know
that we know have a proper way to fix it.
Comment #36
disasm CreditAttribution: disasm commentedWhat's the reason for these additional permissions being added to these test users?
This seems backwards to me. We're switching the Controller to go back to GLOBALS['user']?
Comment #37
tim.plunkett1) The entirety of this bug is that global $user is actually the user running the test, and it was only working because it thought it was uid 1. Now that it's not, we need to give them the proper perms.
2) We're leaving that $GLOBALS['user'] as is, and just fixing the logic around it.
Comment #38
disasm CreditAttribution: disasm commentedMarking RTBC. Fine with both explanations. Lets make sure we get a follow-up opened though for fixing GLOBAL $users everywhere (possibly a meta).
Comment #39
andypost+1 to RTBC here!
Let's get this one asap and unfreeze all conversions!
Comment #40
andypostAdded METAs that depends on this
Comment #41
catchfile_put_contents() and diff is also my favourite last-resort debugging technique, never fails (yet).
Committed/pushed to 8.x, thanks!
Comment #42
catchComment #43
damiankloip CreditAttribution: damiankloip commentedSo I guess I'll just close #2076703: Inject current_user service into CsrfTokenGenerator instead of getting the current user from the Request object then.