#[ReOrderHook] crashes on non-implemented hook.

Probably major.

This is at least major. A case could be made for critical.

Can we add some explicit assertions about the existence of the non-existent hook fixture definition in the registry or whatnot? Otherwise the file could be removed without tests failing. Thanks!

I talked myself into critical. This mostly only affects custom code or rather advanced contrib modules that integrate with other contrib modules, which would usually limit the scope to major, but if someone hits this on any one of the many hooks that act on CRUD operations it could affect data integrity or even something triggered in a post-update hook (and therefore the upgrade path).

@nicxvan pointed out that we should also add test coverage that the hook order is correct with or without the missing hook.

Happy to leave this critical.

However,

Post update hooks are not oop.

Also this blows up in the compiler pass, I'm not sure how data integrity will be affected.

The bigger problem might be modules installed hooks not running.

Re 10, not quite what I meant.

I meant #[Hook('some_hook', order: OrderAfter(classesAndMethods: MissingClass::non_existent_hook_method'))]

Post update hooks are not oop.

I meant things triggered by them. If they themselves were being ordered by this mechanism I would've definitely marked it critical. (I checked that first.) But yeah, let's leave aside what exactly happens when or whether this is possible. The base issue (and the case for critical) is that there are so many different ways this could break advanced usecases that it's hard to rule out the possibility of the "definitely critical" categories of issue (security, data integrity, upgrade path). So let's err on the side of the higher priority which we do when it's borderline in general.

I'm not opposed to this being critical.

I had a chance to investigate why workspace using this on content moderation doesn't blow up.

It turns out that this needs to be an alter hook. It has to try ordering runtime, which means this IS critical since it can blow up during runtime, not collection.

We only pack the operations when it's for an alter. I think when gathering order operations either for packing or applying we should check that the class exists.

In the case of Reorder if the target doesn't exist we should just outright drop it. When OrderBefore or OrderAfter targets don't exist I think that is safe, but we should confirm we have a test.

Regarding my comment on dropping Reorder @donquixote made this comment in slack:

one interesting aspect is that these reorder operations are still applied on multi type alter calls
... we need to keep it. it can target a method registered for a different hook legitimately
the impl can exist for a different hook.
so you target a form_alter impl in the scope of a form_form_id_alter
this currently works.

I don't think that is quite right, ReorderHook can only target one implementation, if that implementation doesn't exist then it's rules should not apply at all.

Specifically:

so you target a form_alter impl in the scope of a form_form_id_alter

but if the form_form_id_alter doesn't exist then no ordering should happen, if the module want's to also order the form_alter they should target that separately.

This is really about targeting a hook that (currently) has no implementations.
If you target a hook that does have implementations, but the targeted implementation does not (currently) exist, there is no problem.

Also it is not "fatal" per se: A TypeError is catchable, it is up to Drupal whether to catch it.

I don't think that is quite right, ReorderHook can only target one implementation, if that implementation doesn't exist then it's rules should not apply at all.

Specifically:

so you target a form_alter impl in the scope of a form_form_id_alter

but if the form_form_id_alter doesn't exist then no ordering should happen, if the module want's to also order the form_alter they should target that separately.

The "if the form_form_id_alter doesn't exist" does not mean anything.
A hook may never be called or it may have no implementations.
There is no such a thing for a hook as to "not exist".

During discovery, we can only determine whether a hook has implementations or not.
But it may still be called.

#[ReorderHook('form_myform_alter', self::class, 'formAlter2', Order::First)]
class MyHooks {
  #[Hook('form_alter')]
  public function formAlter1(array &$form): void {
    $form['#alter_calls'][] = __METHOD__;
  }
  #[Hook('form_alter')]
  public function formAlter2(array &$form): void {
    $form['#alter_calls'][] = __METHOD__;
  }
}

class MyTest extends KernelTestBase {
  public function testFormAlterReorder(): void {
    $this->assertFormAlterCallOrder([
      MyHooks::class . '::formAlter1',
      MyHooks::class . '::formAlter2',
    ], 'form');
    $this->assertFormAlterCallOrder([
      MyHooks::class . '::formAlter2',
      // The #[ReOrderHook] rule is applied if 'form_myform' is within the alter types.
      MyHooks::class . '::formAlter1',
    ], ['form', 'form_myform']);
  }
}

