Remove support for extending #[Hook] and remove classes extending it #[FormAlter] and #[Preprocess]

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 #3523159: Add order parameter to FormAlter attribute which is more urgent. 😉 Let's re-roll this once that lands to focus on the inheritance per the summary and title.

#3471763: API-docs support for OO hooks implementations in classes is where some of the discussion happened too

We just need to keep the api module in mind, see the linked issue #3471763: API-docs support for OO hooks implementations in classes we need some way to tie the sub attribute to the correct hook documentation.

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

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

But maybe that's unnecessary? The FormAlter attribute links to the defining class. So a decision can be made where#[Hook('form_filter_admin_overview_alter')] will continue to work in Drupal, it won't link on api.drupal.org to the form alter doxygen 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 api.module doesn'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

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 :) #3449634: Attributes are not rendered when showing the source code

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 was a goal of the original hook oop issue to make it possible to write a hook without defining a new object for every hook. This is not changing. But it was not a goal to make documentation work for such ad hoc hooks. The plan for documentation always was to have one attribute per documented hook. So the required decision here which 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 hook 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.

Note: the issue summary is supersedes the discussion to this point.

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. New proposal, new class, much more straightforward: just put the name of the hook like form_FORM_ID_alter on the class.

Thishttps://3v4l.org/jR9IT converts the hook documented into a preg.

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.

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 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 DocumentsHook 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. This would solve @param but @return is a problem: 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 reached 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. #3479141: Implement FormAlter attribute also didn't deem it necessary to tell them apart.

Briefly we considered an #[Implements] optional attribute at the implementation point as a modernization of the current Implements hook_foo() docblook to tell FORM_ID and BASE_FORM_ID apart. However, this is the only case when it's actually needed and then if it's always required then it's wasteful 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: entity hooks only have one wildcard version with hook_ENTITY_TYPE_insert etc and while the hook_entity_query_alter family is large, it 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.

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.

Edit: although does calling it HookName imply it's used for something more than documentation? Don't think we would want to give the impression it's used to disambiguate similarly named hooks.

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.

Edit: donquixote on Slack: "for me the old name was ok but the new name also ok :slightly_smiling_face:" nicxvan: "Same, for me having some form of the word document seems important".

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.

Update: rechecked the reflection-based class discovery and looks like attributes are not involved unless explicitly being called by the attribute name. Thus striked through within section above.

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?

To be clear, I just mean pulling the FormAlter and Preprocess classes only. Nothing underlying the preprocess support.

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.

And support in contrib is going to be an absolute mess -- you need to keep track of which attribute was released in which version and deciding what to support.

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.
Our target should be consolidating on #[Hook]

Further, we don't gain that much by adding the additional classes.
#[FormAlter]
vs
#[Hook('form_alter')]

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

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.

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

Preprocess is gone we just need to remove the exception 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.

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.

We actually have a long-standing practice (going all the way back to 2015 before 8.0.0) of deleting obsolete CRs when APIs are superseded within the same minor, and merging the content into a single CR that either briefly mentions or does not mention abandoned/replaced approaches. I almost went to delete the CRs before reading this issue, actually. Thus, I would prefer #94.2.

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

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 think it might still be possible but we'd have to check and I have memories of badness for doing things like that in the past.

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 is 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.

I think we can delete the two obsolete and close this.

I think this is ready.

We just need the two obsolete CRs deleted and the redirects set up.

I've unpublished the two CRs (so it's reversible), which leaves the redirects. I don't think I have permissions on d.o to set those up.

Thanks! I added #3547357: Add redirect for two obsolete change records.

I think the CRs might need deletion still so the redirects can be set up.

Even unpublished they are accessible.

I've updated the credit to add elc.

I think since this had a follow up in drupalorg we might be able to close this now.

Yes let's close this one.