API page: https://api.drupal.org/api/drupal/core%21modules%21path%21path.module/fu...

Since the function the form builder is not anymore the node_form() function, should not the function have the following comment?

/**
 * Implements hook_form_BASE_FORM_ID_alter().
 */

This is the same comment used for system_form_user_form_alter().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kiamlaluno created an issue. See original summary.

jhodgdon’s picture

The comment should still say which form it is altering (perhaps by giving the class name?).

apaderno’s picture

I was thinking to use the form ID returned from FormInterface::formID(), but I see that not all the form classes override that method.
The form ID is already be in the hook name, though. Since path_form_node_form_alter() is said to be an implementation of hook_form_BASE_FORM_ID_alter(), it should be already self-documented which base form the hook is altering. The problem is eventually how easily is to find the class implementing the form knowing the form ID.

It seems we can only use the form class. And to make it easier for users reading the documentation, the same comment should be used for the implementations of hook_form_FORM_ID_alter().
So, the comments for those hooks I was referring in my OP should be the following.

/**
 * Implements hook_form_BASE_FORM_ID_alter() for \Drupal\node\NodeForm.
 */
/**
 * Implements hook_form_FORM_ID_alter() for \Drupal\user\AccountForm.
 */
jhodgdon’s picture

That looks good to me. In D7 and D6, we tried to get these hooks to be documented with the function name (so it would make a link). In 8, the class name makes sense.

imalabya’s picture

Added an initial patch.

imalabya’s picture

Status: Active » Needs review
apaderno’s picture

jhodgdon’s picture

Status: Needs review » Needs work

The patch in #5 is more complete than the patch in #7. But I think #5 is still incomplete. Files that need a similar fix:
book.module
editor.module
forum.module
path.module
menu_ui.module
seven.theme

apaderno’s picture

Whoops, I was producing the patch as per object. I will reroll it.

apaderno’s picture

Title: path_form_node_form_alter() is still commented as "Implements hook_form_BASE_FORM_ID_alter() for node_form()." » hook_form_alter() implementations are still commented with a reference to a form builder that doesn't exist anymore
Status: Needs work » Needs review
FileSize
5.7 KB
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks great, thanks!

apaderno’s picture

If the patch goes in, I guess the Drupal coding standards should be updated as well. Last time I read them, there was a part saying how to document hook implementations.

As secondary question, does the patch need to be done also for the 8.1.x branch, or such a change should be done only for the latest branch? I am not sure if this change should be considered equivalent of an API change, in some way.

jhodgdon’s picture

Most likely, the patch will also be applied to 8.1.x when it is committed.

Regarding standards, here is what we have:
https://www.drupal.org/node/1354#hookimpl

I guess we could add something there saying to reference the relevant function or class. The example is for a function that generates the form.

  • catch committed 0b1a646 on 8.2.x
    Issue #2714375 by kiamlaluno, malavya: hook_form_alter() implementations...

  • catch committed 4fb316f on 8.1.x
    Issue #2714375 by kiamlaluno, malavya: hook_form_alter() implementations...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

Status: Fixed » Closed (fixed)

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