The documentation page for CsrfTokenGenerator::computeToken() describes the return value using the following phrase.

A 43-character URL-safe token for validation, based on the token seed, the hash salt provided by Settings::getHashSalt(), and the 'drupal_private_key' configuration variable.

The drupal_private_key configuration variable doesn't exist anymore; it was a persistent variable used in Drupal 7. The private key is stored in the system.private_key state key.

I would rather change the description to the following one.

A 43-character URL-safe token for validation, based on the token seed, the hash salt provided by Settings::getHashSalt(), and the site private key.

At the same time, the method description should say site private key instead of simply private key.

CommentFileSizeAuthor
#12 fix-documentation-comment3004281-12.patch720 bytesavpaderno
#11 fix-documentation-comment-3004281-11.patch1.3 KBdhirendra.mishra
#5 fix-documentation-comment-3004281-5.patch1.75 KBavpaderno
#3 3004281-3.patch720 byteshardikpandya

Comments

kiamlaluno created an issue. See original summary.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Issue summary: View changes
hardikpandya’s picture

hardikpandya commented
Status: Active » Needs review
StatusFileSize
new 3004281-3.patch720 bytes
joachim’s picture

joachim commented
Status: Needs review » Reviewed & tested by the community

LGTM.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new fix-documentation-comment-3004281-5.patch1.75 KB

There are other places in the CsrfTokenGenerator.php file where private key is used. This patch fixed those too, without touching private key service, which is the correct service name.

prashantgajare’s picture

prashantgajare commented
Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: fix-documentation-comment-3004281-5.patch, failed testing. View results

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs work » Needs review

The last test failed because the CI job was missing, not because an error in the patch.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php
@@ -71,7 +71,8 @@ public function get($value = '') {
+   * Validates a token based on $value, the user session, and the site private
+   * key.

@@ -91,7 +92,8 @@ public function validate($token, $value = '') {
+   * Generates a token based on $value, the token seed, and the site private
+   * key.

This makes the method description run over 80 cols (in 2 lines). According to our coding standards that's not allowed, the first line of documention is only allowed to be one line long.

dhirendra.mishra’s picture

dhirendra.mishra commented
Assigned: Unassigned » dhirendra.mishra

working on it.

dhirendra.mishra’s picture

dhirendra.mishra commented
Assigned: dhirendra.mishra » Unassigned
Status: Needs work » Needs review
StatusFileSize
new fix-documentation-comment-3004281-11.patch1.3 KB

Correction from #9. Please follow the patch.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
StatusFileSize
new fix-documentation-comment3004281-12.patch720 bytes

I left the short descriptions of the methods as they were, otherwise they would get on two lines, or a single line with more than 80 characters, which I understand #9 suggests it is wrong.

This makes the method description run over 80 cols (in 2 lines). According to our coding standards that's not allowed, the first line of documention is only allowed to be one line long.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

That seems a lot better. Thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: fix-documentation-comment3004281-12.patch, failed testing. View results

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs work » Needs review
dhirendra.mishra’s picture

dhirendra.mishra commented
Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: fix-documentation-comment3004281-12.patch, failed testing. View results

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs work » Needs review
bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Queued for testing. Patch itself looks good.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed e52debb and pushed to 8.7.x. Thanks!

  • catch committed e52debb on 8.7.x 
    Issue #3004281 by kiamlaluno, dhirendra.mishra, hardikpandya, borisson_...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.