🇩🇪Germany @donquixote

The ModuleHandler::add() is more broken than we thought.

It picks up procedural implementations only.

This is the least part of the problem.
Much worse is that it removes existing implementations, and prevents them from being added later.
See 🐛 ModuleHandler::add() can prevent existing implementations from being added Active .

donquixote → created an issue.

I added the unit test for ImplementationList in this issue, to prepare for 🐛 HookCollectorPass registers event listeners but they do not follow the required calling convention Active .

donquixote → changed the visibility of the branch 3519561-DeprecationCanaryTest to hidden.

donquixote → changed the visibility of the branch 3519561-hook-ImplementationList-remove-getHookListeners to hidden.

donquixote → changed the visibility of the branch 3519561-DeprecationCanaryTest to hidden.

I created an alternative MR where ProceduralCall is still used in hook implementation identifiers and in the parameters passed to ImplementationList::load().
Tbh, this feels out of place, I don't like it.

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

Is there a doc page about the "langcode:" setting in a config record? I find https://www.drupal.org/docs/develop/creating-modules/defining-and-using-... → but not sure if that is up to date.

I assume the purpose of the "langcode:" setting is to set the "origin" language of any string as they appear in the config record. Typically this would be 'en'. As a consequence, we would then want to translate from 'en' to 'de', and the German translation would be stored in a separate place (config/sync/language).

If a config record has a different language, e.g. 'langcode: de', this would mean that we now need to translate from 'de' to 'en', and the English translation would be stored in config/sync/language.
But is this actually how this works?

I somehow doubt that this is going anywhere as a general policy.
Lets see if there are any responses from others, otherwise ok to see it closed after 3 months.

I somehow doubt that this is going anywhere.
Lets see if there are any responses from others, otherwise yeah it can go its path to eternal sleep.

I created a branch and MR with a reduced set of conversions, using PhpStorm:
- Create inspection profile with only "Property can be promoted", all others disabled.
- Run inspection, apply fixes.

This only converts parameters where the property has the same name and type as the parameter.
So, no automatic rename.

@donquixote you're a wizard! Can you share the script?

I should have done this as a script, but instead just used phpstorm find/replace.
I am trying to find it in the history, be careful, I could be picking the wrong line.

First step:
Find: "^( (?:public|protected|private) function __construct\()((?:(?!public)(?!protected)(?!private)[^,\)\n]*\$\w+(?: = [^,\)\n]*)?,(?: |\n ))*(?:public|protected|private) [^,\)\n]*\$\w+(?: = [^,\)\n]*)?)"
Replace: "$1// BREAK HERE SQUIRREL\n $2"

Second step (repeated):
Find: "// BREAK HERE SQUIRREL(\n [^,\)]*,) "
Replace: "$1// BREAK HERE SQUIRREL\n "

Last step for empty body:
Find: "// BREAK HERE SQUIRREL(\n [^,\)]*)\) \{\n \}"
Replace: "$1,\n \) \{\}"

Last step for non-empty body:
Find: "// BREAK HERE SQUIRREL(\n [^,\)]*)\) \{"
Replace: "$1,\n \) \{"

Later you can use "git diff --ignore-all-space --word-diff -U0" to quickly review the changes.

I’d also support making this required for new code, at least for the first two “strong recommendations”.

I would be ok with that.
I was hoping that something weaker would be more likely to land, but I have no objection to hard rules :)

By supplying a maximum line length (e.g., 120 characters) we could even make the third one a hard rule, too. (I couldn’t really be enforced with just “very long” as the criterion.)

Ok for me, but this could broaden the scope quite a lot.

At least, the comments there need to move elsewhere.

Not sure what you mean by that.
To me it seems clear that the proposed text is inside the quote, and what is below is comments.

I am adding a separator heading "Notes", I hope this helps.

Good old (repeated) regex trick...

