Restrict new hook attributes to use on methods, drop $method parameter

Link dependency issue.

Setting to "Needs review" to get some feedback.
Do not merge yet!

I also wonder about the usefulness of `$module` parameter for Preprocess.
It will affect the order, and it can be used to make the implementation conditional on other modules.
But other than that?

+1 from me. I can’t imagine wanting a separate class for each form ID you want to alter, and wanting an __invoke() method on each one.

Removing this parameter, at least from FormAlter and Preprocess, would also help minimize the yuckiness of 📌 [pp-2] Clean up method default to use NULL for Hook Attributes Active . 😉

It is there because it is patterned after AsEventListener, the class level could already be deprecated and removed in 12 , it's not really needed.

I think there is something to be said for keeping them close so it's easier to switch between a and I'm not sure it's that much more complex, but I don't have a super strong opinion on this.

If we do remove this do we want to remove it from all hooks?

If I am being honest the only novel thing it provides is the ability to specify another class and method as the implementation, but I'm not sure why you'd need to do that.

As for the main reason is so you can place the attribute on the class, but then you have to have an __invoke method. Since we already have that restriction, why not drop method entirely, then if someone does not want a specially named class they use __invoke which already have to do.

If we do remove this do we want to remove it from all hooks?

We can't remove it in #[Hook], it breaks BC in two scenarios:

• Modules which actually put this attribute on a class.
• Modules which pass '' to $method, to then use non-named argument for $module. E.g. #[Hook('hook_name', '', 'node')].

So we can only remove it in 12.x.
And even then it will be disruptive.

So we can only remove it in 12.x.
And even then it will be disruptive.

Yep that's what I meant, we could handle that pretty easily.

Modules which actually put this attribute on a class.

Yes I mentioned that, but what is the advantage there over putting out on the method directly?
If you put it there without the method you have to have invoke.

Modules which pass '' to $method, to then use non-named argument for $module. E.g. #[Hook('hook_name', '', 'node')].

We can handle that.

Why would we want to remove method from extensions but not the root hook attribute.
Honestly I think we want to keep them in sync, either keep the parameter on all our remove it from all.

Why would we want to remove method from extensions but not the root hook attribute.

Simply because we can remove it from subclasses, but removing from the main class is a BC break.
(unless we do some ugly tricks)

Honestly I think we want to keep them in sync, either keep the parameter on all our remove it from all.

I would agree that keeping them in sync could be desirable.

However, if we keep the $method parameter on all the subclasses, later we will have to remove it from all of them.
Whichever solution we find to clean up the main Hook class, will then have to be duplicated to all the subclasses.

My argument here is to remove it from the subclasses while we still have the chance.
Then for the main Hook class we can remove it when we are allowed to break things.

For the record:
If we wanted to change the signature of #[Hook] before 12.x, we could do this:

#[\Attribute(\Attribute::TARGET_CLASS | \Attribute::TARGET_METHOD | \Attribute::IS_REPEATABLE)]
class Hook implements HookAttributeInterface {

  public readonly ?string $method;

  public readonly ?string $module;

  public readonly ?OrderInterface $order;

  /**
   * Constructs a Hook attribute object.
   *
   * @param string $hook
   *   The short hook name, without the 'hook_' prefix.
   * @param \Drupal\Core\Hook\Order\OrderInterface|string|null $order
   *   (optional) Set the order of the implementation.
   *   For legacy support, a string can be passed to this parameter.
   * @param mixed ...$legacy_args
   *   Additional named or indexed arguments for legacy support.
   */
  public function __construct(
    public readonly string $hook,
    OrderInterface|string|null $order = NULL,
    mixed ...$legacy_args,
  ) {
    if (is_string($order)) {
      @trigger_error('Passing a method name as the second parameter to #[Hook] is deprecated.', E_USER_DEPRECATED);
      $method = $order;
      $order = $legacy_args[3] ?? NULL;
    }
    else {
      if (array_key_exists('method', $legacy_args)) {
        @trigger_error('Passing a method name to #[Hook] is deprecated.', E_USER_DEPRECATED);
      }
      $method = $legacy_args['method'] ?? NULL;
    }
    if ($method === '') {
      $method = NULL;
    }
    $this->method = $method;
    if (array_key_exists('module', $legacy_args) || array_key_exists(2, $legacy_args)) {
      @trigger_error('Passing a module name to #[Hook] is deprecated.', E_USER_DEPRECATED);
    }
    $this->module = $legacy_args['module'] ?? $legacy_args[2] ?? NULL;
    $this->order = $order;
  }

}

I suspect that not everybody will like this.

Also, if we go this way, I would strongly suggest to not use Hook as a base class for Preprocess and FormAlter, but instead introduce a separate class HookBase.

Preprocess and FormAlter are gone we should pivot this to the base Hook attribute.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Initial title and summary pivot. Haven’t touched the MR

Preprocess and FormAlter are gone we should pivot this to the base Hook attribute.

We need to be very careful with this.
As soon as we change the signature, we have to add some verbose and ugly code for BC support.
Then if we want to change it again in the future, e.g. to insert a new parameter, or to remove a parameter, we will have to add even more such code.
I think we should avoid any such change for 12.2.x.
Until 12.3.x we have more time to think about how we want this hook system to look like in the future.

I assume you mean 11.2 and 11.3, but yes, in my mind this is purely 11.3 territory.

I think this will conflict with the effort to allow hooks elsewhere, the plan is to only scan the classes i think so we need the method parameter.