Currently this will cause the TypeError for which this issue exists.
But we can easily craft a version of this scenario that does not crash: We simply add an implementation for hook_form_myform_alter().

#[ReorderHook('form_myform_alter', self::class, 'formAlter2', Order::First)]
class MyHooks {
  #[Hook('form_alter')]
  public function formAlter1(array &$form): void {
    $form['#alter_calls'][] = __METHOD__;
  }
  #[Hook('form_alter')]
  public function formAlter2(array &$form): void {
    $form['#alter_calls'][] = __METHOD__;
  }
  #[Hook('form_myform_alter')]
  public function formMyformAlter(array &$form): void {
    $form['#alter_calls'][] = __METHOD__;
  }
}

class MyTest extends KernelTestBase {
  public function testFormAlterReorder(): void {
    $this->assertFormAlterCallOrder([
      MyHooks::class . '::formAlter1',
      MyHooks::class . '::formAlter2',
    ], 'form');
    $this->assertFormAlterCallOrder([
      MyHooks::class . '::formAlter2',
      MyHooks::class . '::formMyformAlter',
      // The #[ReOrderHook] rule is applied if 'form_myform' is within the alter types.
      MyHooks::class . '::formAlter1',
    ], ['form', 'form_myform']);
  }
}

Haven't fully read 17, but this only happens for alter calls, this does not occur as far as I tested for non alter hooks.

Haven't fully read 17, but this only happens for alter calls, this does not occur as far as I tested for non alter hooks.

The "steps to reproduce" show how this applies to non-alter hooks.

Simply add this attribute (and nothing else) on a class in a Hook namespace:

use Drupal\Core\Hook\Attribute\ReorderHook;
use Drupal\Core\Hook\Order\Order;

#[ReorderHook('non_implemented_hook', 'NonExistingClass', 'nonExistingMethod', Order::Last)]

(don't forget the imports, or it won't work)

It also does not matter if the target class and method exist:
- If the hook has (other) implementations, but the target class and method do not exist, there is no error.
- If the hook has no implementations, but the target class and method do exist, just registered for a different hook (or no hook), we do get the TypeError during discovery.

So it had to be a class without an invoke method I suspect.

As part of my testing I enabled workspaces without content moderation and it did not fail.
I then changed the use and class name for content moderation hooks to a non existent class and nothing failed.

I did not dig further, but why would my test not fail?

Ok, I had a long discussion over slack with @donquixote.

I had misunderstood initially that this was about the target implementation for an ordering operation no longer existing. That is not the case so we don't have to worry about OrderBefore / After etc. This truly is about ReorderHook only.

This is when there are no remaining implementations for a hook when ReorderHook is processed. Workspaces doesn't blow up because there are other entity_presave implementations in core.

The question that now remains is how to appropriately handle this.

Legacy behavior
hook_module_implements_alter
This was a separate hook that executed independent of anything else. The ordering rules applied where applicable especially with extra types.

Modern behavior
Current
order directives (whether on #[Hook] or #[ReorderHook] after collection are independent of the attribute / implementation they were defined on or before. In this way they are much like HMIA that they were designed to replace.

My open question is do we want to preserve that behavior long term. My original thinking was that since order directives are defined in a hook implementation (or in the case of reorder, there is a target implementation) if the hook is removed then the order directives would no longer apply.

I would posit that if a hook implementation that has an order directive (whether directly on the Hook attribute or indirectly with ReorderHook) gets removed somehow the ordering directive would be removed as well.

@donquixote disagrees, but I don't see a situation where implementations that don't exist can affect other implementation ordering.

To put it more explicitly after further discussion on slack.

In my opinion, the ordering rules should only be applied or packed for runtime application if the target class and method are known for the defined target hook.

That means in the following cases ordering operations would be dropped:

#[Hook('test_hook', order: OrderBefore(['some_module']))] and there is a #[RemoveHook] elsewhere that targets that implementation

#[ReorderHook('test_hook', classAndMethod, order: OrderBefore(['some_module']))] and the class and method do not exist or were removed somehow (like with a #[RemoveHook] attribute)

If the hook implementation defining an order operation does not exist, then it should not affect any ordering whatsoever.
If you need to affect order of something no matter what you can do that with dummy hooks or other workarounds.

This may technically be a change in behavior from 11.2, but I think the fact that ordering operations were sticky is a bug that should be fixed rather than behavior we should preserve. Further, this is incredibly niche and rare. Anyone implementing #[ReorderHook] should be able to handle a change here.

Same with ordering operations in general.

So the main difference is this:

• Currently, a #[ReorderHook] targets an implementation (class + method) whenever it is invoked with the specified hook as part of the hooks/types passed to the ->invokeAll() or ->alter() call.
• Instead, nicxvan suggests that a #[ReorderHook] should target an implementation (class + method) only if it is registered with the specified hook.
  This means the scenario in #17 would no longer be supported.

The distinction between "invoked with" and "registered with" is relevant only in multi-type alter calls like ->alter('form', 'form_myform').
An implementation is (typically) registered for only one hook, but the alter call can add more hooks into the mix.

The proposed change would a BC break.
The time to suggest this would have beeen pre 11.2.0 :)

