Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Some files are using @inheritdoc instead of {@inheritdoc}. Update the code with the curly brackets.
| Comment | File | Size | Author |
|---|---|---|---|
| #24 | raw-interdiff-16-24.txt | 930 bytes | jungle |
| #24 | 3126957-24.patch | 14.91 KB | jungle |
Comments
Comment #2
Neslee Canil PintoComment #3
cilefen CreditAttribution: cilefen commentedComment #4
Kristen PolI'll take a look.
Comment #5
Kristen PolUpdating title per https://en.wikipedia.org/wiki/Bracket
Comment #6
Kristen PolThanks for the patch. I've reviewed as follows:
1) Found all files that had the missing curly brackets using grep (see list below).
2) Applied patch which applied cleanly.
3) Re-checked the files with the missing curly brackets using grep and the remaining files are:
but laminas and psr are 3rd party libraries so nothing can be done with those.
4) Reviewed the patch and it seems fine.
5) Did not do any manual testing since I'm not sure how it can be tested.
Marking RTBC. Thanks!
List of files without curly brackets for "@inheritdoc" before applying patch:
Comment #7
Kristen PolComment #8
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commented#2 patch got failed to apply. This is the new patch.
Comment #9
Kristen PolIt would be good to include an interdiff file and explain the changes made.
Comment #10
Kristen PolBoth patches applied cleanly for me, but the first patch applied "more cleanly" and the patch in #8 removed some code for fixing
editor.formattedTextEditor.es6.jsthat was in the patch from #2 so moving this back to "Needs work" though the patch from #2 is ok.@Suresh Prabhu Parkala It would be good to know what were your issues. We shouldn't add more patches if there isn't a need. Thanks.
Comment #11
Kristen PolAh, I see the failed patch applying message now in #2.
so the patch from #8 just needs to be updated to include
editor.formattedTextEditor.es6.jsas noted in #10.Comment #12
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedUpdated patch. Please review!
Comment #13
Kristen PolComment #14
Kristen PolThanks for the update.
@Suresh Prabhu Parkala It's good to add an interdiff when making incremental changes https://www.drupal.org/documentation/git/interdiff.
1) Compared patches from #8 and #12 and the only difference was the addition of
core/modules/editor/js/editor.formattedTextEditor.es6.js.2) Patch applies cleanly:
3) Searched the 9.0 dev codebase after applying the patch and there are no more references to
* @inheritdoc.4) Compared the patch in #2 to #12 and noticed:
but this is correct based on:
https://www.drupal.org/docs/develop/standards/api-documentation-and-comm...
Technically, it shouldn't be fixed in this issue unless we update the issue summary to include this. Then we should look for others with the same problem. I think it would be difficult to search for more of these. IMO, it's ok to fix this one for now.
5) Did a quick scan of the code and
inheritDocis used instead ofinheritdoc(note the casing) for:6) Marking back to "Needs work" for #5.
Comment #15
Kristen PolNote, I found a discussion about the
inheritdocvsinheritDocat #3060580: Allow inheritdoc and inheritDoc?. For now, it still should be all lowercase but that might change later.Comment #16
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedUpdated patch. Please review.
Comment #17
Kristen PolThanks for the update.
1) Confirmed changes are just the
inheritDoc=>inheritdoc.2) Patch applies cleanly:
3) There are other
inheritDoc(note case) after applying the patch but they aren't related to this issue.4) Marking RTBC based on this and previous reviews.
Comment #18
jungleIn addition to #17
The patch made changes to .es6.js files, in general, the corresponding .js files should be generated by running commands below
I did run it once, confirmed that no changes to .js files, as changes are made to comments, whilst comments are stripped out while building
Comment #19
xjmNice work on this!
There are a few other
@inheritdocthat are wrong in different ways. I found them by running:Can someone create a single followup issue to fix those?
Comment #20
xjmSaving issue credit.
Comment #23
xjmI reviewed the patch with
git diff --color-wordsto verify that only changes to add the curly braces are included. There's one place where we're deleting a redundant one-line summary along with correcting it to{@inheritdoc}, but since the patch is just 15K I'll let it slide to include that as well....Goodness does it take a bit to run the
yarn runchecks on all those files...Committed to 9.1.x and 9.0.x. Thanks!
The patch doesn't apply to 8.9.x, so I think we'll want a new patch for 8.9.x and probably 8.8.x. Marking as Patch to be ported for those backports.
Comment #24
jungle@xjm, thanks for committing!
Just did reroll the patch for 8.9.x from #16, and tested on my local that it's applied successfully to 8.8.x, with the minimal change without fixing #19, and with the raw-interdiff, I am setting back to RTBC. This should not be counted as self-RTBC :p.
Comment #25
jungleFollow-up issue filed for #19
Comment #26
jungleFrom the output @xjm posted in #19
We need to be careful to generate the new patch in the follow-up as MenuLinkContentTest.php is a binary file recognized. Mentioned it on the follow-up too.
#3126906: MenuLinkContentTest.php is recognized as a binary file by git is the ongoing issue related to it. Comments to there are welcome. Thanks!
Comment #27
jungleBTW. patch uploaded to the follow-up #3132287: Fix wrong usages of {@inheritdoc}, and testings against 8.8.x, 8.9.x, 9.0.x and 9.1.x passed. RTBC is expected if applicable. Thanks!
Comment #30
xjmCommitted the backports to 8.9.x and 8.8.x. Thanks @jungle!