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

  /**
   * {@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.

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.

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;
    }

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.

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.

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

Related discussions happening in 🐛 Order of alter hooks is not always respected by ModuleHandler Needs work .

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

I found this new issue while working on this one:
🐛 #[ReOrderHook] crashes on non-implemented hook. Active

Good find!

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'].

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?

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.