Support for OO hooks implementations in classes

The biggest change is that you define an implementation using the Hook attribute.

You can define it for a whole class with a method, or a method only.

There is also a LegacyHook attribute for tagging existing procedural implementations so contrib can support both styles to support multiple version of core. (essentially before and after this change).

Also note eventually modules can extend hook for their own Hook implementations.

Note this is only on the implementation side so it's about detecting when core implements it's own hooks.

The referenced issue only converted two implementations as POC. There are followups that will convert core implementations, but those will not start until the referenced issue is merged.

In this issue I will attempt to store class methods marked with the #[Hook('foo')] attribute the same way as functions marked with Implements hook_foo are stored.

In a followup, we need to support Drupal\Core\Hook\Attribute\FormAlter The current state of affairs is not working for a static parser. Drupal\Core\Hook\Attribute\Hook constructor needs to change: $this->hook = static::PREFIX . $form_id . static::POSTFIX so every hook attribute extending this one will hopefully just override the class constants and have no code at all. php-parser supports constant expression evaluation so this is doable. nicxvan, if you are reading this please either do the change or at least file a followup so it gets done.

In the next followup, we need to make the doxygen on say CommentInsert to be stored the same place where the doxygen for hook_comment_insert is stored currently.

After that, we could once again turn to constant expression evaluation, and as the "Unsupported expressions and evaluator fallback and details we could handle ConstFetch and ClassConstFetch because api already picks up every constants in the parsed codebase. So we could support #[Hook(SomeClass::FOO)] or even an entire constant expression there. That would be cool.

Now the yapping is done, let me see what I can code here.

Somewhere a skip for #[LegacyHook] needs to happen.

You mean just in the documentation right?

I'll look into your suggestion:

Drupal\Core\Hook\Attribute\Hook constructor needs to change: $this->hook = static::PREFIX . $hook . static::POSTFIX so every hook attribute extending this one will hopefully just override the class constants and have no code at all

And copy over your comment on FormAlter.

Yes. I do not think there's much of a point of api picking up #[LegacyHook] functions. These will be 99% automatically generated, skipped by Drupal 11.1, there's just no value in documenting them. There's no change to the core MR here. It already does the skip in HookCollectorPass and ModuleHandler::invoke.

I am not 100% what's best for FormAlter, I trust y'all will figure it out. I mean, it's not used, it's a demo of what can be, it should be totally fine to finetune it in followups or skip it for now. The Hook class does need to change to use constants either now or later but it needs to change.

Take a look at the changes I made. I think it doesn't quite solve the issue you raised here.

I think there might be value in renaming $hook to $form_id for the FormAlterId attribute, let me know what you think.

My concern is api now because I made a promise I will help here. I am done with core.

I thought having Drupal\Core\Hook\Attribute\Cron extends Hook so the hook implementation looks like #[Cron] is nice DX and would have the side effect of providing a convenient space for hook doxygen. Truth to be told, when I suggested separate classes I was unaware of just how powerful an attribute is -- like being able to add constants and concats there and until yesterday I had no idea php-parser can evaluate constant expressions. So it's possible to just define a class constant somewhere, perhaps in Drupal\modulenamespace\Api or anywhere, even, wrap it in @addtogroup hooks and be done. Like #[Alter(FormBuilder::FORMALTER . FormClass::FORMID)] today and #[Alter(FormBuilder::FORMALTER . FormClass::class)] tomorrow when procedural hooks are gone would work just as well -- I still think Alter should be a separate thing especially because that's something fabianx suggested, not me.

So perhaps I was hasty with suggesting PREFIX/SUFFIX and that also should be just reverted, FormAlter removed and the whole debacle of hook DX + doxygen home moved into followups. Whatever core lands on as long as it forms a constant expression it can be supported by api and I will be here to turn that "can" into "does" (as long as no one wants me to run cspell on tests...).

Sorry, when I mentioned take a look I meant as far as the api documentation to make this issue work.

I've added a new IMPLEMENTS constant that I think will help.

FormAlterById becomes:

#[\Attribute(\Attribute::TARGET_CLASS | \Attribute::TARGET_METHOD | \Attribute::IS_REPEATABLE)]
class FormAlterById extends Hook {
  public const IMPLEMENTS = 'hook_form_FORM_ID_alter';

  public function __construct(
    string $form_id,
    public string $method = '',
    public ?int $priority = NULL,
    public ?string $module = NULL,
  ) {
    $hookName = "form_{$form_id}_alter";
    parent::__construct($hookName, $method, $priority, $module);
  }

}

Once again, whatever it is, we can support it. Just let me know where it ends.

This issue is progressing nicely.

  #[Hook('test56')]
  public function test56Implementation() {

Becomes:

Give me a little more time to fix a few broken things and write an automated test.

Thanks for working on this.

I know that the issues is still not in "Need review", so it's totally ok if things are WIP. I added some minor comments in the MR.

Note that we also have a good amount of testing code and tests, so we might want to add a test for this as well.

Yes, the lack of a test is why I haven't put into review yet. Also, I think the NameResolver I added is completely unnecessary as there is already use alias resolving going on , I just need to dig it into the how.

Yes, I fully agree this is not critical, Hook is not yet committed , this issue is happening to show when it gets committed api won't break.

Once again, congrats on the high quality code, it was really easy to put this in.

Perfect. I'll keep an eye on changes and make sure to review once it's ready.

So I think 🐛 Attributes are not rendered when showing the source code Active is only partially relevant here because this issue needs to work with code which has a mix of OOP and procedural hooks. That issue in general has different requirements. I will comment there.

IMPLEMENTS was removed for further discussion here.

The generic attributes issue itself was postponed on 🐛 Namespaced class methods are not linked Active which is now fixed so that can proceed. I expect it to resolve relatively quickly since the only thing missing is linking on display and a test or two then we can revisit this one.