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 Action 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 #1816682: Make Action module pass Coder Review
#1310084: [meta] API documentation cleanup sprint #1787900: Clean up cruft in the Action documentation
#500866: [META] remove t() from assert message #1798954: Remove t() from test asserts in Action module [part of system tests in d7]
CommentFileSizeAuthor
#29 1802070_29.patch883 bytesMile23
#24 add_missing_type-1802070-24.patch1.62 KBalvar0hurtad0
#22 add_missing_type-1802070-22.patch5.14 KBalvar0hurtad0
#17 1802070_17.patch2.33 KBMile23
#14 1802070_14.patch1.62 KBMile23
#8 Add_missing_type_hinting_to_Action_module_docblocks-1802070-8.patch2.58 KBdeepakaryan1988
#6 interdiff.txt1.43 KBMile23
#6 1802070_6.patch3.24 KBMile23
#4 1802070_3_reroll.patch1.81 KBMile23
#3 core-action-module-type-hint-1802070-3.patch1.95 KBbaisong
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Lars Toomre’s picture

Lars Toomre CreditAttribution: Lars Toomre commented
Status: Active » Postponed

Marking this postponed until #1787900: Clean up cruft in the Action documentation is committed.

Lars Toomre’s picture

Lars Toomre CreditAttribution: Lars Toomre commented
Issue summary: View changes

Updated issue summary.

Lars Toomre’s picture

Lars Toomre CreditAttribution: Lars Toomre commented
Issue summary: View changes

Updated issue summary.

Lars Toomre’s picture

Lars Toomre CreditAttribution: Lars Toomre commented
Component: base system » action.module
Status: Postponed » Active

Setting this issue to active now, since there now is a report in #1816682-3: Make Action module pass Coder Review that aims to correct the coding style in the rest of the Action module.

Lars Toomre’s picture

Lars Toomre CreditAttribution: Lars Toomre commented
Issue summary: View changes

Updated issue summary.

baisong’s picture

baisong
he/they
Primary language English
Location Occupied Kumeyaay Land; San Diego, CA, US–Tijuana, BC, MX
CreditAttribution: baisong commented
Issue summary: View changes
Status: Active » Needs review
FileSize
core-action-module-type-hint-1802070-3.patch1.95 KB

I was pointed to this issue in core mentoring hours.

I added as many type hints as I could following these guidelines, and corrected 1 punctuation error (missing period).

Patch attached.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
FileSize
1802070_3_reroll.patch1.81 KB

Reroll.

The only problematic docblock is in EmailAction::_construct(). It's missing the parameter name for $mail_manager.

I say this rather than change it so I can do the review later.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
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
FileSize
1802070_6.patch3.24 KB
interdiff.txt1.43 KB

Caught the problems in #4, with a few others.

Mile23’s picture

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

deepakaryan1988
Primary language Hindi
CreditAttribution: deepakaryan1988 commented
FileSize
Add_missing_type_hinting_to_Action_module_docblocks-1802070-8.patch2.58 KB

Re-rolling the patch of @mile23 .

yogen.prasad’s picture

yogen.prasad CreditAttribution: yogen.prasad commented
Status: Needs review » Reviewed & tested by the community

Looking fine

deepakaryan1988’s picture

deepakaryan1988
Primary language Hindi
CreditAttribution: deepakaryan1988 commented
Status: Reviewed & tested by the community » Needs review
deepakaryan1988’s picture

deepakaryan1988
Primary language Hindi
CreditAttribution: deepakaryan1988 commented
Status: Needs review » Reviewed & tested by the community
xjm’s picture

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

Thanks @yogen.prasad and @deepakaryan1988. Did you carefully review each function to ensure that the typehints were correct in all cases?

These patches should only add missing type hinting, 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

+++ b/core/modules/action/src/Plugin/Action/EmailAction.php
@@ -53,13 +53,13 @@ class EmailAction extends ConfigurableActionBase implements ContainerFactoryPlug
-  /** The language manager.
+  /** The language manager service.

This is not the correct docblock format, but it doesn't belong in this patch anyway. :) See above.

deepakaryan1988’s picture

deepakaryan1988
Primary language Hindi
CreditAttribution: deepakaryan1988 commented

Ahh @xjm Thanks for pointing out..
will keep in mind in future!

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Status: Needs work » Needs review
FileSize
1802070_14.patch1.62 KB

I re-did the work rather than try to modify #8.

A couple of small out-of-scope items are still in there, because they modify the same file as in-scope issues.

There are instructions for reviewing this issue in the issue summary.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/modules/action/action.api.php
@@ -8,7 +8,7 @@
+ * @param mixed $aid
  *   The action ID.

Don't we know the action ID is a string (or maybe an integer?)

Mile23’s picture

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

I couldn't find it. The example feeds it to a DB function requiring mixed.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Version: 8.0.x-dev » 8.1.x-dev
FileSize
1802070_17.patch2.33 KB

Reroll of #14.

Still not sure about the type of $aid, because it's not documented anywhere.

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer and at Acquia for Acquia commented

I suspect its integer

Status: Needs review » Needs work

The last submitted patch, 17: 1802070_17.patch, failed testing.

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer and at Acquia for Acquia commented
Issue tags: +Needs reroll

The last submitted patch, 17: 1802070_17.patch, failed testing.

alvar0hurtad0’s picture

alvar0hurtad0
he/him
Primary language Spanish
Location Cáceres
CreditAttribution: alvar0hurtad0 at ASPgems commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
add_missing_type-1802070-22.patch5.14 KB

here's the reroll of #17

alvar0hurtad0’s picture

alvar0hurtad0
he/him
Primary language Spanish
Location Cáceres
CreditAttribution: alvar0hurtad0 at ASPgems commented
Assigned: Unassigned » alvar0hurtad0
Status: Needs review » Needs work

I think, my patch removes to much things, I'm reviewing it.

alvar0hurtad0’s picture

alvar0hurtad0
he/him
Primary language Spanish
Location Cáceres
CreditAttribution: alvar0hurtad0 at ASPgems commented
Status: Needs work » Needs review
FileSize
add_missing_type-1802070-24.patch1.62 KB

Sorry for my previous patch this is the reroll of #17

The last submitted patch, 22: add_missing_type-1802070-22.patch, failed testing.

Mile23’s picture

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

In IRC w/ @chx we discussed that action.api.php is a) wrong, and b) isn't needed because we already document hook_ENTITY_TYPE_delete().

If action.api.php had any action module specific information it would be worth salvaging but it doesn't.

Let's revert the changes to action.api.php in this patch, and then have a follow-up about deleting the file in another issue.

Mile23’s picture

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

The follow-up #2663830: Remove action.api.php

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Again, please remove the out-of-scope changes. See: https://www.drupal.org/core/scope#merge

(Don't be caught by the fallacy that if it's on the same line or an adjacent line, you should fix it in the same patch to avoid conflicts. Remember that a word diff can be used to review changes within lines, and that an unrelated change on the same line or adjacent line will still be missing accurate information of "why" in the commit log. Instead, create a followup issue.)

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Version: 8.1.x-dev » 8.0.x-dev
Status: Needs work » Needs review
FileSize
1802070_29.patch883 bytes

Needed a reroll anyway. Removes out of scope changes.

Changing to 8.0.x because that's what we've done on the other similar child issues. The patch applies cleanly to 8.0.x and 8.1.x.

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.

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented
Status: Needs review » Closed (duplicate)

This has been fixed as part of #2721901: Fix Drupal.Commenting.FunctionComment.MissingParamName