Rethink #[Hook] attribute inheritance

Also, the $method parameter is pretty useless, nobody will use these attributes on a class.
For the existing #[Hook] attribute we have to keep it, but for new attributes we should only have them on a method.

And for some reason we dropped the $order parameter for #[FormAlter].

I RTBC’ed the form alter issue, but I agree this is cleaner and better DX. still waking up, so not a thorough enough review to RTBC (yet), but in principle, +1 from me. More later.

Thanks!
-Derek

This will conflict with 📌 Add order parameter to FormAlter attribute Active which is more urgent. 😉 Let's re-roll this once that lands to focus on the inheritance per the summary and title.

✨ support for OO hooks in classes Active is where some of the discussion happened too

We just need to keep the api module in mind, see the linked issue ✨ support for OO hooks in classes Active we need some way to tie the sub attribute to the correct hook documentation.

The original reason was, I thought we need to be able to link #[Hook('form_filter_admin_overview_alter')] to hook_form_alter documentation which will live on the class doxygen of FormAlter.

But maybe that's unnecessary? Just link FormAlter directly and while #[Hook('form_filter_admin_overview_alter')] will continue to work in Drupal , it won't work on api.drupal.org and that might be just fine if hooks get converted to their respective attribute when the doxygen is moved. Yeah. It might just work.

It's my fault for not realizing this sooner. Yes, that can work certainly that makes api.drupal.org implementation indeed a lot easier if we don't parse hook names and only link on attributes. But that is a decision far above my current paygrade.

Probably needs sign off for the api change.

Who is familiar enough with how api.drupal.org works to confirm this will work? I'll ping @quietone, she might know or at least know who to ask

So the battle plan is to create separate hook attributes and move hook documentation from the api.php files to their respective class doxygen.

I am a bit lost how the change proposed here changes the situation with api docs.
We already have an overridden constructor FormAlter::__construct(), so what changes?

Re: #13: per https://www.drupal.org/project/api →

This project is currently under the active maintainership of drumm and fjgarlin.

Thanks @dww, have pinged them

I can tell you with authority that linking an attribute to its defining class is working with api module because I wrote the attribute integration for api module :) 🐛 Attributes are not rendered when showing the source code Active

Now, that's just attributes in general not hooks. But this change would mean that api does not need to do anything else. Right?

Hook specific behavior, once again would be linking a method marked with #[Hook('form_filter_admin_overview_alter')] to the class page of FormAlter where you can find the api documentation, much like on the current page. My battle plan was to take all PREFIX and SUFFIX pairs from classes extending Hook, form a massive preg and match. You can not do this with $hook = ($form_id === NULL) ? 'form_alter' : 'form_' . $form_id . '_alter'; that's not something a simple parser can understand.

But, if the moving of api documentation is conditional with changing #[Hook('form_filter_admin_overview_alter')] to #[FormAlter('filter_admin_over')] then it just works by the nature of FormAlter being a class which links to defining page. As you can see on the test, AttributeTest is already a link: Nothing hook specific needs to happen. This does make the life of the api maintainer easier for sure.

It's a decision core leadership needs to make: is it acceptable that while #[Hook('form_filter_admin_overview_alter')] works in Drupal it will not link to the documentation page on api.drupal.org?

Ah no, not linking won't work because it's not just api.drupal.org which would make this mostly a core concern -- but it's every other tool out there too. Then it doesn't need framework review because things no longer break.

Tagging so we try to fix this before we ship any releases with the old way

As I was extending on the documentation of the attribute I realized prefix/suffix is not needed at all.

The rector rule already has the battle tested code to convert something like form_FORM_ID_alter to a preg so we can just add the name of the hook documented here and be done.

