This is part of this meta issue: Make flag module pass coder review

In this issue, we plan to fix the missing type hinting from '@return' and '@param' positions. We DO NOT aim to add any new doc blocks here.

CommentFileSizeAuthor
#7 interdiff-5-7.txt2.26 KBcs_shadow
#7 flag-add_missing_type_hintings-2245609-7.patch22.15 KBcs_shadow
#5 flag-add_missing_type_hintings-2245609-5.patch22.19 KBcs_shadow
#1 flag-add_missing_type_hintings-2245609-1.patch22.18 KBcs_shadow
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cs_shadow’s picture

cs_shadow CreditAttribution: cs_shadow commented
Status: Active » Needs review
FileSize
flag-add_missing_type_hintings-2245609-1.patch22.18 KB

Add type hinting for several @params and @returns. Need review one this one to proceed further.

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Needs review » Postponed

Argh, this is probably going to clash with the work I'm doing at #2202969: clean up flag() and its low-level helpers. I'd rather postpone it until that's in.

cs_shadow’s picture

cs_shadow CreditAttribution: cs_shadow commented

Maybe this issue can be opened now, as the blocking issue has been closed.

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Postponed » Needs work

Yup. It'll need a reroll, sorry!

cs_shadow’s picture

cs_shadow CreditAttribution: cs_shadow commented
Status: Needs work » Needs review
FileSize
flag-add_missing_type_hintings-2245609-5.patch22.19 KB

Re-rolled the patch in #1.

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Needs review » Needs work

Thanks for rerolling. There's a few mistakes in the patch, however:

  1. +++ b/flag.api.php
@@ -238,14 +238,14 @@ function hook_flag_access($flag, $entity_id, $action, $account) {
+ * @param int $entity_ids

    This is an array. 

  2. +++ b/flag.module
@@ -59,12 +59,12 @@ function flag_entity_info() {
- * @return
+ * @return DrupalEntityControllerInterface

    This doesn't look right. 

  3. +++ b/flag.module
@@ -515,7 +515,7 @@ function flag_hook_info() {
- * @param $entity_type
+ * @param string|NULL $entity_type

    Do we need to say NULL for an optional parameter? Isn't that implicit by it being optional? 

  4. +++ b/flag_actions.module
@@ -136,17 +136,17 @@ function flag_actions_get_actions($flag_name = NULL, $reset = FALSE) {
- * @param $fid
+ * @param flag_flag $fid

    It's an integer not an object. 

  5. +++ b/flag_actions.module
@@ -136,17 +136,17 @@ function flag_actions_get_actions($flag_name = NULL, $reset = FALSE) {
- * @param $callback
+ * @param string $callback

    Callable? Though I don't know if this actually takes a full PHP callable -- would need to check the function code. 

  6. +++ b/flag_actions.module
@@ -165,15 +165,15 @@ function flag_actions_insert_action($fid, $event, $threshold, $repeat_threshold,
- * @param $aid
+ * @param flag_flag $aid

    This doesn't look right. 

  7. +++ b/includes/flag/flag_flag.inc
@@ -461,16 +461,16 @@ class flag_flag {
-   * @param $account
+   * @param string|NULL $account
    *   The user on whose behalf to test the flagging action. Leave NULL for the

    This is a user account object, not a string. 

  8. +++ b/includes/flag/flag_flag.inc
@@ -530,14 +530,14 @@ class flag_flag {
-   * @param $account
+   * @param string|NULL $account

    Same here: user account entity.

cs_shadow’s picture

cs_shadow CreditAttribution: cs_shadow commented
FileSize
flag-add_missing_type_hintings-2245609-7.patch22.15 KB
interdiff-5-7.txt2.26 KB

Fixed the errors mentioned in #6. Attaching patch and interdiff.

cs_shadow’s picture

cs_shadow CreditAttribution: cs_shadow commented
Status: Needs work » Needs review
joachim’s picture

joachim CreditAttribution: joachim commented
Status: Needs review » Fixed

Committed, with a minor reroll where a couple of hunks failed.

Thanks, and sorry it's taken me ages to get back to this!

  • joachim committed 2d63bab on 7.x-3.x authored by cs_shadow 
    Issue #2245609 by cs_shadow: Added type hinting to function...
cs_shadow’s picture

cs_shadow CreditAttribution: cs_shadow commented

Thanks for committing :) Better late than never.

Status: Fixed » Closed (fixed)

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