In FormBuilder::prepareForm()
there are the following code comments that need to be fixed.
-
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).
-
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.
-
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 isint $count
.This can be replaced by a single sentence.
Generate a public token and a placeholder based on the form ID.
-
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.
-
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.
-
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.
Comment | File | Size | Author |
---|---|---|---|
#59 | interdiff.3040274.55-59.txt | 1.11 KB | longwave |
#59 | 3040274-59.patch | 7.79 KB | longwave |
#57 | 9.0.x.jpg | 14.59 KB | ayushmishra206 |
#57 | 8.9.x.jpg | 14.54 KB | ayushmishra206 |
#56 | diff_48-55.txt | 1.07 KB | sarvjeetsingh |
Comments
Comment #2
apadernoComment #3
apadernoComment #4
apadernoComment #5
apadernoComment #6
apadernoI noticed just now that api.drupal.org has now links for Drupal 8.8.x documentation.
Comment #7
apadernoComment #8
kwfinken CreditAttribution: kwfinken at Michigan State University for Michigan State University commentedWorking on this at MidCamp 2019.oops, commented on wrong tab of browser.Comment #9
kkalaskar CreditAttribution: kkalaskar at Asentech LLC commentedHello @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
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.
Comment #10
apadernoI made a quick review, and this is what I found.
If I recall correctly, the description of a class property doesn't need to be split in two lines.
The last item of a list should not have any period.
There should not be a period even after the
@see
directive.It would be better something similar to the following one.
Comment #11
gringoinc CreditAttribution: gringoinc commentedI'm working on this Issue at Drupalcon2019 Seattle.
Comment #12
gringoinc CreditAttribution: gringoinc as a volunteer commentedHi everyone, I'm in Seattle at Drupalcon, and I was reviewing what was commented by @kiamlaluno and I found the following:
I eliminated the line break, but the "CODESNIFFER" would throw me error of maximum characters per line.
I eliminated the period.
I eliminated the period, and the blank line.
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,
Comment #13
theotherlondonHi Everyone! I'm working on this issue at DrupalCon.
Comment #14
theotherlondonI 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.
Comment #15
awardsolutions CreditAttribution: awardsolutions as a volunteer commentedRead through this. Looks good!
Comment #16
kdebisschop CreditAttribution: kdebisschop at SciShield commentedI 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?
Comment #18
amarphule CreditAttribution: amarphule at DevsAdda for DevsAdda commentedComment #19
amarphule CreditAttribution: amarphule at DevsAdda for DevsAdda commentedFixed grammar, spelling, and style of the code comments
Comment #20
apadernoThis is a quick review; there could be other changes to make.
In a list, none of the items have a period at the end, including the last item.
The empty comment between @see and @todo is correct.
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.).
This patch should just fix the content of comments, not how code is formatted.
Comment #21
volkswagenchickTagging for DrupalNorth 2019
Comment #22
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedI have modified.
Comment #23
volkswagenchickTagging for badcamp2019, thanks! (October 2-5)
Comment #26
kkalashnikov CreditAttribution: kkalashnikov at Srijan | A Material+ Company for Drupal India Association commentedComment #27
kkalashnikov CreditAttribution: kkalashnikov at Srijan | A Material+ Company for Drupal India Association commented#22 patch applied successfully for Drupal 9.1 as well.
Good to move to RTBC
Comment #28
kkalashnikov CreditAttribution: kkalashnikov at Srijan | A Material+ Company for Drupal India Association commentedComment #29
nitvirus CreditAttribution: nitvirus at Srijan | A Material+ Company commentedHi,
#22 patch applies successfully checked again. Changing the status to RTBC
Comment #30
nitvirus CreditAttribution: nitvirus at Srijan | A Material+ Company commentedunassigning
Comment #31
nitvirus CreditAttribution: nitvirus at Srijan | A Material+ Company commentedComment #32
alexpottThis is not an improvement. It does not define all the element value callables.
How come we put string on the next line? The sentence that starts "The placeholder" is very much a follow-on from the next.
If we're doing this then we can remove the line that says
// Generates a placeholder based on the form ID.
Comment #33
rishabhthakur CreditAttribution: rishabhthakur as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #34
rishabhthakur CreditAttribution: rishabhthakur as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedModified last Patch and also fix some coding standard issues.
Comment #35
tim.plunkettYou cannot use @see inline.
ID
Either ajax or AJAX, not Ajax.
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.
Comment #36
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedChanging status to needs work as per #34 and #35.
Comment #37
rishabhthakur CreditAttribution: rishabhthakur as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedmodified as per last comment #35
Comment #38
davidhernandezIs this an improvement? I'm not sure what this sentence is trying to say. Should it say "callables to skip" ?
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.
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.
Comment #39
sarvjeetsingh CreditAttribution: sarvjeetsingh as a volunteer and at QED42 for Drupal India Association commentedComment #40
sarvjeetsingh CreditAttribution: sarvjeetsingh as a volunteer and at QED42 for Drupal India Association commentedI have updated the patch as per the above suggestions.
please review. Thanks!
Comment #41
apadernoThe existing text is more correct. There isn't any need to change it.
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 needs the verb in third person plural (require).
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.
Comment #42
apadernoAs 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 still1
.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.
Comment #43
sarvjeetsingh CreditAttribution: sarvjeetsingh as a volunteer and at QED42 for Drupal India Association commentedThanks @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.
Comment #44
apadernoI apologize, I reviewed the patch completely, I noted a change that should be fixed, but I didn't report it in my previous comment.
Instead of adding a period where it should not be, I would remove the full comment.
The comment doesn't help in understanding the code line following it.
Comment #45
sarvjeetsingh CreditAttribution: sarvjeetsingh as a volunteer and at QED42 for Drupal India Association commentedComment #46
sarvjeetsingh CreditAttribution: sarvjeetsingh as a volunteer and at QED42 for Drupal India Association commentedUpdated the patch with recommended changes.
please review.
Comment #47
apadernoThe patch has been changed as suggested.
Comment #48
alexpottAs we're fixing spellings here we need to regenerate the dictionary.
Comment #49
larowlanSaving issue credits
Comment #51
larowlanCommitted 97cff66 and pushed to 9.1.x. Thanks!
Needs reroll for earlier versions
Comment #52
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedComment #53
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedRerolled for 9.0.x, Please review.
Comment #54
quietone CreditAttribution: quietone as a volunteer commentedThe 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.
Comment #55
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedRerolled the patch for both 8.9 and 9.0. Please review.
Comment #56
sarvjeetsingh CreditAttribution: sarvjeetsingh as a volunteer and at QED42 for Drupal India Association commentedThe 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.
Comment #57
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commented@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.
Comment #58
apadernoYes, the core/misc/cspell/dictionary.txt exists only in the 9.1.x branch. Previous branches don't have that file.
Comment #59
longwaveFixed the two errors noted in #56. This should apply to both 8.9.x and 9.0.x.
Comment #60
sarvjeetsingh CreditAttribution: sarvjeetsingh as a volunteer and at QED42 for Drupal India Association commentedYes, @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.
Comment #61
alexpottBackported 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!