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.
https://drupal.org/coding-standards/docs#order states
a blank line before the first parameter and a blank line after the last parameter before the @return section starts
, and gives an order for docblock elements.
The attached patch adds space mostly before @return elements, and shifts a few @see to other parts of the docblock according to the coding standards defined order.
Comment | File | Size | Author |
---|---|---|---|
#13 | interdiff-2029033-4-10.txt | 847 bytes | dang42 |
#10 | add_simpletest_docblock_spacing_fix_order_10.patch | 15.88 KB | dang42 |
#4 | add_simpletest_docblock_spacing_fix_order_4.patch | 15.35 KB | shnark |
add_simpletest_docblock_spacing_fix_order.patch | 15.97 KB | deekayen | |
Comments
Comment #1
deekayen CreditAttribution: deekayen commentedComment #2
jhodgdonSuper! Easy-to-review patch and it all looks good to me.
I'll get it committed when the test turns green (just in case). Thanks!
Comment #3
jhodgdonThis patch does not apply today.
Comment #4
shnark CreditAttribution: shnark commentedThere were a couple of conflicts, there were descriptions that were line wrapped in the latest version of core. Also, one of the @returns had a typehint. I resolved them.
Comment #5
dang42 CreditAttribution: dang42 commentedI've done the following:
* Reviewed the API documentation requirements for docblocks
* Reviewed the patch in #4
* The changes made by the patch all comply with the API requirements and are all within the scope of the issue summary
* Applied the patch to a local install
Yet to complete:
* Review patched files to confirm that all non-compliant instances have been resolved
I should be able to get this done sometime tonight...
-------------------------
Note - in the following patched file:
core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
there is some extra whitespace / lines (lines 591-593). This is outside the scope of the issue, but should be fixed...?
Note 2 - thank you very much to YesCT for helping me with my first issue cue review, and all the mentors at the Nerdery / 2013 TwinCities Drupal Camp!!!
Comment #6
dang42 CreditAttribution: dang42 commentedTestBase.php:
Other than the whitespace issue noted in #5 above, I don't see any problems with either spacing or section order in the docblocks.
WebTestBase.php:
1 - a space needs to be added after line 2259
2 - a space needs to be removed between two @param tags at line 3363
Tests/SimpleTestTest.php:
No spacing or section order issues. However, I think I might have found another bit to be fixed (based on the API info found here).
Lines 247-251 of core/modules/simpletest/lib/Drupal/simpletest/Tests/SimpleTestTest.php:
The syntax notes for the @param tag state that
Is this technically a "spacing" issue? Is it an issue at all? If it is a problem, and it is appropriate to fix it here, I'll be happy to do so.
--------------------
I am a complete noob at this issue que stuff and didn't get a chance to learn today (well, yesterday...) at the sprint how to create a patch. I'll do my best to figure it out on my own and put it up here in the next couple of days - and if I haven't figured it out by the time the D8 core mentoring starts on Wednesday, I'll pop in there to ask for help... :)
Comment #7
jhodgdonThis patch is limited to making sure there is a blank line between @param and @return, and that the sections are in the right order, as stated in the issue summary. There are many other documentation standards issues in these files, unfortunately, but the patches get really big if we try to fix them all at once and they're hard to review.
Anyway, thanks for the review! It sounds like within the scope of this issue, this is RTBC according to narragansett, so I'll change the status and see about committing it shortly.
Comment #8
dang42 CreditAttribution: dang42 commentedGot it - thanks for clarifying that for me!
Given that clarification and stripping out all the off-topic issues I mentioned, there are still two fixes to be made to one of the files included in the patch shown in #4:
WebTestBase.php:
1 - a space needs to be added after line 2259 to separate the @param & @return tags
2 - a space needs to be removed between two @param tags at line 3363
Everything else should be good to go...
Comment #9
jhodgdonAs per #8, two small changes need to be made, so updating status accordingly. Thanks!
Comment #10
dang42 CreditAttribution: dang42 commentedSo, after a little bit of 'core mentoring' from "valthebald" (thank you very much!), I did the following:
* made the necessary changes to WebTestBase.php
* ran 'git diff' to get the correct output for those two changes
* manually copied the output into the correct spots in the patch file from #4 above
* tested the new patch to be sure it still works - and it does...
* renamed it to add_simpletest_docblock_spacing_fix_order_10.patch and attached it here
I don't know how attribution works on this kind of thing, so to be clear:
The patch uploaded here is essentially EllaTheHarpy's - I only added two more changes, I did not remove anything. This is all Ella's work, with a couple of additions from me... :)
If I need to do anything else, or do something differently, just let me know and I'll be glad to make it happen!
Comment #11
jhodgdonWe try to give credit to everyone who contributed to a patch. :)
The only other thing that would be helpful here is if you could also provide an "interdiff" along with your patch.
Comment #12
jhodgdonBut don't bother with an interdiff now -- I just reviewed the patch in #10 and it all looks good. Thanks! I'll get it committed after the bot turns green.
Comment #13
dang42 CreditAttribution: dang42 commentedAsk and ye shall receive...
ETA - I created & uploaded it before I saw your second reply.
That's alright - I needed to figure out how to do it for future reference anyway... ;)
Comment #14
jhodgdonYes, good practice. :)
Comment #15
alexpottCommitted 5a66f5d and pushed to 8.x. Thanks!