Problem/Motivation
As minor as it sounds, but after reviewing the core for missing @return tags in PHPDoc comments we found only 2 (two) cases reported by phpcs, so why not to fix that.
Affected files are:
- /core/lib/Drupal/Core/Ajax/CssCommand.php
- /core/lib/Drupal/Core/Asset/CssOptimizer.php
Proposed resolution
Add tags
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | add_proper_return_tags-2470936-13.patch | 1.52 KB | googletorp |
| #11 | add_proper_return_tags-2470936-11.patch | 1.55 KB | googletorp |
| #8 | add_proper_return_tags-2470936-8.patch | 1.58 KB | googletorp |
| #7 | interdiff_7.txt | 596 bytes | googletorp |
| #5 | drupal_core-add_missing_return_tags-2470936-5.patch | 1.55 KB | meramo |
Comments
Comment #1
meramo commentedSending patch for review
Comment #2
Reno Greenleaf commentedReviewed. Looks good.
Comment #3
googletorp commentedAlso reviewed, think this is RTBC.
Comment #4
alexpottThis returns
$thisAfaics this returns the file path.
Comment #5
meramo commentedComments amended. There are still a lot of inconsistencies in the documentation, but it least we'll have the returns documented everywhere.
Comment #6
xanoIncorrect description. Also, when using
@return $this, the description may be left out completely.Comment #7
googletorp commentedRerolled the patch with proper documentation.
Comment #8
googletorp commentedAdd missing \ in previous patch
Comment #9
googletorp commentedComment #10
xanoThis must be
@return $this.Comment #11
googletorp commentedWith $this it is :)
Comment #12
xanoNo description is needed, because
@return $thiscan only mean one thing (the object on which the method was called). See the handbook for more information (and we copied it from PhpDoc again).Comment #13
googletorp commented@Xano Thanks for clearing this up and the review, I wasn't aware of the special @return $this.
Updated in the patch, removed the description after @return $this.
Comment #14
xanoLooks good. Thanks!
Comment #15
meramo commentedI wasn't aware of the $this trick as well, thanks everyone for the help!
Looks good!
Comment #16
webchickI think that's right, but sending to jhodgdon to confirm.
Comment #17
jhodgdonI can confirm that the formatting of the documentation added lines is correct. I have not reviewed them for accuracy, but others have done that. +1.
Comment #19
xjmThanks @jhodgdon for confirming. This issue only improves documentation, so per https://www.drupal.org/core/beta-changes, this can be completed any time during the Drupal 8 beta phase.
I checked over both methods' return values and the documentation appears accurate. (Though, yowza at
CssOptimizer::loadNestedFile()generally, but improving the inline docs isn't in scope here.)Committed and pushed to 8.0.x. Thanks everyone!