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

CommentFileSizeAuthor
#13 add_proper_return_tags-2470936-13.patch1.52 KBgoogletorp
#11 add_proper_return_tags-2470936-11.patch1.55 KBgoogletorp
#8 add_proper_return_tags-2470936-8.patch1.58 KBgoogletorp
#7 interdiff_7.txt596 bytesgoogletorp
#7 add_proper_return_tags-2470936-7.patch1.58 KBgoogletorp
#5 drupal_core-add_missing_return_tags-2470936-5.patch1.55 KBmeramo
#1 drupal_core-add_missing_return_tags-2470936-1.patch1.2 KBmeramo
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

meramo’s picture

meramo CreditAttribution: meramo commented
Status: Active » Needs review
FileSize
drupal_core-add_missing_return_tags-2470936-1.patch1.2 KB

Sending patch for review

Reno Greenleaf’s picture

Reno Greenleaf CreditAttribution: Reno Greenleaf commented

Reviewed. Looks good.

googletorp’s picture

googletorp CreditAttribution: googletorp commented
Status: Needs review » Reviewed & tested by the community

Also reviewed, think this is RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work 
  1. +++ b/core/lib/Drupal/Core/Ajax/CssCommand.php
@@ -61,6 +61,9 @@ public function __construct($selector, array $css = array()) {
+   *
+   * @return array
+   *   The property/value pair.

    This returns $this 

  2. +++ b/core/lib/Drupal/Core/Asset/CssOptimizer.php
@@ -220,6 +220,9 @@ protected function processCss($contents, $optimize = FALSE) {
+   *
+   * @return string
+   *   The prefixed path.

    Afaics this returns the file path.

meramo’s picture

meramo CreditAttribution: meramo commented
Status: Needs work » Needs review
FileSize
drupal_core-add_missing_return_tags-2470936-5.patch1.55 KB

Comments amended. There are still a lot of inconsistencies in the documentation, but it least we'll have the returns documented everywhere.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/Ajax/CssCommand.php
@@ -61,6 +61,9 @@ public function __construct($selector, array $css = array()) {
+   *   The property/value pair.

Incorrect description. Also, when using @return $this, the description may be left out completely.

googletorp’s picture

googletorp CreditAttribution: googletorp commented
Status: Needs work » Needs review
FileSize
add_proper_return_tags-2470936-7.patch1.58 KB
interdiff_7.txt596 bytes

Rerolled the patch with proper documentation.

googletorp’s picture

googletorp CreditAttribution: googletorp commented
FileSize
add_proper_return_tags-2470936-8.patch1.58 KB

Add missing \ in previous patch

googletorp’s picture

googletorp CreditAttribution: googletorp commented
Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/Ajax/CssCommand.php
@@ -61,6 +61,9 @@ public function __construct($selector, array $css = array()) {
+   * @return \Drupal\Core\Ajax\CommandInterface
+   *   The called command.

This must be @return $this.

googletorp’s picture

googletorp CreditAttribution: googletorp commented
Status: Needs work » Needs review
FileSize
add_proper_return_tags-2470936-11.patch1.55 KB

With $this it is :)

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/Ajax/CssCommand.php
@@ -61,6 +61,9 @@ public function __construct($selector, array $css = array()) {
+   * @return $this
+   *   The called command.

No description is needed, because @return $this can 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).

googletorp’s picture

googletorp CreditAttribution: googletorp commented
Status: Needs work » Needs review
FileSize
add_proper_return_tags-2470936-13.patch1.52 KB

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

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Status: Needs review » Reviewed & tested by the community

Looks good. Thanks!

meramo’s picture

meramo CreditAttribution: meramo commented

I wasn't aware of the $this trick as well, thanks everyone for the help!
Looks good!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Assigned: meramo » jhodgdon

I think that's right, but sending to jhodgdon to confirm.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Assigned: jhodgdon » Unassigned

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

  • xjm committed b9c64cf on 8.0.x 
    Issue #2470936 by googletorp, meramo, Xano: Add proper @return tags to...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed

Thanks @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!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.