This is a sub-issue of #1800046: [META] Add missing type hinting to core docblocks, fix Drupal.Commenting.FunctionComment.Missing* focused on correctly adding @param and @return type hinting to the Image module.

Documentation patches that include type hinting are time consuming to both review and commit because one must dig into the actual code to confirm that the type hints are both correct and complete. Hence, please be patient and try to limit type hint patches to covering only a limited number of docblocks (10-15 as a guess).

How To Review This Issue

  1. Attempt to apply the patch to see if it needs a reroll.
  2. Use the phpcs one-liner to evaluate whether all the relevant standards errors have been resolved: https://gist.github.com/paul-m/227822ac7723b0e90647
  3. Look at each change and determine whether the type hint is correct.

Related sprint issues:

Sprint Topic Sub Issue
#1518116: [meta] Make Core pass Coder Review #1533246: Make image module pass Coder Review
#1310084: [meta] API documentation cleanup sprint #1326608: Clean up API docs for image module
#500866: [META] remove t() from assert message #1797328: Remove t() from assertion messages in tests for the image module
CommentFileSizeAuthor
#22 interdiff.txt8.01 KBMile23
#22 1802060_22.patch4.08 KBMile23
#20 core-changed_Image_module_docblocks-1802060-20.patch8.55 KBsuhel.rangnekar
#12 1802060_11.patch7.7 KBMile23
#7 add-missing-type-hinting-to-image-module-docblocks-1802060-#7.patch7.86 KBrishikant05
#4 interdiff.txt1.09 KBMile23
#4 1802060_4.patch7.73 KBMile23
#2 1802060_2.patch7.54 KBMile23
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Issue summary: View changes
Parent issue: » #1800046: [META] Add missing type hinting to core docblocks, fix Drupal.Commenting.FunctionComment.Missing*
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Active » Needs review
FileSize
1802060_2.patch7.54 KB

Start here. :-)

Status: Needs review » Needs work

The last submitted patch, 2: 1802060_2.patch, failed testing.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Needs work » Needs review
FileSize
1802060_4.patch7.73 KB
interdiff.txt1.09 KB

Got it wrong in ImageFieldTestBase::previewNodeImage(), fixed here.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented

Still applies.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Issue summary: View changes
rishikant05’s picture

rishikant05 CreditAttribution: rishikant05 at Srijan | A Material+ Company commented
Issue tags: +india, +SprintWeekend2015
FileSize
add-missing-type-hinting-to-image-module-docblocks-1802060-#7.patch7.86 KB

Rerolling the patch because #4 patch is not applied.

deepakaryan1988’s picture

deepakaryan1988
Primary language Hindi
CreditAttribution: deepakaryan1988 at Srijan | A Material+ Company commented
Status: Needs review » Reviewed & tested by the community

Looking good to me!

xjm’s picture

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

These patches should only add missing type hinting to documentation blocks only, not do any other cleanups. The scope creep of adding other changes do these patches makes them more difficult to review. Long reference as to why: http://webchick.net/please-stop-eating-baby-kittens

Also, before RTBCing, did you carefully check each function affected to ensure the added types were correct in all usages? Thanks @deepakaryan1988 and @Mile23!

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue tags: -india, -SprintWeekend2015

@rishikant05 Thanks for working on this.

Sprint Weekend https://groups.drupal.org/node/447258 was in January, so this should not be tagged with that, since that event is past.

Also, note the guidelines on using tags:
"Before adding tags read the issue tag guidelines: https://www.drupal.org/node/1023102 . Do NOT use tags for adding random keywords or duplicating any other fields.."

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Assigned: Unassigned » Mile23

Working on it.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Needs work » Needs review
FileSize
1802060_11.patch7.7 KB

I just did the work over rather than try to remove code changes, so there's no interdiff.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Assigned: Mile23 » Unassigned
deepakaryan1988’s picture

deepakaryan1988
Primary language Hindi
CreditAttribution: deepakaryan1988 at Srijan | A Material+ Company commented

@xjm I did check on my local machine.
I did one round of testing but I think I was mistaken. In future, I will be more careful before changing the status to RTBC. Thanks for the pointing out. :)

deepakaryan1988’s picture

deepakaryan1988
Primary language Hindi
CreditAttribution: deepakaryan1988 at Srijan | A Material+ Company commented
Status: Needs review » Needs work

@xjm @Mile23 I did test on patch #12 and it's working fine.
Should I change the status to RTBC?

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented

Part of the way this works is that a maintainer (like @xjm) isn't so likely to come and review unless someone marks it as RTBC. :-)

But be sure and look at it to see if it's complete and acceptable.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Needs work » Needs review

Also, if you're not going to RTBC, you should still allow it to be reviewed, by leaving the 'needs review' status. :-)

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Version: 8.0.x-dev » 8.1.x-dev
Status: Needs review » Needs work
Issue tags: +Needs reroll

Bumping to 8.1.x. Patch no longer applies.

suhel.rangnekar’s picture

suhel.rangnekar CreditAttribution: suhel.rangnekar as a volunteer and at Faichi Solutions Pvt Ltd commented
Assigned: Unassigned » suhel.rangnekar

working..

suhel.rangnekar’s picture

suhel.rangnekar CreditAttribution: suhel.rangnekar as a volunteer and at Faichi Solutions Pvt Ltd commented
Assigned: suhel.rangnekar » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
core-changed_Image_module_docblocks-1802060-20.patch8.55 KB

I have updated the patch against 8.1.x-dev branch.
Please make a review on attached patch.

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer and at Acquia for Acquia commented
Version: 8.1.x-dev » 8.0.x-dev

As per @jhodgdon in one of the similar issue, all the documentation changes so they are BC compatible and we can push them to 8.0.x

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
FileSize
1802060_22.patch4.08 KB
interdiff.txt8.01 KB

Thanks. :-)

phpcs says this:

$ phpcs --standard="Drupal" --extensions="module/php,php" --report-csv core/modules/image/ | grep -F $'Missing param\nReturn type missing'
"/Users/paul/pj2/drupal/core/modules/image/src/Tests/Migrate/d6/MigrateImageCacheTest.php",160,6,error,"Missing parameter type",Drupal.Commenting.FunctionComment.MissingParamType,5,0
"/Users/paul/pj2/drupal/core/modules/image/src/Tests/Migrate/d6/MigrateImageCacheTest.php",162,6,error,"Missing parameter type",Drupal.Commenting.FunctionComment.MissingParamType,5,0

So there are a couple changes needed in MigrateImageCacheTest.php.

There are a bunch of out-of-scope changes #20, but I realize I added them originally. :-)

Here's an in-scope patch.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs review » Closed (duplicate)
Related issues: +#3207949: Fix Drupal.Commenting.FunctionComment.MissingParamType

This work is now being done by sniff. The work here is now in #3207949: Fix Drupal.Commenting.FunctionComment.MissingParamType so closing this as a duplicate. I've identified credit to add over there, let me know if I got it wrong.