$parts = explode('_', $hook);
$isUppercase = false;
foreach ($parts as &$part) {
  if (!$part) {
    continue;
  }
  if ($part === strtoupper($part)) {
    if (!$isUppercase) {
      $isUppercase = true;
      $part = '[a-z0-9_]+';
    }
  }
  else {
    $isUpperCase = false;
  }
$hookRegex = implode('_', $parts);

I really like the direction this is going!

Two comments on the MR that need to be addressed then I think we can mark this as ready.

There are a few other comments, but they have been responded it in a way I think we can consider closed.

This cleans up the extensions a lot and lets us still get the documentation for the api module.

This is so much cleaner! It allows the documentation to be declarative, removes the workaround in extending Hook attributes and will allow us to drop Implements hook_form_FORM_ID_alter while maintain the api links.

I see this is RTBC but I would still want to further review the more recent changes.

The feedback has been addressed where it was relevant. Parts of it, I believe was made before the whole doxygen was read so they are not relevant.

I think you mistake the purpose of this entire attribute. All we want to do is to create the modern equivalent of this link: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21... whenever a #[Hook('preprocess_foo')] occurs.

There is no such function as hook_form_alter, the very purpose of this MR and the linked todo is to remove it.

Then I think I might disagree with this change / intention.

Currently the hook_something() function in *.api.php is the place where we document a hook.
The nice thing here is that this includes parameters and return type, which can be naturally documented with native type _and_ @param and @return docs.
A function feels like a relic of the past, but in fact it is a clean way to document a signature outside of an interface.

If I understand correctly, the documentation should be moved over to attribute classes.
Or rather, this should happen for _some_ hooks, whereas other hooks remain documented as hook_*() functions.

This leads to the following questions:

• How far would we eventually go with this? Will we have one attribute per hook, or will we introduce attributes for all _dynamic_ hooks?
• What is the DX benefit once this transition is complete?
• How disruptive is this change going to be?

Benefits:

One DX benefit might be that you can navigate in one step from an implementation to the attribute that has the documentation.
If we keep the function, you would instead have to navigate two steps, first to the attribute and then to the hook_*() function.

A more obscure possible future benefit is that we can introduce hook names with special characters like '.', e.g. you would have entity.node.update instead of node_update.
This would not be possible while using the hook_*() functions for documentation, or the placeholders like ENTITY_TYPE would have to stand for strings that include these characters.

Problems:

But currently I don't see how we want to document the parameters and return value in the attribute, in a way that is preferable to or en par with the hook_*() function.

If we only convert some hooks to have their own attributes (or only dynamic hooks), then there will be a perceived inconsistency.
If we convert all hooks, we will get far too many attributes.

I checked the original issue and one significant change from the original intent is the doxygen was meant to be on the class constructor and not the class itself. This might or might not help. I committed the change to move there (but it's easy to revert).

It's entirely possible I intended to move over to the constructor because I thought we could transplant the arguments to the constructor and we clearly can't do that, the constructor is defining the params of the attribute not the hook. I am all too often wrong and too stubborn to realize. Thanks for keeping me straight.

But since DocumentsHook now targets a method, it could be used anywhere where documentation is convenient.

My suggestion would be to put it on EntityStorageBase::invokeHook() as an example. And where such a method doesn't exist, write one. The amount of boilerplate seems to be the same: one function currently vs one method factored out solely for documentation purposes. One issue certainly is @return -- if we were to factor out \Drupal::moduleHandler()->invokeAll('field_info_max_weight', $args) into a method that would return a list of integers while hook_field_info_max_weight returns an int. That's a problem. Maybe @return-hook? if phpstan can define custom tags can't Drupal do the same?

Or maybe only use DocumentsHook on methods when it's convenient and keep hook_foo_bar in *.api.php alive. For consistency, marking those functions with DocumentsHook would be nice.

This, however, would completely separately DocumentsHook from this issue and so if the decision is to go in this direction then this issue could go back to just ripping out prefix/suffix and then DocumentsHook can be resolved in a separate issue.

Another very long Slack discussion. First the agreement reach is the documentation remains in api.php for the time being.

This, however, circles back to the original issue which PREFIX/SUFFIX attempted to address: how does api.module recognize the documentation for #[FormAlter('filter_admin_overview')] can be found at hook_form_FORM_ID_alter?

There was immediate agreement there should be at least an attribute on the FormAlter attribute class itself which helps with this.

While it would be possible to add base_form_id to the FormAlter constructor and use the new attribute like

#[HookPattern('form_alter')]
#[HookPattern('form_FORM_ID_alter'), ['form_id]]
#[HookPattern('form_BASE_FORM_ID_alter'), ['base_form_id]]

Then a method marked with #[FormAlter(base_form_id: 'node_form')] could be linked to form_BASE_FORM_ID_alter.

The complexity doesn't seem to be worth it, the documentation for form_FORM_ID_alter and form_BASE_FORM_ID_alter is nearly identical, merging them seems to be the better road forward.

Briefly we considered an #[Implements] optional attribute at the implementation point as a modernization of the current Implements hook_foo() docblook but if it's always required then it's too boilerplate-y and if only occasionally used then the confusion on when it should be used and when not would be a challenge.

Thus, we converged on a simple solution: just use HookPattern , nothing fancy and merge the documentations where needed. It will be rare, note how hook_entity_query_alter is careful with its underscores so the matching is not ambiguous.

I think this is ready again, I'll give @donquixote a chance to respond, but this cleans up the extensions and gives us a way to tie to documentation.

We can also add to it if we want to extend further.

Ok, I think this is ready now!
Thanks for everybody involved, especially @ghost of drupal past who clarified what we need for the API module documentation parsing, and who pushed most of the recent changes.

I left some review comments open. These are not blockers, just an opportunity for others to comment.

I have only skimmed this issue and the MR and can't find any discussion on the name of the new attribute, "HookDocumentedAt". Is there discussion? While waiting for an answer, I will add that "HookDocumentedAt" is cumbersome. How about just "HookDocument"? It is shorter and, for me, conveys the same meaning. To avoid confusion, I am not asking for a change, it is just a question.

Most of the discussion was in a gargantuan Slack thread:
https://drupal.slack.com/archives/C079NQPQUEN/p1746863182915979

I'll agree that "HookDocumentedAt" is a little cumbersome. However, I think "HookDocument" is too ambiguous.

Looking at it with fresh eyes, what about "HookName"? Isn't that what this is really about"? The name of the hook that children of Hook are targeting. The fact we use this name to help find the right documentation is relevant, but it's not necessarily the only use for this.

p.s. Saving credit for the 4 of us who've been in all the Slack threads, MR threads, etc.

HookDocumentation?

https://www.youtube.com/watch?v=5sSp-t2fXXg

I'm a bit behind but I quite like HookName or HookDocumentation.

Moving back to needs review.

The general approach to solve linking if dynamic hooks seems good.

That's two core committer votes for HookDocumentation so it's done. I reset back

No, that's premature, let's wait for quietone.

Try to look at this with fresh eyes:

#[HookDocumentation('hook_form_alter')]

You call that documentation? 😂

I call it a name.

I think HookName would be way more clear. The fact it's linking to documentation is sort of a side effect. Yes, it's why we're adding it. But it's not documentation. It's a pointer that we use to find the documentation.

#46 is a good point.

yep, name looks better, 🚲️🎨🏚️

It was just a question! ;-)

I stared at these options in the IDE I use for a while

#[HookDocumentedAt('hook_form_alter')]
#[HookDocumentation('hook_form_alter')]
#[HookDoc('hook_form_alter')]
#[HookDocument('hook_form_alter')]
#[HookName('hook_form_alter')]
#[LegacyHookName('hook_form_alter')]

And my thoughts on them.
HookDocumentedAt - Meaningful, cumbersome to read. Clear this is just for documentation purposes.
HookDocumentation - Meaningful, easy to read. Clear this is just for documentation purposes.
HookDoc - Similar to HookDocumentation but the abbreviation made it a little less clear.
HookDocument - Same as HookDocumentation but somehow not as easy to grasp.
HookName - Immediate thought was that this is a code thing, or some requirement I need to learn. Just like @ghost of drupal past suggested in #47.
LegacyHookName - Same as "HookName"

So I would go for the longer "HookDocumentation".

So do we have an agreement on HookDocumentation then?

Yeah that works for me. I can see the reasoning in #46, but it's like a reference to documentation, not the actual documentation, so I think people will get the hang of it. And #49 lays out the pros and cons very clearly.

Then let's do this. The name was originally suggested by larowlan and now we have buy in from catch and quietone. It was ready before, the rename was simple and automated.

I’m still not convinced, but not worth it to keep arguing. I’m happy to stand aside and let the majority view prevail here. 😉 HookDocumentation it is.

I think the only other concerns in here are very minor docs nits that could be fixed any time, so let’s get this in before beta gets tagged and we can’t fix any of it.

RTBC++

just 5c as help module maintainer - why not #[HelpTopic]?

@andypost: re #54: Because this isn't necessarily tied to the "help topic" world, there aren't necessarily help topics about each of these custom hook attributes, etc. We've already been around the bikeshed many times on this name. 😉 I don't love HookDocumentation, but that's definitely more accurate and future-proof than HelpTopic would be.

What about #[HookInvolved]?

Re: #56: #[HookInvolved] seems even more opaque, vague, and unclear.

@all: Please, we've got 3 core committers (@larowlan, @catch and @quietone) and 3 prolific contributors (@chx, @donquixote and @nicxvan) in agreement that HookDocumentation is our best choice. Let's leave the bikeshed a nice pale green and move on, okay? 😂

Took a look into the current MR and leaving some notes here.

#[HookDocumentation] is a confusing attribute. Its primary purpose is outside of what is relevant for the runtime of basically all running sites, yet it will demand computation resources from all of them, because attributes are part of reflection-based class discovery. Those *.api.php files were not really demanding such amount of computation resources on every site, as opposed to this concept.

Looking at the currently suggested implementation of #[HookDocumentation], the constructor argument callable $documentation is confusing. It would indicate one may pass along whatever is callable (first assumption was maybe a function returning documentation?). But what is supposed to be finally passed along, is not even callable. There is no real function like "hook_form_FORM_ID_alter".

Its primary purpose is outside of what is relevant for the runtime of basically all running sites, yet it will demand computation resources from all of them, because attributes are part of reflection-based class discovery.

Nothing is reflecting this directory, these only go on classes extending Hook.

Those *.api.php files were not really demanding such amount of computation resources on every site, as opposed to this concept.

This is also not accurate, only ides and the api module will care about these.

They will also be rare.

Looking at the currently suggested implementation of #[HookDocumentation], the constructor argument callable $documentation is confusing.

While I agree, the threads on slack are approaching 600 comments.

This is only needed for compound hooks like form alter and entity hooks.

The actual important bit is the hook change to remove the constants. This is internal, can we get this in and open a follow up for all of the naming discussions?

Now that I think about it, maybe the extensions of Hook are not needed?

It's clear the work needed to tie the documentation in is controversial.

Is the convenience of typing:

FormAlter()
Vs
Hook('form_alter')

Really worth it?

Maybe we just pull them now that we know Hook can work with the api module?

The custom attributes were added because I confused the attribute constructor parameters with the hook parameters. It's not a viable idea. That's on me.

Now that we know the documentation stays in api.php their disadvantage shows: they always will be controversial, which hooks get separate attributes and which don't and how many? Trying to decide how entity hooks for example should be sliced will be a lot of fun. And another controversy is this very issue, we spent so much back and forth and there's still new voices and discussion to be had.

For api.module, not having custom attributes is easier since it always needs to support #[Hook('form_foo_bar_alter')] so if it doesn't to support FormAlter on top, that's easier.

I have no problems with fixing this issue by deleting FormAlter and Preprocess attribute classes.

I'll take a crack at this

I just pushed up https://git.drupalcode.org/project/drupal/-/merge_requests/12128/diffs with the removal. I really think this is the right way to go.

This will likely need a CR I'll work on shortly.
We will need to update these too:
https://www.drupal.org/node/3499495 →
https://www.drupal.org/node/3499788 →
https://www.drupal.org/node/3496491 →

I'd like to thank everyone that has tracked and worked on this issue here and slack, an enormous amount of work has gone into this in the last few days, approaching 700 comments in slack.

After responding in 59 I decided to take a step back and consider from a higher level why an issue like this required SO much discussion cause at it's root it's pretty simple, clean up how we extend hooks and ensure we can document.

I then realized we had solved the issue of documentation which is why we could remove PREFIX and SUFFIX and realized that the extensions were making documentation HARDER, not EASIER.
I then thought through it and realized with the backporting issues that having new attributes for hooks will be a nightmare for contrib to track and we don't want to go through this for each round of hooks.

It took a lot of effort from several people, but it made me realize we had the wrong target.

I'll go through the slack threads after creating a CR and make sure everyone there is mentioned here for credit.

I also added a reference in the linked CRs to watch this issue.

Marking this critical because we are in alpha, we have to finish this before 11.2 is beta and comes out.

Slack discussion participants:
Thread 1
https://drupal.slack.com/archives/C079NQPQUEN/p1746863182915979
481 replies
Participants
nicxvan
ghost of drupal past
donquixote
dww

Thread 2
https://drupal.slack.com/archives/C079NQPQUEN/p1747142557421249
138 replied
Participants
nicxvan
dww
ghost of drupal past
donquixote

nicxvan → changed the visibility of the branch 3523109-rethink-hook-attribute-inheritance to hidden.

Bit of a facepalm about how much effort went into this, only to collectively realize we were on the wrong track all along. 😬

But kudos to @nicxvan for looking critically enough to see a more simple way forward.

https://git.drupalcode.org/project/drupal/-/merge_requests/12128 is certainly more clean. The diffstat is way better. 😉 It's mostly removing stuff we realized we no longer want (the FormAlter and Process attributes), and converting a handful of usages back to just #[Hook].

Everything should be as simple as possible, but no simpler.

-- Einstein.

This is definitely simpler for everyone: for developers, for core maintainers, for contrib maintainers, for api.module, for documentation.

Given the MR is removing a bunch of things we don't want, and only adding back raw #[Hook] attributes, I spot nothing to complain about with the diff.

Given the pipeline is green, I know the linting and tests are all happy.

Therefore, RTBC.

Agreed this is urgent before beta1 is tagged, so let's get this in ASAP. Critical++

Thanks!
-Derek

The new CR here at https://www.drupal.org/node/3524585 → mostly looked great, but I did some minor edits and enhancements.

However, tagging for "Needs change record updates" to capture this task from #65:

We will need to update these too:
https://www.drupal.org/node/3499495 →
https://www.drupal.org/node/3499788 →
https://www.drupal.org/node/3496491 →

For now, I edited them to indicate they were added in 11.2.0-alpha1 (which I know we don't normally do, but this is a special case).

This is a good option, and better to do it it now than realise it in three years, then have to spend another three years unpicking it again.

Committed/pushed to 11.x and 11.2.x, thanks!

I'm leaving this needs work for the change record updates, just so we can explicitly track we did them when marking this fixed.

catch → closed merge request !12128

One interesting thing here.

I had actually implemented #[Preprocess] in the alpha of a new contrib module already.

When testing against 11.1, #[Preprocess] was ignored - the module supports 10.x so it also has a procedural preprocess that calls the service method with LegacyHook.

I switched from #[Preprocess] to #[Hook] and got

In HookCollectorPass.php line 386:
                                                                                                                           
  The hook preprocess_field on class Drupal\lms_h5p\Hook\H5PHooks does not support attributes and must remain procedural. 

Should we open another follow-up to remove that exception from 11.1? As long as there's a #[LegacyHook] it should work on all versions.

Sorry for the noise, just want to briefly tell you this reconsideration and turnaround is highly appreciated. Thanks!

Should we open another follow-up to remove that exception from 11.1? As long as there's a #[LegacyHook] it should work on all versions

Yes please, I need to think about that.

I updated all of the CRs and published this one.

I also created https://www.drupal.org/project/drupal/issues/3524716 📌 [11.1] update gather rules to manage hook preprocess Active

Should we open another follow-up to remove that exception from 11.1? As long as there's a #[LegacyHook] it should work on all versions.

For half of the projects this will be the correct solution.

For the other half, they remove (or never add) the procedural preprocess, add the methods with #[Hook('preprocess_*')], test with 11.2.x, but declare compatibility with 11.1.x. Then it will silently not work in 11.1.x.

The separate attribute for #[Preprocess] would have avoided that. For the documentation parser we could even hard-code #[Preprocess] support, instead of providing that new attribute.

The remaining problem then would be the expected bikeshedding of parameters passed to #[Preprocess] as in 📌 Restrict new hook attributes to use on methods, drop $method parameter Active where we either just drop $method or also $module. And more disruption if we would add other parameters in the future.

Preprocess is gone we just need to remove the excepting for 11.1 and warn that the procedural is required.

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.

Thanks for all the updates @nicxvan! I did a pass on all 4 related CRs and did a round of additional edits. They're all published and good to go. Removing the tag and calling this Fixed.

Yay,
-Derek

Published change records should not be marked "Obsolete". At the time is was published and for that release it was accurate. That will still be true. When it changes, in a future release, a new change record with the new information should be created.

This needs some work and review of when the change records were published. I don't have a suggestion right now as I am being pulled in to many directions. This should be sorted before the beta release. I need to think on this.

I think the unique thing about these ones are they were released in alpha or very shortly before alpha and removed before beta, they were never in a release of Drupal.

I think we should probably delete the change records - because they never made it into a release, there is no change for module developers as such. The issue history is still here to reference.

I think we should probably delete the change records - because they never made it into a release, there is no change for module developers as such. The issue history is still here to reference.

I have no strong opinion, it makes sense to delete them if they were not part of a release, if that is our general policy.
But, I can also find sympathy for the idea of marking a cr as obsolete if it was not part of a release. This would for people who read these CRs in the past, and are now confused if they don't find them anymore.

However, if we do delete them, we should change https://www.drupal.org/node/3524585 → which talks about the PREFIX and SUFFIX constants being removed, even though these constants were not part of a release. Either don't mention this at all, or say that these briefly existed in 11.2.0-alpha1.

Having just found out that these new attributes have been removed, I'm very glad that the change records were not deleted and have been marked as [Obsolete]. If they'd been deleted I'd still be searching for signs of them.

I only had a passing recollection of what I had seen in a change record so I searched for it and found these two. From there I now know that I can just use [Hook] because the [FormAlter] didn't survive, and I can read the issues around to check on any future plans for them. It helps to plan out the best way update modules to be compatible with future cores.

It also serves as a more public "Tried this, didn't work" than a closed issue.

So i think these are the options:

1. Delete the obsolete crs
2. Delete the obsolete crs but add a note on one of the remaining crs about the deletion
3. Leave them marked as obsolete

I'm honestly not sure what is best.

Actually I like keeping them as "obsolete" following the motivation from @elc in #93. I find that very convincing.

We actually have a long-standing practice (going all the way back to 2015) of deleting obsolete CRs when APIs are superseded within the same release and merging the content into a single CR. I almost went to delete the CRs before reading this issue, actually. This, I would prefer #94.2.

Given #93, maybe we could keep them around for six months or something as a compromise. Not sure.

Can we add redirects?

Normally, I would say "yes we can definitely add redirects", but CRs unfortunately don't have a URL alias by default, only the base system path of /node/whatever.

I searched for "drupal change record formalter attribute" to find the obsolete CR, purely based on the content of the page as it has no fancy url alias.

If the page is deleted and the crux of the CR noted on another one, then that's where that such search will go in the future. There would be no need to dive into doing a redirect when there is no drive for users to hit the old link.

Given the options, and the general practice of removing obsolete CR, then adding details to one of the CR that will remain is the better option.

In that case it's this enough? https://www.drupal.org/node/3524585 →

Word of infra is:

You’ll want to add the redirect after the delete. We commonly do that for deleted redundant documentation. Redirecting before the deletion can lead to confusion, and redirect module cleans up redirects for the node on deletion, so it could get cleaned up

So that's doable.

@nicxvan That CR would have turned up in my search ok and it contains enough information for a user to understand what happened. It's currently showing up in the search index too, just a few rows down.

It has a link to the old CR which should perhaps link to the original issue instead.

And, oh boy, did this need a release note. The 11.2.x release notes are currently in a confusing state because we were working from the past issues (which were tagged from release notes when thes were not). We could use contributors help on the review:
https://docs.google.com/document/d/1GEP1kEqNsjngaLLfcFe1AMfIpZTaHChve_xl...

Adding the release note snippet I'm using.

These two marked obsolete can be deleted:
https://www.drupal.org/node/3499788 →
https://www.drupal.org/node/3499495 →

I've reviewed the other two and all relevant information is there.