Problem/Motivation

The new duplicate alter hook detection mechanism introduced in #3485896 is incorrectly triggering warnings for legitimate form alter hooks when a form's base_form_id and form_id are identical.

This issue manifests with forms like \Drupal\Core\Entity\Form\RevisionRevertForm where both the base form ID and form ID are the same value, causing the FormBuilder to generate duplicate hook candidates for the same alter hook (e.g., revision_revert_alter).

The error message displayed is:

Exception: User notice: The function f() is registered for more than one of the alter hooks ['form_alter', 'form_foo_revision_revert_alter', 'form_foo_revision_revert_alter'] from the current ->alter() call. Only one instance will be part of the implementation list for this hook combination.

Two additional issues are evident:

  • The function name in the error message shows as "f()" instead of the correct hook function name
  • The revision_revert_alter hook appears twice in the hooks array

Steps to reproduce

  1. Install Drupal 11.2.x
  2. Create a custom module that implements hook_form_alter() and hook_form_FORM_ID_alter() for a revision revert form
  3. Navigate to a revision revert form in the UI, or run tests that involve revision revert forms
  4. Observe the user notice being triggered by triggerErrorForDuplicateAlterHookListener()

Proposed resolution

The root cause is in the FormBuilder where it generates hook candidates from both base_form_id and form_id without checking for duplicates. When these IDs are identical, the same hook is added twice to the hooks array.

Potential solutions:

  • Primary fix: Add $hooks = array_unique($hooks) in FormBuilder before dispatching alter hooks to prevent duplicate hook names
  • Alternative: Modify the hook generation logic to skip adding the base_form_id hook when it's identical to the form_id
  • Debug improvement: Fix the error message generation to display the correct function name instead of "f()"

This fix should be applied broadly as this issue could affect any form where base_form_id equals form_id, not just RevisionRevertForm.

Remaining tasks

  • Identify all locations where duplicate hooks could be dispatched
  • Implement the fix in FormBuilder
  • Fix the error message generation to show correct function names
  • Add test coverage for forms with identical base_form_id and form_id
  • Review and test the fix

User interface changes

None. This is a bug fix that eliminates incorrect warning messages.

Introduced terminology

None.

API changes

None. This is an internal fix to prevent false positive warnings.

Data model changes

None.

Release notes snippet

Fixed false positive duplicate alter hook warnings for forms where base_form_id and form_id are identical, such as RevisionRevertForm.

Issue fork drupal-3533944

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mxr576 created an issue. See original summary.

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented 
  /**
   * {@inheritdoc}
   */
  public function getBaseFormId() {
    // Assign ENTITY_TYPE_form as base form ID to invoke corresponding
    // hook_form_alter(), #validate, #submit, and #theme callbacks, but only if
    // it is different from the actual form ID, since callbacks would be invoked
    // twice otherwise.
    $base_form_id = $this->entity->getEntityTypeId() . '_form';
    if ($base_form_id == $this->getFormId()) {
      $base_form_id = NULL;
    }
    return $base_form_id;
  }

Just found this workaround in \Drupal\Core\Entity\EntityForm::getBaseFormId() so somebody might have tried to address this problem before with a local fix.

nicxvan’s picture

nicxvan commented
Issue tags: +Needs framework manager review

We'll definitely need sign off on collapsing the alters.

I think it's best, but we need to ensure that there isn't some time that that behavior is essential.

I cannot think of any off the top of my head, but why was baseform trying to solve it there instead of in the alter in the first place.

donquixote’s picture

donquixote commented

The workaround exists since Drupal 9.0.* (or earlier).

Behavior in older Drupal versions

In Drupal prior to 11.2.x, when calling ->alter(['a', 'a'], ..) with the same alter type repeated, the implementations would simply be repeated.
The order was by module and then by alter type:

class HookAlterTest extends KernelTestBase {
  protected static $modules = [
    'procedural_hook_test',
    'system',
  ];
  public function testAlter(): void {
    $this->assertAlterCallOrder([
      'procedural_hook_test_test_main_alter',
      'procedural_hook_test_test_other_alter',
      'procedural_hook_test_test_main_alter',
      'procedural_hook_test_test_main_alter',
      'procedural_hook_test_test_other_alter',
      'system_test_main_alter',
      'system_test_main_alter',
      'system_test_main_alter',
    ], ['test_main', 'test_other', 'test_main', 'test_main', 'test_other', 'test_x']);
  }
  [...] (not sharing the full class here)

Behavior since 11.2.x

Since 11.2.x, every implementation is only executed once, and we see the notice as reported here.

