Fix grammar, spelling, and style of the code comments in FormBuilder::prepareForm()

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

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.

I'm working on this Issue at Drupalcon2019 Seattle.

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,

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

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.

Read through this. Looks good!

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?

Fixed grammar, spelling, and style of the code comments

Tagging for DrupalNorth 2019

I have modified.

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

#22 patch applied successfully for Drupal 9.1 as well.

Good to move to RTBC

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

unassigning

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.

Modified last Patch and also fix some coding standard issues.

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.

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

modified as per last comment #35

-   * 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.

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

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.

Updated the patch with recommended changes.
please review.

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

Saving issue credits

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

Needs reroll for earlier versions

Rerolled for 9.0.x, Please review.

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.

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

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.

@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.

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

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.

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!