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?

Comments

just_like_good_vibes created an issue. See original summary.

just_like_good_vibes’s picture

Issue summary: View changes
tim.plunkett’s picture

Version: 8.4.x-dev » 8.5.x-dev
Priority: Minor » Normal
Status: Active » Postponed (maintainer needs more info)

Can you share your implementation of hook_module_implements_alter() as well as the actual names of the form_alter hooks you are implementing?

just_like_good_vibes’s picture

StatusFileSize
new1.41 KB

hello,
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)

just_like_good_vibes’s picture

StatusFileSize
new1.47 KB
just_like_good_vibes’s picture

StatusFileSize
new1.47 KB
just_like_good_vibes’s picture

the test "core/modules/system/tests/src/Functional/Form/AlterTest.php" failed.
I can see this inside the test :

// Ensure that the order is first by module, then for a given module, the
    // id-specific one after the generic one.
    $expected = [
      'block_form_form_test_alter_form_alter() executed.',
      'form_test_form_alter() executed.',
      'form_test_form_form_test_alter_form_alter() executed.',
      'system_form_form_test_alter_form_alter() executed.',
    ];

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?

just_like_good_vibes’s picture

hello,

any hints/comments ? thanks

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

chi’s picture

Status: Postponed (maintainer needs more info) » Active
Related issues: +#765860: drupal_alter() fails to order modules correctly in some cases

The 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().

$this->getImplementations() can only return ordered implementations of a single hook.

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.

  • A_form_alter()
  • A_form_FORM_ID_alter()
  • B_form_alter()
  • B_form_FORM_ID_alter()

I think it would make more sense to sort alter functions by hook type first.

  • A_form_alter()
  • B_form_alter()
  • A_form_FORM_ID_alter()
  • B_form_FORM_ID_alter()
chi’s picture

A note from the documentation.

Note that hooks invoked using \Drupal::moduleHandler->alter() can have multiple variations(such as hook_form_alter() and hook_form_FORM_ID_alter()). \Drupal::moduleHandler->alter() will call all such variants defined by a single module in turn. For the purposes of hook_module_implements_alter(), these variants are treated as a single hook. Thus, to ensure that your implementation of hook_form_FORM_ID_alter() is called at the right time, you will have to change the order of hook_form_alter() implementation in hook_module_implements_alter().

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().

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

mxr576’s picture

Status: Active » Closed (duplicate)

Closing 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.