  public function testAlter(): void {
    $this->assertAlterCallOrder([
      'procedural_hook_test_test_main_alter',
      'procedural_hook_test_test_other_alter',
      'system_test_main_alter',
    ], ['test_main', 'test_other', 'test_main', 'test_main', 'test_other', 'test_x']);
  }

However, both the deduplication and the notice were actually meant for a different case, where the same method is registered for two distinct alter hooks, which are then executed together with ->alter().
E.g. ->alter(['a', 'b'], ..) with #[Hook('a_alter')] and #[Hook('b_alter')] on the same method.
The case with ->alter(['a', 'a'], ..) was not really considered at the time. This is why the notice looks so strange.

Solution

Now we need to decide what should be the "correct" behavior.
I think it makes sense to execute every implementation only once.
But we don't want the same notice, if the duplication is caused by a duplicate alter type passed to ->alter().

The easiest might be to simply call array_unique() in ->alter().

class ModuleHandler ... {
  [..]
  public function alter($type, &$data, &$context1 = NULL, &$context2 = NULL) {
    [..]
    if (is_array($type)) {
      $type = array_unique($type);
      $cid = implode(',', $type);
    }
    else {
      $cid = $type;
    }
donquixote’s picture

donquixote commented

We'll definitely need sign off on collapsing the alters.

We should have asked for this before 11.2.0 release.
(And we should have added a test for this case)
Now if we change it back to have implementations repeated, we break with the behavior of 11.2.0.

nicxvan’s picture

nicxvan commented

Alternatively if we change it back we fix the behavior of 11.2.

However, since we have the ability to have multiple implementations there are ways to execute your alters multiple times if you really want to.

I suspect this was just a hidden misfeature for many years.

Calling the same alter multiple times similarly seems fundamentally wrong. You could likely achieve that with a second call but I fail to see the use case of duplicates alter runs.

I would advocate for array_unique but that is first impression without deep consideration.

nicxvan’s picture

nicxvan commented
Related issues: +#3521067: Explore whether multiple Hook attributes on the same method targeting different modules in the same alter call should escalate to an error
donquixote’s picture

donquixote commented

In addition to array_unique() we could deprecate passing a non-unique list, so that it will _also_ be fixed in all calling code.

donquixote opened merge request !12752

donquixote opened merge request !12753

donquixote opened merge request !12754

donquixote opened merge request !12756

donquixote’s picture

donquixote commented
Related issues: +#3120298: Order of alter hooks is not always respected by ModuleHandler

Related discussions happening in #3120298: Order of alter hooks is not always respected by ModuleHandler.

donquixote’s picture

donquixote commented

I created multiple MRs to demonstrate how the changes compare to current behavior.

donquixote’s picture

donquixote commented
Related issues: +#3536470: TypeError if #[ReOrderHook] targets hook that has no implementations anywhere in enabled modules.

I found this new issue while working on this one:
#3536470: TypeError if #[ReOrderHook] targets hook that has no implementations anywhere in enabled modules.

nicxvan’s picture

nicxvan commented

Good find!

donquixote’s picture

donquixote commented

Alternatively if we change it back we fix the behavior of 11.2.

We could still do that as a follow-up, if we really want to.
I think for now the least disruptive solution is to preserve the 11.2.x behavior while removing the notice.

Btw currently we use FormattableMarkup to produce that notice, and as a result we get ugly escaped html in the placeholder variables.
Any other trigger_error() call in core uses raw string operations.

I would advocate for array_unique but that is forest impression without deep consideration.

Btw I notice that it matters where we call array_unique().
If we call array_unique() directly in ->alter(), it will change how the #[ReOrderHook] order operations are applied.
If instead we call array_unique() in ->getCombinedListeners() to get the listener lists, but we use the original (non-unique) hook names for order operations, we can keep the current behavior.

However this distinction is quite theoretical.
Most of the relevant cases are with form alter, and here the pattern is only ever ['form', 'form_x', 'form_x'], so ['a', 'b', 'b'].
To reproduce the order operation glitch, we need a pattern like ['a', 'b', 'a'].

donquixote’s picture

donquixote commented

Imo we should merge the tests-only branch "3533944-alter-non-unique-current-behavior" before we do anything else.
https://git.drupalcode.org/project/drupal/-/merge_requests/12754/diffs
Maybe this needs a separate issue?

nicxvan’s picture

nicxvan commented

Trying to wrap my mind around all of these options.

Quick note:

Btw currently we use FormattableMarkup

yes, I wasn't a fan of adding that in the original issue, I'd never seen that, but the actual ordering was more important, I forgot the follow up for that though so thanks for calling that out here.

donquixote’s picture

donquixote commented

I opened #3540505: Add tests for alter hook calls with non-unique hook list (prepare for #3533944) so that we can add tests before we change any behavior.
The PR contains the changes from https://git.drupalcode.org/project/drupal/-/merge_requests/12754/diffs

nicxvan’s picture

nicxvan commented

When we're adding multiple approaches like this it would be helpful to have a brief summary in the issue summary outlining each MR.

The other issue with the tests is helpful too to simplify this.

nicxvan’s picture

nicxvan commented
Status: Active » Needs work
Issue tags: +Needs issue summary update

Needs work for 21.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.