Detect and mark any constructor with at least one promoted parameter:

  public function __construct(// BREAK HERE
    $a, $b, protected $c, $d) {

Repeatedly apply a regex replace to move the marker step by step.

  public function __construct(
    $a,// BREAK HERE
    $b, protected $c, $d) {

  public function __construct(
    $a,
    $b,// BREAK HERE
    protected $c, $d) {

  public function __construct(
    $a,
    $b,
    protected $c,// BREAK HERE
    $d) {

Handle the last parameter and the {}.

In fact, if we want to reduce the size of this MR, we should get this in first:
📌 [pp-1] Explore adding an ImplementationList object for hook implmentations Active

This change feels far more important.

And in general, we should prioritize changes that feel less important, if that makes other changes easier.

I added support for using ProceduralCall in order attributes.

We still would need tests for this.
But, tbh, ProceduralCall is marked as @internal, and I don't really want anybody to use it.

Can't we move the proceduralcall removal to a followup? It looks like it's not necessary here and is scope creep.

I was proposing the same earlier.
But it really hurts to see these "Drupal\Core\Extension\ProceduralCall::$function" identifiers, or to see $function passed as a method.
The class was necessary as long as we had to register hook implementations as event subscribers.

The complexity of an MR comes from the diff size, but also from the end result.
In this case, we get a cleaner end result by having a slightly larger diff size (but still less than some earlier MRs).
I don't want to engineer the new solution around this leftover.

I created a preview MR.
No tests, and more attributes than we should keep in the end.
The goal is to look at the different options we have, and decide what we like best.

We cannot remove it without providing an alternative way to target procedural hooks for ordering.

In the other issue the argument was made that it is usually fine to just target all implementations of the respective module.

As a reminder, there were two cases where we really need to target just the procedural implementation:

• The target module has procedural _and_ OOP implementations for the same hook.
  This could be because the OOP implementations were added later, and provide different functionality, while the old procedural hook implementation has not been converted yet (because out of scope/budget).
• We want to support different versions of the target module.
  One version has a single procedural implementation for a given hook. The other version has multiple OOP implementations. We want to target the procedural implementation of the older version, and possible some (but not all) of the OOP implementations from the new version.

How do we interact with the procedural calls if that class is gone?

I assume this is about "ProceduralCall" class.
That class is already marked as internal, so using it to target a hook would already be wrong.

A solution for this can be discussed in 📌 [pp-1] Create new attribute to make reordering procedural hooks easier. Postponed .

donquixote → changed the visibility of the branch 3516146-rethink-procedural-hook-ordering to hidden.

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.

The question is which cache tags should be invalidated here.
I suspect we would have to invent a new cache tag that does not exist yet.

(here is what I remember, please correct me)

There is one cache tag for the published / active version of an entity.
This gets invalidated when that published / active version is updated.
There are good reasons why we would not want to invalidate this when other revisions are deleted or updated, because we want the published display to be served from cache as often as possible.

Instead, we could introduce a cache tag for "revision list for entity ", which gets invalidated when any revision of that entity is deleted, created or updated, or there is a workflow change.

We would have to properly test this.
And we need to determine if we really want to fill the cache with rendered entity revisions.
It could be really nasty on storage, for possibly little benefit.

It was argued in slack that we don't need to target _only_ a procedural implementation with e.g. #[RemoveHook].
Instead, one could simply target all implementations of the module, because most modules will either be fully on OOP or fully on procedural hooks.

One reason I can think of to support removing only a procedural implementation is if a module wants to work with two major versions of another module.
In that case, it may want to remove the procedural implementation from version 1 of the other module, but also remove a specific OOP implementation (but not all) from version 2 of that other module.

---

It was also argued that we should not add more attribute classes like RemoveProceduralHook, but rather add parameters to existing ones.

The counter-argument here would be that changing the signature of an existing class can be a painful thing to do with regard to BC, especially if we support named arguments passing, and if we want to add or remove more parameters in the future.

Introducing new classes tailored to specific cases is a lot easier.
Also, this means that we can keep parameters required, and we don't need to explain which combination of parameters is allowed or not.

This default setting was changed in #3347015: Consider using the administration theme when editing or creating content by default, does that just need to be committed to 11.1.x?

I would say this is a significant functional change that we would not want to have in 11.1.x.
If all we care is to make a test work, we can change that setting in the test itself.
Or rather, we change that setting in the test to use the front-end theme instead of the back-end theme, if we are interested in the cache behavior.

Then, the more important question is:
Can and should we cache a revision list?
If so, which cache tags make sure that this cache is invalidated when revisions other than the published revision are added, modified or deleted? I don't think such a cache tag exists currently.
Also, what is the use case where caching the revision list actually makes sense? That is, where the revision list is visited significantly more often than it is changed? (I suppose working with revisions in workflow)

Ok for me.
In the original issue we need to determine if a revision list should be cacheable at all, with which cache tags, and then add more tests.

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.

donquixote → changed the visibility of the branch 3523385-11.1.x-upstream to hidden.

One question is whether we should do anything for 11.x.
Currently the test is passing in 11.x without the change.

donquixote → changed the visibility of the branch 3525090-11.1-resolvedlibrarydefinitionsfilesmatchtest-is to hidden.

So the fix should be to just get the new module handler from the container right?

(Sorry, cross post earlier)

We want the library discovery to have the new module handler.
The only way to achieve that is to get the new library discovery after the modules are installed.
(MR asap)

I tried to cherry-pick the commit from 📌 Add the ability to install multiple modules and only do a single container rebuild to ModuleInstaller Active into 11.1.x.
Now it is still failing, but differently from before.
Now ->setModuleList() is called multiple times, once per "module group".
The old module handler will get 13 of the newly installed modules from the first batch, but not any of the remaining 60 modules.
Now instead of 'big_pipe' it fails on 'block'.

Solution

I think a solution is to not use a property for $this->libraryDiscovery in the test.
Instead, grab the library discovery from the new container after modules are installed.

Perhaps we would also want to do this for 11.x, to not rely on setModuleList() being called.

With some xdebug I find a difference between 11.1.x and 11.x in this test.

In the test, we do the following steps:
- Initialize $this->libraryDiscovery from the container.
- Install additional modules.
- Get a module list from the (new) container.
- For each installed module (and each theme) we get libraries.

This means the module list is coming from the new module handler from the new container.
But the library discovery still has the module handler from the old container.

In 11.x, I suppose since 📌 Add the ability to install multiple modules and only do a single container rebuild to ModuleInstaller Active , we are calling ->setModuleList() on the old container, so that now even the old container has the updated module list.
In 11.1.x, that call to ->setModuleList() is not happening.

Then we have this code in LibraryDiscoveryParser:

      if ($this->moduleHandler->moduleExists($extension)) {
        $extension_type = 'module';
      }
      else {
        $extension_type = 'theme';
      }
      $path = $this->extensionPathResolver->getPath($extension_type, $extension);

Some tricks with xdebug

To find out what is going on, I used xdebug and also inserted some debug statements into the code.
Specifically I added some local variables like this in LibraryDiscoveryParser and also in the test itself:

      $module_handler_id = spl_object_id($this->moduleHandler);

And then break points to look into the module list in each module handler instance.

This way we can see where we have the same or a different instance of the module handler.
And we can see which instance has the updated module list or the old module list.

It appears this caused problems in 11.1.x.
See 🐛 [11.1] ResolvedLibraryDefinitionsFilesMatchTest is failing Active

git bisect reveals commit d9535798624 as the culprit, introduced as backport in 📌 Add a deprecated module version of ResolvedLibraryDefinitionsFilesMatchTest Active .
This is not released yet, but will be released in 1.1.8.

The MR works for me in local.

However, I am a bit puzzled by this:

• In 11.x I don't see node being added as cacheable dependency, and yet no fail.
  Maybe this is not cached at all in 11.x?
• On the other hand, I don't know if the node or entity itself is even sufficient cache tags for a revision list.
  If you change (or delete) a revision that does not change the current/published revision, does this even invalidate cache tags for the entity/node?

To be analysed in 🐛 NodeRevisionsAllTest::testRevisions() fails in 11.1.x Active

donquixote → created an issue.

If a page was not cached before, it is not enough to just fix the reasons why it was not cached.
We also need to check that all the cache metadata added to the page is complete!
I suspect we have missing cache tags..

We have a failing test in 11.1.x:
Drupal\Tests\node\Functional\NodeRevisionsAllTest::testRevisions()

git bisect reveals the failure was introduced here.

For some reason, it was not failing in the MR when it was merged.

The same test is failing in 11.1.x for me.

We should get this merged before 11.2.0-beta1.
It fixes what could possibly be seen as regression.

Actually it is kind of too late, it is all in 11.1.7.

donquixote → changed the visibility of the branch 3523385-11.1.x-revert-bc-stubs to active.

This is critical, we have to avoid releasing a version of 11.1.x that has a bad version of these stubs.

Perhaps we should fully revert the BC stubs now, and then do a follow-up where we re-add what we need.
That is, if we cannot quickly agree on something else.

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.

Always passing the hook as last parameter is problemetic in multiple ways:
- Variadic hooks that accept any number of arguments.
- Hooks with many optional arguments.
- Hooks where we add/invent new parameters in the future.

An alternative is to mark one parameter (the first one) with a special attribute.
Or mark the method itself with an attribute.

class MyHooks {
  #[Hook(...)]
  #[Hook(...)]
  public function foo(#[HookName] string $hook, $other_arg) {}
}

class MyHooks {
  #[Hook(...)]
  #[Hook(...)]
  #[HookNameAsFirstArgument]
  public function foo(string $hook, $other_arg) {}
}

I think to remember a case where this would have been convenient, but in the end I did not really need.

In slack discussions related to 📌 Rethink #[Hook] attribute inheritance Active , we argued that attribute classes are not a good place to document the parameters passed to a hook implementation.

Instead, we decided that we want to link from the attribute class to the old function in *.api.php.

This also means that we can introduce new attributes without changing the hook names.
In this issue we should focus on the attributes.

As for attribute names, maybe we should keep a 'Hook' prefix?
This would be to distinguish from other attributes that have nothing to do with hooks.
But I have no super strong opinion, maybe prefix 'Entity' is fine, especially if the implementations all live in 'Hook' namespace.

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.

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.

Now if you want to ship a module that supports 11.1.x and 11.2.x, and which needs to order hook implementations, you basically have to keep the HMIA without the #[LegacyHookModuleImplementsAlter], and eat up the deprecation warnings.

This is just flat out wrong. LegacyHookModuleImplementsAlter does nothing in 11.1 so hmia will run as expected, and in 11.2 where it was deprecated if you have the attribute you won't get the deprecation.

The point is, if we don't have the BC stubs in 11.1.x, then you cannot add the order parameter in Hook for 11.2.x without breaking 11.1.x compatibility.
As a consequence, you have to keep your HMIA working in 11.2.x, which means you cannot add the #[LegacyHookModuleImplementsAlter] to it.

Introduce minimal BC stubs for 11.1.x.
Yes, this is the only thing we need to do to handle the named order parameter.

Correct.
If we do that, the problem goes away.

The above comment was directed at the proposal to do nothing, from catch in #8:

The last patch release of 11.1 is in a couple of weeks, so I'm starting to think we should not worry about this at all, and just continue on 11.2 and later. People writing brand new modules can require >= 11.2. People who want to convert existing modules to OOP hooks can require 10.5 | >= 11.2 (at least in 2-7 months). Having to add > 11.1.7 in .info.yml already limits the feasibility of using this and maintaining 11.1 compatibility.

This.
The main problem here would be

People writing brand new modules

because in fact it would not apply just to "brand new modules" but to any module with HMIA that wants to dodge the deprecation by using the order parameter in #[Hook].

But again, the problem goes away if we introduce the BC stubs.

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.

Which one is the one to review?

For now the idea was to put some options on the table so that we can think about how these could go wrong.

The last patch release of 11.1 is in a couple of weeks, so I'm starting to think we should not worry about this at all, and just continue on 11.2 and later. People writing brand new modules can require >= 11.2. People who want to convert existing modules to OOP hooks can require 10.5 | >= 11.2 (at least in 2-7 months). Having to add > 11.1.7 in .info.yml already limits the feasibility of using this and maintaining 11.1 compatibility.

Maybe the problem here is the deprecation of hook_module_implements_alter().
Now if you want to ship a module that supports 11.1.x and 11.2.x, and which needs to order hook implementations, you basically have to keep the HMIA without the #[LegacyHookModuleImplementsAlter], and eat up the deprecation warnings.

You can drop support for 11.1.x, but maybe there are websites which use that module but which also use other modules that are not yet ready for 11.2.x, for whichever reason.

Personally I think we went too far in that hook ordering issue. Or rather we started things in the wrong order.
The correct order would have been this:

1. Fix ModuleHandler to not group implementations by module, and thus allow proper ordering.
2. Provide an alternative to hook_module_implements_alter(), and a way to suppress specific hook_module_implements_alter() implementations in newer Drupal versions.
3. (future) Deprecate hook_module_implements_alter().

So now I see these options:

• Introduce minimal BC stubs for 11.1.x.
• Un-deprecate hook_module_implements_alter().
  This raises the question when we are ready to deprecate it. Maybe in 11.3.0?
• Do nothing, and force modules to choose or to eat up the deprecation.

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 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 see this is RTBC but I would still want to further review the more recent changes.

I am adding another branch where I add what seems to be the minimum we need.
This is mostly to see our options, I am undecided myself what is the best solution here.

We have to think a little bit what exactly we want to support:

Scenario 1:
A contrib module adds OOP hooks with #[Hook] attribute and using the order parameter.
It still wants to support older Drupal versions using by hook_module_implements_alter() and procedural hooks with the legacy attributes.
-> For this purpose, it is sufficient to backport only the order classes and the new parameter in Hook.

Scenario 2:
A contrib module introduces its own attribute classes that extend Hook.
At the same time it wants to be compatible with 10.5.x and/or 11.1.x.
-> It seems this would work fine with minimal BC stubs.

Scenario 3:
A contrib module uses the new FormAlter attribute.
At the same time it wants to be compatible with 10.5.x and/or 11.1.x.
-> This would require backport of that FormAlter attribute.

Scenario 4:
A contrib module uses the new Preprocess attribute.
At the same time it wants to be compatible with 10.5.x and/or 11.1.x.
-> This would require backport of that Preprocess attribute, AND all the new logic to register the theme hooks. So it is quite hopeless.

Scenario 5:
A contrib module or package wants to provide a functional polyfill for hook attributes, so that they work in 11.1.x and/or 10.x.
-> This would clash with any incomplete BC stubs for order classes and attributes.
-> I don't know if we care about this case.

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?

I am not saying that full revert is what we need to do, but now we have the option :) (draft MR)

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?

Link dependency issue.

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

(add supporting organization)

Follow-up here: 📌 Reduce hook attribute order BC stubs to the minimum necessary Active

Follow-up here: 📌 Reduce hook attribute order BC stubs to the minimum necessary Active

donquixote → created an issue.

(recap of what I said in slack)

My concern is this:

We have a grace period before 11.2.0 to tweak api details of the new attributes and API features we are adding.
But whatever we put into older versions of Drupal may get released before 11.2.0, as part of a minor release. At that point we can no longer change these, so we are cutting that grace period short.

Also for disruptive future changes to the new attributes, we now will need to cater to the 10.5.x, 11.0.x and 11.1.x as well.
Worse, we could have contrib modules written for 11.2.x that are compatible with 10.5.x but not with 10.6.x, or vice versa, because we just backported another change.

You are correct about order classes though, we do need these in any branch that collects hook attributes.
(more below)

I'm not sure what plan you are referring to

I remember a discussion where we determined that missing attributes are not a problem, only incompatible constructors.
The conclusion was that we update existing attributes in older versions where they exist, to allow alternative parameters, but that we don't backport all new attribute classes.

For concerns with phpstan, I am sure this can be handled in some way.

we backported Hook and LegacyHook to 10.4 as part of the original OOP hook initiative

I would have complained about backporting hook attribute to 10.x, but I probably did not pay attention at the time.

In this case the Order classes are required otherwise there would be a fatal, and since we backported Hook already we need to backport those as well to all branches.

This is correct, we do have to backport the order classes to any branch which has the Hook attribute class, OR any branch where we actually collect hook attributes.
But this also means: If we had not backported the Hook class to 10.x, we now would not have to create BC stubs for 10.x.

None of the plumbing for collection or ordering was backported, why are you concerned about backporting these classes, what could it possibly affect, they are not used.

The attribute classes are part of public API, the plumbing is not.
In general, releasing public API features is easy, the problem is that we cannot easily remove or change them later.

This MR did a lot more than what we need to for the BC support.
We absolutely don't need all these new attributes.
We only need to make the existing attributes compatible with new modules.
We did have a very clear plan for this, which we somehow forgot.

Missing attributes are ok, because we call ->getAttributes() with a type parameter. Only attributes classes with incompatible constructor signature are a problem.

We should either revert this and do again, or remove all the changes we don't need.
And before the next release for 11.1.x or 11.0.x. (if that has not already happened, I am not following).

The constants PREFIX and SUFFIX are not released yet, they may be removed in 📌 Rethink #[Hook] attribute inheritance Active , and they are not needed to make this BC support work.

Let's limit this change to what is necessary, and also apply that correction in 11.1.x.

Also the FormAlter is not released yet, and there is no reason to add it in 10.5.x or 11.1.x.
What I remember from earlier discussions is that missing attributes are not going to cause problems, only existing attributes with incompatible signature. Or do I remember it wrong?

Also RemoveHook, ReorderHook etc don't need to be added here, I think.

This is a code style change, fully equivalent to original code (except support for some old php versions), so it does not really tighten anything.
Changing title.

In general I am ok with this change.

Generally I support this, thank you!

dww → credited donquixote → .

donquixote → created an issue.

I don't like that PREFIX and SUFFIX solution.
Follow-up here: 📌 Rethink #[Hook] attribute inheritance Active

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

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.

Where did the order parameter go?

Follow-up here: 📌 Rethink #[Hook] attribute inheritance Active

donquixote → created an issue.

Choosing a more open-ended issue title

The ProceduralCall class is gone.
(TBD if we want to keep it for BC, or do that removal in a follow-up)

Actually I would be fine to do this in a follow-up.
When we do this, we also want to review the ordering attributes, where currently you would do #[ReOrderHook(ProceduralCall::class, 'foo', ..)]. See 📌 [pp-1] Create new attribute to make reordering procedural hooks easier. Postponed

Its not NR

sorry :)

For now we need to target procedural hooks by setting ProceduralCall as the class name, which is awkward.
With 🐛 HookCollectorPass registers event listeners but they do not follow the required calling convention Active , which includes 📌 Drop/replace the ProceduralCall class for hooks Active , we drop the class ProceduralCall.

We then either need #[ReOrderHook(function: ...)], or we need a separate attribute #[ReOrderProceduralHook($function)] or #[ReOrderHookFunction($function)]. Or we could pass $module instead of $function.

The main argument for a separate attribute would be to keep the other arguments required in the regular #[ReOrderHook] attribute.

In the same way we would need OrderBefore(function: ...) or OrderBeforeProcedural($module)

I came up with the following text:

  /**
   * Sets an explicit list of currently active modules.
   *
   * After the module list has been replaced in this way, calls to
   * ->getModule(), ->getModuleList() and ->moduleExists() will behave according
   * to the new module list.
   * 
   * Methods like ->load() and ->loadInclude() will behave according to the new
   * module list, except for modules that were already included.
   * 
   * Hook implementations from modules not in this list are skipped on 
   * subsequent calls to ->alter() or ->invoke*().
   * 
   * However, no new implementations will be added from modules that were not
   * part of the initial list passed in the constructor. Also, the order of
   * implementations does not change, except for alter hooks with a 
   * multiple-value type argument.
   *
   * Restoring the original module list will restore the original hook
   * implementations.
   *
   * @param \Drupal\Core\Extension\Extension[] $module_list
   *   An associative array whose keys are the names of the modules and whose
   *   values are Extension objects.
   */
  public function setModuleList(array $module_list = []);

But, I think the following would be more suitable, assuming we would create that new test method:

  /**
   * Sets an explicit list of currently active modules.
   *
   * Calling this directly may not have the result one would expect, and should
   * be avoided outside of Drupal core.
   *
   * For more details see the respective test method.
   *
   * @param \Drupal\Core\Extension\Extension[] $module_list
   *   An associative array whose keys are the names of the modules and whose
   *   values are Extension objects.
   *
   * @see \Drupal\KernelTests\Core\Extension\ModuleHandlerTest::testSetModuleList()
   */
  public function setModuleList(array $module_list = []);

Both of this sounds like follow-up material to me.

So this might be the only remaining useful test case?

What I wonder is whether there is any code out there that relies on the reset in the old ModuleHandler instance after install or uninstall.
Such code would probably already be broken..