This is a spin-off from #2608536: Some fixes for 'optional' parameter in core/include. While checking for optional @param directives, it was noted that several existing variable directives in the file core/includes/batch were missing type hints. This issue proposes to rectify that small problem.

CommentFileSizeAuthor
#4 2636980-4-type-hint-batch-inc-d8.patch2.35 KBLars Toomre
#4 interdiff-2636980-4-2.txt789 bytesLars Toomre
#2 2636980-2-type-hint-batch-inc-d8.patch2.57 KBLars Toomre
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Lars Toomre created an issue. See original summary.

Lars Toomre’s picture

Lars Toomre CreditAttribution: Lars Toomre as a volunteer commented
Status: Active » Needs review
FileSize
2636980-2-type-hint-batch-inc-d8.patch2.57 KB

Attached is a patch addressing missing type hints in three docblocks.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work 
+++ b/core/includes/batch.inc
@@ -367,17 +367,17 @@ function &_batch_current_set() {
+ * @return true|null
  *   TRUE if a subsequent set was found in the batch.

We could be pedantic and also change the docs line to say

... in the batch; no return value if not.

or something like that?

The rest of the patch looks good, thanks!

I think I'll set it to Needs Work to fix that docs line because it looks kind of funny to me, to say it is true|null (which is accurate) and then not say that in the docs.

Lars Toomre’s picture

Lars Toomre CreditAttribution: Lars Toomre as a volunteer commented
Status: Needs work » Needs review
FileSize
interdiff-2636980-4-2.txt789 bytes
2636980-4-type-hint-batch-inc-d8.patch2.35 KB

I think that was a good suggestion @jhodgdon in #3. I worded the thought slightly differently. Attached is an interdiff and updated patch incorporating your suggestion from #3.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Reviewed & tested by the community

Thanks! That looks good to me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed d23534f and pushed to 8.0.x and 8.1.x. Thanks!

  • alexpott committed 0c54ec8 on 8.1.x 
    Issue #2636980 by Lars Toomre, jhodgdon: Type hint additions in batch....

  • alexpott committed d23534f on 
    Issue #2636980 by Lars Toomre, jhodgdon: Type hint additions in batch....

Status: Fixed » Closed (fixed)

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