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.
I noticed that book_entity_info_alter() and hook_entity_info_alter() had different parameters ($entity_info vs $info), and that bothered me.
Digging through the hooks I noticed a couple more that were different (hook_menu_link_alter() vs user_menu_link_alter()).
And about half of the hook_form_FORM_ID_alter()s were missing the $form_id parameter.
Comment | File | Size | Author |
---|---|---|---|
#26 | drupal-1209786-26.patch | 45.36 KB | dman |
#24 | drupal-1209786-24.patch | 47.58 KB | underq |
#22 | drupal-1209786-22.patch | 47.6 KB | underq |
#20 | drupal-1209786-20.patch | 48.41 KB | underq |
#18 | drupal-1209786-18.patch | 48.35 KB | underq |
Comments
Comment #1
tim.plunkettAttached.
Comment #2
tim.plunkettOkay, 34KB is pretty big, this is split into two patches, one adding $form_id to form_alters, the other with the parameter name tweaks.
Comment #4
tim.plunkettFixed the failure, should work this time.
Comment #5
chx CreditAttribution: chx commentedWhile i like unifying variable names in the first patch and giving people a reminder that form_id is readily available in the second patch, I can't agree with taking everything by reference just because we can. In fact, it's safer not to -- dont take anything by ref unless you must.
Comment #6
sunnot all hook implementations have to take all parameters.
typical example is hook_theme() and hook_theme_registry_alter() -- only edge-case implementations really need the additional parameters
Do we have some bogus docs elsewhere?
The variable for hook_menu_link_*() should be $link, not $item, because it's a link, not a router item.
19 days to next Drupal core point release.
Comment #7
tim.plunkettNoted. Now it's just unifying names and adding $form_id. Nothing to break here!
Comment #8
tim.plunkettCross-post. Will read and follow up on #6.
Comment #9
tim.plunkettI fixed the bits about hook_menu_link_alter, but I agree with chx over sun in terms of including arguments like $form_id.
Paraphrasing myself from IRC, "newer developers often look to core implementations AS the documentation, core should set a good example"
Comment #10
tim.plunkettComment #11
tim.plunkettI still have to double check the following hooks:
Comment #13
tim.plunkettI don't personally have time to work on this, but it's still worthwhile.
Comment #14
ryan.gibson CreditAttribution: ryan.gibson commented@tim.plunkett I was going to help out and do this, but I am not quite sure where the discussion left off. Maybe there was something on IRC that clarified that I missed?
Comment #15
YesCT CreditAttribution: YesCT commentedHere is some clarification:
the patch contains some examples. more of this needs to be done.
For all those listed functions, find the definition, check the parameter list, and then compare it to the documentation for what the parameter list should be. Make changes to make them match.
Comment #16
YesCT CreditAttribution: YesCT commented#11: drupal-1209786-11.patch queued for re-testing.
Comment #18
underq CreditAttribution: underq commentedI reroll patch #11 to try to pass bot testing.
Comment #20
underq CreditAttribution: underq commentedI replace
by
Comment #22
underq CreditAttribution: underq commentedReroll
Comment #24
underq CreditAttribution: underq commentedRetry with last commit :)
Comment #25
YesCT CreditAttribution: YesCT commentedunderq, thanks for moving this forward!
Comment #26
dman CreditAttribution: dman commentedRe-roll due to the HUGE menu refactoring that just weet in #916388: Convert menu links into entities
- multiple offset changes.
core/modules/system/system.api.php implementations of hook_menu_alter() things : removed.
core/modules/user/user.module hook_menu_alter() removed.
locale_translate_file_directory was renamed into translation_path during another patch elsewhere, so that needed to be cleaned.
So, this may be a replacement for #24
Try it out testbot...
Comment #27
socketwench CreditAttribution: socketwench commentedNovice issue cleanup.
Comment #29
jhedstromAnother huge reroll will be needed. Note that some of these simpler ones may be caught by the coding standard cleanup, but the ones where hook implementations are missing parameters, or have misnamed parameters, will not be caught there.