What I would propose is to only fix the error case, and make it align with the non-error case.

If I understand correctly, the current behaviour that @nicxvan is arguing to remove and that @donquixote is arguing to retain, is that if you try to affect the order relative to a hook that doesn't exist, this still affects the order of other hooks that do exist. To me this sounds unexpected and a bug, that we can fix in a patch release - since it's a behaviour change it will need a CR, but I disagree it's a 'bc break' this seems like an extreme edge case and undocumented behaviour.

if you try to affect the order relative to a hook that doesn't exist, this still affects the order of other hooks that do exist.

Not really - unless I misunderstand.

When I see "order relative to", I think of #[ReorderHook(..., new OrderBefore(...))].
This is not what this is about.

The best example is in comment #17.

As you know, ->alter() can be called with more than one "alter type", which then maps to more than one alter hook.
E.g. ->alter(['form', 'form_node_form', 'form_node_article_form'], ..).
The hooks are 'form_alter', 'form_node_form_alter', 'form_node_article_form_alter'.

Each of these hooks can have order operations that were declared either with #[ReorderHook], or by setting an order in the #[Hook] attribute for a specific implementation.
All of these order operations will be applied when the implementations from these multiple hooks are "merged" at runtime.
When this happens, an order operation from e.g. 'form_node_form_alter' is allowed to target and reorder an implementation of 'form_alter'.

How?

Most of the #[ReorderHook] will target an implementation of the same hook that is passed in the first parameter of the attribute.
E.g.
- #[ReorderHook('form_alter', MyClass::class, 'formAlter', Order::First)], or
- #[ReorderHook('form_node_form_alter', MyClass::class, 'formNodeFormAlter', Order::Last)],
assuming that ::formAlter() implements hook_form_alter(), and ::formNodeFormAlter() implements hook_form_node_form_alter().

However, a #[ReorderHook] can target (and thus reorder) an implementation from a different hook.
This operation is only applied when both hooks are used together in an ->alter() call.

E.g. a completely silly example would be completely unrelated hooks #[ReorderHook('mail_alter', SomeClass::class, 'toolbar', Order::First)], assuming that SomeClass::toolbar() implements hook_toolbar().
These are never used together in ->alter(), and the second one ('toolbar') is not even an alter hook.
Therefore this #[ReorderHook] is completely pointless and has no effect.

However, we can #[ReorderHook('form_node_form_alter', MyClass::class, 'formAlter', Order::First)].
Here the target hook is 'form_node_form_alter', but the target method implements 'form_alter'.

What could be the intent behind this?

• For any other form, when ->alter(['form', 'form_other_form'], ..) is called, MyClass::formAlter() should be in its regular order position, between other implementations.
• For 'node_form', when ->alter(['form', 'form_node_form'], ..) is called, MyClass::formAlter() should be first, before other implementations.

Currently, this works as described _if_ there is at least one implementation of hook_form_node_form_alter().
This implementation could be from a different module that we don't care about.
If there is no such implementation, we get the TypeError as reported in this issue.

The new version is a bit verbose but hopefully more clear.

The test-only branch illustrates the current behavior.