In FormBuilder::prepareForm() there are the following code comments that need to be fixed.

  1. Merges in a default, this means if you've explicitly set #token to the the $form_id on a GET form, which we don't recommend, it will work.

    The first verb should not use the third person. There should be a period after default. (It's a comma-splice sentence.) the is repeated twice in row (set #token to the the $form_id).

  2. Prevent user agents from prefilling the build id with earlier values. When the ajax command "update_build_id" is executed, the user agent will assume that a user interaction changed the field. Upon a soft reload of the page, the previous build id will be restored in the input, causing subsequent ajax callbacks to access the wrong cached form build. Setting the autocomplete attribute to "off" will tell the user agent to never reuse the value.

    ajax and id should be written using capital letters.

  3. Generate a public token based on the form id. Generates a placeholder based on the form ID.

         // ['\Classname', 'methodname']
         // ['Classname', 'methodname']
         // '\Classname::methodname'
    -    // 'Classname::methodname'
    +    // 'Classname::methodname'.
    

    I would rather leave out all those examples of callable, as the parameter is callable $value_callable and what PHP considers a callable is well documented. It would be as adding a comment containing examples of integer values for a parameter that is int $count.

    This can be replaced by a single sentence.

    Generate a public token and a placeholder based on the form ID.

  4. Form processing and validation requires this value, so ensure the submitted form value appears literally, regardless of custom #tree and #parents being set elsewhere.

    The first sentence has a plural subject (form processing and validation). I would rewrite is as follows.

    Form processing and validation require this value. Ensure the submitted form value appears literally, regardless of custom #tree and #parents being set elsewhere.

  5. Instead of setting an actual CSRF token, we've set the placeholder in form_token's #default_value and #placeholder. These will be replaced at the very last moment. This ensures forms with a CSRF token don't have poor cacheability.

    It can be replaced from the following comment.

    Instead of setting an actual CSRF token, we've set the placeholder in form_token's #default_value and #placeholder. These will be replaced at the very last moment, to ensure that forms with a CSRF token don't have poor cacheability.

  6. Provide a selector usable by JavaScript. As the ID is unique, its not possible to rely on it in JavaScript.

    The correct comment is the following one.

    Provide a selector usable by JavaScript. As the ID is unique, it's not possible to rely on it in JavaScript.

CommentFileSizeAuthor
#59 interdiff.3040274.55-59.txt1.11 KBlongwave
#59 3040274-59.patch7.79 KBlongwave
#57 9.0.x.jpg14.59 KBayushmishra206
#57 8.9.x.jpg14.54 KBayushmishra206
#56 diff_48-55.txt1.07 KBsarvjeetsingh
#55 3040274-55.patch7.79 KBayushmishra206
#54 diff-48-53.txt1.08 KBquietone
#53 3040274-53.patch7.79 KBayushmishra206
#48 3040274-48.patch8.08 KBalexpott
#48 46-48-interdiff.txt296 bytesalexpott
#46 interdiff_43-46.txt1.02 KBsarvjeetsingh
#46 fix-grammar-spelling-style-of-code-comments-3040274-46.patch7.79 KBsarvjeetsingh
#43 interdiff_40-43.txt2.02 KBsarvjeetsingh
#43 fix-grammar-spelling-style-of-code-comments-3040274-43.patch7.6 KBsarvjeetsingh
#40 interdiff_36-40.txt1.47 KBsarvjeetsingh
#40 fix-grammar-spelling-style-of-code-comments-3040274-40.patch8.17 KBsarvjeetsingh
#37 fix-grammar-spelling-style-of-code-comments-3040274-36.patch8.08 KBrishabhthakur
#34 fix-grammar-spelling-style-of-code-comments-3040274-34.patch9.38 KBrishabhthakur
#34 interdiff_22-34.txt5.01 KBrishabhthakur
#27 Screenshot 2020-06-02 at 7.10.34 PM.png258.1 KBkkalashnikov
#22 3040274-22.patch6.63 KBravi.shankar
#19 fix-grammar-spelling-style-of-code-comments-3040274-19.patch9.28 KBamarphule
#14 interdiff.txt263 bytestheotherlondon
#14 3040274-14.patch11.29 KBtheotherlondon
#12 fix-comments-3040274-12.patch11.3 KBgringoinc
#9 fix-comments-3040274-9.patch11.77 KBkkalaskar
#9 code fix.JPG133.04 KBkkalaskar
#7 fix-comments-3040274-7.patch6.63 KBapaderno
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kiamlaluno created an issue. See original summary.

apaderno’s picture

Issue summary: View changes
apaderno’s picture

Issue summary: View changes
apaderno’s picture

Issue summary: View changes
apaderno’s picture

Issue summary: View changes
apaderno’s picture

Issue summary: View changes
Issue tags: +Novice

I noticed just now that api.drupal.org has now links for Drupal 8.8.x documentation.

apaderno’s picture

Status: Active » Needs review
FileSize
6.63 KB
kwfinken’s picture

Working on this at MidCamp 2019. oops, commented on wrong tab of browser.

kkalaskar’s picture

Hello @kiamlaluno,
I have applied the patch on my local Drupal 8.8 installation. It seems okay to proceed with patch #7. Also, I checked with following command

phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md core/lib/Drupal/Core/Form/FormBuilder.php

I found out there were several coding standard issues for reference please check the attached screenshot(code fix.jpg). I have fixed all those in patch fix-comments-3040274-9.patch. Please review once and let me know your thoughts on it.

apaderno’s picture

Status: Needs review » Needs work

I made a quick review, and this is what I found.

  •    /**
    -   * Defines element value callables which are safe to run even when the form
    -   * state has an invalid CSRF token.
    +   * Defines element value callables.
    +   *
    +   * Those callbacks are safe to run even when the form
    +   *  state has an invalid CSRF token.
        *
    

    If I recall correctly, the description of a class property doesn't need to be split in two lines.

  •     *  - Drupal\Core\Datetime\Element\Datetime::valueCallback
        *  - Drupal\Core\Render\Element\ImageButton::valueCallback
        *  - Drupal\file\Plugin\Field\FieldWidget\FileWidget::value
    -   *  - color_palette_color_value
    +   *  - color_palette_color_value.
    

    The last item of a list should not have any period.

  •      // If the form returns a response, skip subsequent page construction by
         // throwing an exception.
         // @see Drupal\Core\EventSubscriber\EnforcedFormResponseSubscriber
    -    //
    +    // .
    

    There should not be a period even after the @see directive.

  • +      // The placeholder uses a fixed string that is
    +      // Crypt::hashBase64('Drupal\Core\Form\FormBuilder::prepareForm');.
    

    It would be better something similar to the following one.

    +      // The placeholder uses a fixed string that is the output of
    +      // Crypt::hashBase64('Drupal\Core\Form\FormBuilder::prepareForm').
    
gringoinc’s picture

I'm working on this Issue at Drupalcon2019 Seattle.

gringoinc’s picture

Status: Needs work » Needs review
FileSize
11.3 KB

Hi everyone, I'm in Seattle at Drupalcon, and I was reviewing what was commented by @kiamlaluno and I found the following:

If I recall correctly, the description of a class property doesn't need to be split in two lines.

I eliminated the line break, but the "CODESNIFFER" would throw me error of maximum characters per line.

The last item of a list should not have any period.

I eliminated the period.

There should not be a period even after the @see directive.

I eliminated the period, and the blank line.

It would be better something similar to the following one.

I add the description.

It is my first contribution, and I do not know if there are more possible changes ... I await your comments. Regards,

theotherlondon’s picture

Issue tags: +Seattle2019

Hi Everyone! I'm working on this issue at DrupalCon.

theotherlondon’s picture

I removed the comma in the comment "replaced at the very last moment, to ensure forms with a CSRF token"

The comment now reads, "replaced at the very last moment to ensure forms with a CSRF token"

This is my first contribution and I think that this update removes an unnecessary comma.

awardsolutions’s picture

Status: Needs review » Reviewed & tested by the community

Read through this. Looks good!

kdebisschop’s picture

I don't understand why there is a period on line 524 of core/lib/Drupal/Core/Form/FormBuilder.php -- should that have been removed also?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 3040274-14.patch, failed testing. View results

amarphule’s picture

Assigned: Unassigned » amarphule
amarphule’s picture

Assigned: amarphule » Unassigned
Status: Needs work » Needs review
Issue tags: +#d8docpune
FileSize
9.28 KB

Fixed grammar, spelling, and style of the code comments

apaderno’s picture

Status: Needs review » Needs work

This is a quick review; there could be other changes to make.

    *  - Drupal\Core\Datetime\Element\Datetime::valueCallback
    *  - Drupal\Core\Render\Element\ImageButton::valueCallback
    *  - Drupal\file\Plugin\Field\FieldWidget\FileWidget::value
-   *  - color_palette_color_value
+   *  - color_palette_color_value.

In a list, none of the items have a period at the end, including the last item.

     // If the form returns a response, skip subsequent page construction by
     // throwing an exception.
     // @see Drupal\Core\EventSubscriber\EnforcedFormResponseSubscriber
-    //
     // @todo Exceptions should not be used for code flow control. However, the
     //   Form API does not integrate with the HTTP Kernel based architecture of
     //   Drupal 8. In order to resolve this issue properly it is necessary to

The empty comment between @see and @todo is correct.

-   * #lazy_builder callback; renders form CSRF token.
+   * The #lazy_builder callback; renders form CSRF token.

From the patch is not clear the context, but it would be probably better to make it a sentence by removing the semicolon, and adding the article before form CSRF token (The #lazy_builder callback renders the form CSRF token.).

-        $element = call_user_func_array($form_state->prepareCallback($callback), [&$element, &$form_state, &$complete_form]);
+        $element = call_user_func_array(
+          $form_state->prepareCallback($callback),
+          [
+            &$element, &$form_state, &$complete_form,
+          ]
+        );

This patch should just fix the content of comments, not how code is formatted.

volkswagenchick’s picture

Issue tags: +drupalnorth2019

Tagging for DrupalNorth 2019

ravi.shankar’s picture

Status: Needs work » Needs review
FileSize
6.63 KB

I have modified.

volkswagenchick’s picture

Issue tags: +badcamp2019

Tagging for badcamp2019, thanks! (October 2-5)

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

kkalashnikov’s picture

Assigned: Unassigned » kkalashnikov
kkalashnikov’s picture

image#22 patch applied successfully for Drupal 9.1 as well.

Good to move to RTBC

kkalashnikov’s picture

Assigned: kkalashnikov » Unassigned
nitvirus’s picture

Assigned: Unassigned » nitvirus
Status: Needs review » Reviewed & tested by the community

Hi,
#22 patch applies successfully checked again. Changing the status to RTBC

nitvirus’s picture

unassigning

nitvirus’s picture

Assigned: nitvirus » Unassigned
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -107,7 +107,9 @@ class FormBuilder implements FormBuilderInterface, FormValidatorInterface, FormS
    -   * Defines element value callables which are safe to run even when the form
    +   * Defines element value callables.
    +   *
    +   * Those callbacks are safe to run even when the form.
        * state has an invalid CSRF token.
    

    This is not an improvement. It does not define all the element value callables.

  2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -689,8 +691,9 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
           // Use the proper API to generate the placeholder, when we have one. See
    -      // https://www.drupal.org/node/2562341. The placeholder uses a fixed string
    -      // that is Crypt::hashBase64('Drupal\Core\Form\FormBuilder::prepareForm');
    +      // https://www.drupal.org/node/2562341.
    +      // The placeholder uses a fixed string that is the output of
    +      // Crypt::hashBase64('Drupal\Core\Form\FormBuilder::prepareForm');
    

    How come we put string on the next line? The sentence that starts "The placeholder" is very much a follow-on from the next.

  3. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -760,7 +762,7 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
    -        // Generate a public token based on the form id.
    +        // Generate a public token and placeholder based on the form ID.
             // Generates a placeholder based on the form ID.
    

    If we're doing this then we can remove the line that says // Generates a placeholder based on the form ID.

rishabhthakur’s picture

Assigned: Unassigned » rishabhthakur
rishabhthakur’s picture

Assigned: rishabhthakur » Unassigned
Status: Needs work » Needs review
FileSize
5.01 KB
9.38 KB

Modified last Patch and also fix some coding standard issues.

tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -690,9 +689,10 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
    -      // Use the proper API to generate the placeholder, when we have one. See
    -      // https://www.drupal.org/node/2562341. The placeholder uses a fixed string
    ...
    +      // @see https://www.drupal.org/node/2562341.
    

    You cannot use @see inline.

  2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -729,15 +729,15 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
    -      // Prevent user agents from prefilling the build id with earlier values.
    ...
    +      // Prevent user agents from prefilling the build Id with earlier values.
    ...
    +      // reload of the page, the previous build Id will be restored in the
    

    ID

  3. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -729,15 +729,15 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
    -      // When the ajax command "update_build_id" is executed, the user agent
    ...
    +      // When the Ajax command "update_build_id" is executed, the user agent
    ...
    +      // input, causing subsequent Ajax callbacks to access the wrong cached
    

    Either ajax or AJAX, not Ajax.

  4. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -748,8 +748,7 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
    -    // requested previously by the user and protects against cross site request
    -    // forgeries.
    +    // requested previously by the user and protects against CSRF.
    

    Why change to an abbreviation?

Many of the changes are inconsistent. Some join two sentences with a comma. Others split up a sentence previously joined by a comma.

ravi.shankar’s picture

Status: Needs review » Needs work

Changing status to needs work as per #34 and #35.

rishabhthakur’s picture

Status: Needs work » Needs review
FileSize
8.08 KB

modified as per last comment #35

davidhernandez’s picture

Status: Needs review » Needs work
-   * Defines element value callables which are safe to run even when the form
-   * state has an invalid CSRF token.
+   * Defines element value callables for skip on invalid CSRF token.

Is this an improvement? I'm not sure what this sentence is trying to say. Should it say "callables to skip" ?

+      // Use the proper API to generate the placeholder, when we have one.
+      // See https://www.drupal.org/node/2562341.
+      // The placeholder used to fix string, that is output of:

I think this happened after the review in #35 but now the "See ... " is on a new line when it was wrapped before. On a new line it feels like it would need the start of a new paragraph after it.

The placeholder used to fix string, that is output of:
Also this sentence needs rewriting. The grammar changes don't seem to improve the sentence.

+      // Merges in default, this means if you've explicitly set #token to
       // the $form_id on a GET form, which we don't recommend, it will work.

This is a bit of a run on sentence. It should probably be two separate sentences.

Most of the typo changes like "it's" seem fine, but for the grammar and explanation changes, I think someone with word smithing skills needs to do some rewriting.

sarvjeetsingh’s picture

Assigned: Unassigned » sarvjeetsingh
sarvjeetsingh’s picture

Assigned: sarvjeetsingh » Unassigned
Status: Needs work » Needs review
FileSize
8.17 KB
1.47 KB

I have updated the patch as per the above suggestions.
please review. Thanks!

apaderno’s picture

Issue summary: View changes
Status: Needs review » Needs work
-   * Defines element value callables which are safe to run even when the form
-   * state has an invalid CSRF token.
+   * Defines form element value callables to skip an invalid CSRF token.

The existing text is more correct. There isn't any need to change it.

-   * #lazy_builder callback; renders a form action URL.
+   * The #lazy_builder callback renders a form action URL.

Since it's the comment documenting a function/method, it should be Renders a form action URL. If adding it doesn't make the line longer than 80 characters, I would change it to Renders a form action URL. It's a #lazy_builder callback.

-          // Form processing and validation requires this value, so ensure the
+          // Form processing and validation requires this value. Ensure the

Form processing and validation needs the verb in third person plural (require).

-      // Merges in a default, this means if you've explicitly set #token to the
-      // the $form_id on a GET form, which we don't recommend, it will work.
+      // Merges in default. This means if you've explicitly set #token to
+      // the $form_id on a GET form, which is not recommended, it will work.

As the original text, it misses a that. Since it's a comment inside code, the first verb needs to be an imperative verb. I would rather change the phrase in the comment to the following.

Merge in the default value. This means that, if you've explicitly set #token to the $form_id on a GET form, which isn't recommended, it will work.

apaderno’s picture

As for last suggested change, I am not sure the last sentence (This means that, if you've explicitly set #token to the $form_id on a GET form, which isn't recommended, it will work.) is really necessary.
Since it's commenting code that is $form += ['#token' => FALSE]; what explained is the behavior of PHP. In the following example, $array['a'] is still 1.

$array = [
  'a' =>  1,
  'b' => 2,
];

$array += ['a' => 4];

It would make sense to say not to set #token to the value of $form_id, but I doubt that developers would notice that comment, and avoid using the value of $form_id as #token value.

Given this, I would rather remove the full comment, as the left part (Merge in the default value.) says something that is obvious by reading the comment.

That patch part should rather be the following.

-      // Merges in a default, this means if you've explicitly set #token to the
-      // the $form_id on a GET form, which we don't recommend, it will work.
sarvjeetsingh’s picture

Thanks @kiamlaluno for suggestions. I also think that (This means that, if you've explicitly set #token to the $form_id on a GET form, which isn't recommended, it will work.) is not really necessary.
Updated patch with recommended changes.
please review.

apaderno’s picture

Status: Needs review » Needs work

I apologize, I reviewed the patch completely, I noted a change that should be fixed, but I didn't report it in my previous comment.

     // ['\Classname', 'methodname']
     // ['Classname', 'methodname']
     // '\Classname::methodname'
-    // 'Classname::methodname'
+    // 'Classname::methodname'.
     if (is_callable($value_callable, FALSE, $callable_name)) {

Instead of adding a period where it should not be, I would remove the full comment.

  // The same static class method callable may be formatted in two array and
  // two string forms:
  // ['\Classname', 'methodname']
  // ['Classname', 'methodname']
  // '\Classname::methodname'
  // 'Classname::methodname'

The comment doesn't help in understanding the code line following it.

sarvjeetsingh’s picture

Assigned: Unassigned » sarvjeetsingh
sarvjeetsingh’s picture

Assigned: sarvjeetsingh » Unassigned
Status: Needs work » Needs review
FileSize
7.79 KB
1.02 KB

Updated the patch with recommended changes.
please review.

apaderno’s picture

Status: Needs review » Reviewed & tested by the community

The patch has been changed as suggested.

alexpott’s picture

As we're fixing spellings here we need to regenerate the dictionary.

larowlan’s picture

Saving issue credits

  • larowlan committed 97cff66 on 9.1.x
    Issue #3040274 by sarvjeetsingh, rishabhthakur, theotherlondon, alexpott...
larowlan’s picture

Title: Fix grammar, spelling, and style of the code comments in FormBuilder::prepareForm() » [backport] Fix grammar, spelling, and style of the code comments in FormBuilder::prepareForm()
Version: 9.1.x-dev » 9.0.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +Needs reroll, +Bug Smash Initiative

Committed 97cff66 and pushed to 9.1.x. Thanks!

Needs reroll for earlier versions

ayushmishra206’s picture

Assigned: Unassigned » ayushmishra206
ayushmishra206’s picture

Status: Patch (to be ported) » Needs review
FileSize
7.79 KB

Rerolled for 9.0.x, Please review.

quietone’s picture

Status: Needs review » Needs work
FileSize
1.08 KB

The patch in #53 has coding standard errors. Before seeing that I made a diff between #48 and #53, which shows that a comment was changed in the reroll.

ayushmishra206’s picture

Assigned: ayushmishra206 » Unassigned
Status: Needs work » Needs review
FileSize
7.79 KB

Rerolled the patch for both 8.9 and 9.0. Please review.

sarvjeetsingh’s picture

Status: Needs review » Needs work
FileSize
1.07 KB

The last patch still has some minor coding standard errors, it can be seen in the attached diff of #48 and #55.
Also , it looks like the patch is only Rerolled for 9.0 and not for 8.9 yet.

ayushmishra206’s picture

FileSize
14.54 KB
14.59 KB

@sarvjeetsingh core/misc/cspell/dictionary.txt This file does not exist in both 8.9.x and 9.0.x. Please suggest what's to be done about that?
Also i did apply the patch on both 8.9 and 9.0 applies cleanly.

apaderno’s picture

Yes, the core/misc/cspell/dictionary.txt exists only in the 9.1.x branch. Previous branches don't have that file.

longwave’s picture

Fixed the two errors noted in #56. This should apply to both 8.9.x and 9.0.x.

sarvjeetsingh’s picture

Status: Needs review » Reviewed & tested by the community

Yes, @longwave, these were thr two errors i pointed out. Thanks for the patch.
Thanks @ayushmishra206 for clarifying about rerolling the patch for both the branches, i was a bit confused.

Mentioned errors are fixed and Patch applied cleanly on both the branches i.e 9.0.x and 8.9.x. Moving it to RTBC.

alexpott’s picture

Title: [backport] Fix grammar, spelling, and style of the code comments in FormBuilder::prepareForm() » Fix grammar, spelling, and style of the code comments in FormBuilder::prepareForm()
Version: 9.0.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed

Backported to 8.9.x to keep the code aligned. This makes security patches for example a bit easier.

Committed and pushed 4414343867 to 9.0.x and 31c7fb41ff to 8.9.x. Thanks!

  • alexpott committed 4414343 on 9.0.x
    Issue #3040274 by sarvjeetsingh, ayushmishra206, rishabhthakur, alexpott...

  • alexpott committed 31c7fb4 on 8.9.x
    Issue #3040274 by sarvjeetsingh, ayushmishra206, rishabhthakur, alexpott...

Status: Fixed » Closed (fixed)

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