Hello,
i just encounter a very special behavior today concerning the order of execution of form alter hooks.
Explanation :
i wanted to alter a form, already altered in a contrib module by a hook_form_FORM_ID_alter() .
I was unable to make my hook implementation execute after the other implementation, even when i implement the hook hook__module_implements_alter to enforce the execution of my particular hook at the end (the other module did not implement hook__module_implements_alter).
After investigation, i found the problem occurs in the following code inside the method "prepareForm " of core/lib/Drupal/Core/Form/FormBuilder.php
$hooks = ['form'];
if (isset($build_info['base_form_id'])) {
$hooks[] = 'form_' . $build_info['base_form_id'];
}
$hooks[] = 'form_' . $form_id;
$this->moduleHandler->alter($hooks, $form, $form_state, $form_id);
$this->themeManager->alter($hooks, $form, $form_state, $form_id);
the alter method of the ModuleHandler class is called with the three hooks as argument.
inside the method, a merge is done of all form alter hooks (defined by all enabled modules), and (in my install) the order is lost even when enforced by the implementation of hook__module_implements_alter.
Details about the manifestation of the unexpected behavior :
The module editor_advanced_link implements hook_form_FORM_ID_alter()with the function editor_advanced_link_form_editor_link_dialog_alter (altering the form "editor_link_dialog").
to refine the alteration of the form "editor_link_dialog", in a custom module, i also implemented hook_form_editor_advanced_link_form_editor_link_dialog_alter
and hook__module_implements_alter to ensure my hook would be executed after the one defined by the module editor_advanced_link.
Unfortunately order defined by implementing hook__module_implements_alter is lost inside moduleHandler->alter($hooks,...
Quick and dirty solution :
i replaced the code above by the following code, and everything it works (this is a quick and dirty solution).
$hooks = ['form'];
if (isset($build_info['base_form_id'])) {
$hooks[] = 'form_' . $build_info['base_form_id'];
}
// commented code
//$hooks[] = 'form_' . $form_id;
$this->moduleHandler->alter($hooks, $form, $form_state, $form_id);
$this->themeManager->alter($hooks, $form, $form_state, $form_id);
// added code
$this->moduleHandler->alter(['form_' . $form_id], $form, $form_state, $form_id);
$this->themeManager->alter(['form_' . $form_id], $form, $form_state, $form_id);
Indeed, i think hook_form_FORM_ID_alter should be executed after hook_form_alter hooks because they are more specific. There should be a way to enforce this in the code.
i think a more elegant solution would be needed to keep order of form alter hooks and the order of hooks depending on their scope (more or less specific).
maybe something like this or an update of the implementation of method "moduleHandler->alter"
// hook_form_alter
$this->moduleHandler->alter(['form'], $form, $form_state, $form_id);
$this->themeManager->alter(['form'], $form, $form_state, $form_id);
// hook_form_FORM_ID_alter
$this->moduleHandler->alter(['form_' . $build_info['base_form_id']], $form, $form_state, $form_id);
$this->themeManager->alter(['form_' . $build_info['base_form_id']], $form, $form_state, $form_id);
// hook_form_FORM_ID_alter
$this->moduleHandler->alter(['form_' . $form_id], $form, $form_state, $form_id);
$this->themeManager->alter(['form_' . $form_id], $form, $form_state, $form_id);
what do you think?
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | patch_issue_2917192.v1.patch | 1.47 KB | just_like_good_vibes |
Comments
Comment #2
just_like_good_vibesComment #3
tim.plunkettCan you share your implementation of
hook_module_implements_alter()as well as the actual names of the form_alter hooks you are implementing?Comment #4
just_like_good_vibeshello,
i've coded a very basic module to reproduce the issue.
i reproduced the issue with a standard install and just that simple module installed (editor_advanced_link is also installed to exhibit the issue).
i've pushed that module to github at https://github.com/mikaelmeulle/drupal_issue_2917192
i've created a very basic patch, i uploaded it..
(the patch seems to apply on 8.4.x and 8.5.x codebase)
thanks for the feedback
(edit: sorry i made a mistake when i first uploaded the patch, now it should be ok)
Comment #5
just_like_good_vibesComment #6
just_like_good_vibesComment #7
just_like_good_vibesthe test "core/modules/system/tests/src/Functional/Form/AlterTest.php" failed.
I can see this inside the test :
clearly my patch breaks this test. but it seems this test is not taking care of "hook_implements_alter" possible alterations, is-it?
my patch was too simple indeed... a proper handling of the issue i have raised would be to adapt the code inside "ModuleHandler->alter("
what is your opinion?
maybe this "AlterTest" would need some refinement ? do we really want hook execution order to follow alphabetic order of module names?
Comment #8
just_like_good_vibeshello,
any hints/comments ? thanks
Comment #11
chi commentedThe issue is just a particular case of general behavior of alter hooks. This is described very well in this comment.
https://www.drupal.org/project/drupal/issues/765860#comment-5301428
There is an important note about this in ModuleHandler::alter().
Which means hook_implementations_alter is invoked only for hook_form_alter() and never for hook_form_FORM_BASE_ID_alter() and hook_form_FORM_ID_alter().
Currently, even without hook_module_implements_alter() the order of alter functions is a bit odd.
It is sorted first by module, then by hook type.
I think it would make more sense to sort alter functions by hook type first.
Comment #12
chi commentedA note from the documentation.
That last sentence might seem as a workaround but it is not. Because if you change the order of hook_form_alter() without implementing it you will get RuntimeException from ModuleHandler::buildImplementationInfo().
Comment #15
mxr576Closing as duplicate of https://www.drupal.org/project/drupal/issues/3120298 where we have a working patch and an open discussion about the possible side-effects of fixing this issue.