Problem/Motivation

The CsrfTokenGenerator service currently gets the request set, just to get the _account attribute. Now that we have a current_user service, we can just inject this instead. This should be more reliable as well as being able to remove the setRequest() method.

Proposed resolution

Inject current_user service, remove request usage, convert tests

Remaining tasks

See above, Resolve current_user request scope issues.

User interface changes

None

API changes

None

CommentFileSizeAuthor
#10 2076703-10.patch5.33 KBdamiankloip
#10 interdiff-2076703-10.txt584 bytesdamiankloip
#8 2076703-8.patch5.53 KBdamiankloip
#8 interdiff-20767038-8.txt510 bytesdamiankloip
#4 2076703-4.patch5.53 KBdamiankloip
#4 interdiff-2076703-4.txt2.39 KBdamiankloip
csrf-generator-current-user.patch5.41 KBdamiankloip
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Probably dependent on #2076411: Remove the request scope from the current user service

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Active » Needs review
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Note: this won't work when we put back the request scope back as the token generator is used outside of the request response. Maybe we could try to make the user service optional as only the generate method is used outside of the request scope.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
FileSize
interdiff-2076703-4.txt2.39 KB
2076703-4.patch5.53 KB

Yes, I like that idea. Let's do that.

Status: Needs review » Needs work

The last submitted patch, 2076703-4.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review

#4: 2076703-4.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 2076703-4.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
FileSize
interdiff-20767038-8.txt510 bytes
2076703-8.patch5.53 KB
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php
@@ -26,30 +27,32 @@ class CsrfTokenGenerator {
+   * @param \Drupal\Core\Session\AccountInterface
+   *   The current user account.
    */

Unneeded...

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
FileSize
interdiff-2076703-10.txt584 bytes
2076703-10.patch5.33 KB

Totally.

Status: Needs review » Needs work

The last submitted patch, 2076703-10.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review

#10: 2076703-10.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 2076703-10.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

I just removed a docblock... :(

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

This broke after #1975962: Move comment_links() into CommentRenderController. I think this is the reason the tests in #2109433: Replace user_access() through injected user accounts in views. are also failing too.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Issue summary: View changes
Status: Needs work » Closed (duplicate)