Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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().
Comment | File | Size | Author |
---|---|---|---|
#10 | change-hook-implementation-comment-2714375-10.patch | 5.7 KB | apaderno |
#7 | change-hook-implementation-comment-2714375-7.patch | 546 bytes | apaderno |
#5 | 2714375.patch | 1.09 KB | imalabya |
Comments
Comment #2
jhodgdonThe comment should still say which form it is altering (perhaps by giving the class name?).
Comment #3
apadernoI 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 ofhook_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.
Comment #4
jhodgdonThat 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.
Comment #5
imalabyaAdded an initial patch.
Comment #6
imalabyaComment #7
apadernoComment #8
jhodgdonThe 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
Comment #9
apadernoWhoops, I was producing the patch as per object. I will reroll it.
Comment #10
apadernoComment #11
jhodgdonLooks great, thanks!
Comment #12
apadernoIf 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.
Comment #13
jhodgdonMost 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.
Comment #16
catchCommitted/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!