triggerErrorForDuplicateAlterHookListener() incorrectly triggers for forms with identical base_form_id and form_id

Created on 3 July 2025, 6 days ago

Problem/Motivation

The new duplicate alter hook detection mechanism introduced in #3485896 πŸ“Œ Hook ordering across OOP, procedural and with extra types Active 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.

πŸ› Bug report
Status

Active

Version

11.0 πŸ”₯

Component

extension system

Created by

πŸ‡­πŸ‡ΊHungary mxr576 Hungary

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @mxr576
  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary
      /**
       * {@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.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    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.

  • πŸ‡©πŸ‡ͺGermany donquixote

    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;
        }
    
  • πŸ‡©πŸ‡ͺGermany donquixote

    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.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    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 forest impression without deep consideration.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
Production build 0.71.5 2024