🇩🇪Germany @donquixote

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

(cross post, but still valid)

Long term we might drop the setModuleList() and resetImplementations().
Instead, we could:

- Accept that module_set_weight() and install/uninstall have no effect on the old container.
- For the theme registry on update page, we could have an immutable setter ModuleHandler->withReducedModuleList(['system']), which would return a clone with the reduced module list and starting with empty hook cache properties.

This would mean changing the ModuleHandlerInterface, so perhaps in 12.x.

But I think the change proposed here and the tests make sense in the spirit of preserving or restoring existing (old) functionality, and for the sake of consistency. As long as the method exists, we want it to behave as one would expect or as it used to.

@godotislate

Searching for ->setModuleList(, outside tests:
- In module_set_weight() it updates the configuration and sets the list in the existing ModuleHandler instance, but does not trigger a rebuild.
- In ModuleInstaller::doInstall() and ::uninstall(), it updates the list in the existing ModuleHandler instance, and we get a rebuild later (I suppose).
- In Drupal\Core\Theme\Registry::get(), if the kernel is an UpdateKernel, it temporarily sets the module list to be just the system module.

Searching for ->resetImplementations(, outside tests:
- In ModuleHandler::add(), which we are going to remove.
- In ModuleHandler::setModuleList(), see above.

I don't know about contrib, but it could only be some specialized module that wants to change how hooks or modules work.

So it seems that in older versions, we would immediately get new/updated hook implementations on install and uninstall with the old ModuleHandler instance from the old container.
With newer versions of Drupal this would already be problematic because even procedural implementations may call \Drupal::service() and require services that only exist in the newly built container. Actually this might work if the new container is already built and only the ModuleHandler is still the old instance.
With OOP hooks, all the services used in a hook implementation are coming from the same outdated container as the ModuleHandler itself. So we can remove implementations on uninstall but never add them on install.
Even the ModuleHandler::add() which only adds procedural implementations can only work if the new container is already present, for added procedural implementations that call Drupal::service().

All review points have been addressed.

But we should do the following issues first to start at a good place:
📌 Enhance array docs in ModuleHandler Active
🐛 ModuleHandler::resetImplementations should reset $this->invokeMap Active

Not sure how else to test resetImplementations. I see that module_set_weight() has a called to setModuleList(). Are there tests around changing module weights and how that affects hook ordering?

Actually this should have no effect on hook implementation order, which is determined at container build time.

Is this a regression or an intentional change? Before, module weights would be part of what determines the order in which hook implementations run.

Currently, the module weights in configuration determine the order.
If you call module_set_weight(), the new order will become active for hooks after a container rebuild.

Originally, before OOP hooks via event dispatcher were introduced, it would have been possible to change the module order at runtime.
Now, it is only possible to remove modules at runtime (and thus remove the implementations), but not to change the order at runtime.

I suspect the change came with the first move to OOP hooks.
But it is also possible that it came with the hook order MR.
It reveals a lack of tests, which imo should have been added before the first OOP hook MR.

We had this issue but the MR was a bit complex, and not sure it would cover this case. 📌 Improve unit test coverage for ModuleHandler Active
Now I think the best is to improve coverage piecemeal.
But to really see the breakage we already may have done in the past, we need to open draft/test MRs to older versions to compare.

Not sure how else to test resetImplementations. I see that module_set_weight() has a called to setModuleList(). Are there tests around changing module weights and how that affects hook ordering?

Actually this should have no effect on hook implementation order, which is determined at container build time.
There is no code in ModuleHandler that would change the order.

Found it.
So the one place where ->setModuleList() is called outside tests and install/uninstall is in the theme registry, for the update page:

   if ($this->kernel instanceof UpdateKernel) {
      $module_list = $this->moduleHandler->getModuleList();
      $filter_list = array_intersect_key($module_list, ['system' => TRUE]);

      // Call ::build() with only the system module and then revert.
      $this->moduleHandler->setModuleList($filter_list);
      $this->build();
      $this->moduleHandler->setModuleList($module_list);

Our test should replicate that.

We can fix 🐛 Regression: Empty hook implementation lists are not cached in ModuleHandler Active as part of this.
This will make the behavior more symmetric and makes it easier to write the test.

I enhanced the tests to better reflect what resetImplementations does and when it is called.
I might still have some misconceptions in the comments.

Not sure how else to test resetImplementations. I see that module_set_weight() has a called to setModuleList(). Are there tests around changing module weights and how that affects hook ordering?

We could use module_set_weight() in the test. But then we need two test modules with their hook implementations, and test the order.

I was originally thinking that the main use case is update page where only system module hook implementations are invoked.
But I don't see ->setModuleList() being called in that context.

If we want to support local development websites and intranets, we could still go with a public/private keypair approach.
Only in these cases, we need to be able to directly put the public key into localize.drupal.org, instead of having it fetched from an API endpoint from the contributing site.

And to easily support multiple local Drupal sites (e.g. something.ddev.site, or just "localhost"), the keypair could be shared between those instances, and localize.drupal.org could store the public key without associating it with a specific hostname.

----

There is a big design space between the originally proposed token solution and the specific keypair solution proposed in #16.

Secrets per user or per site

We can have a solution with per-user secrets (e.g. the token proposed here), or one with per-website secrets.
With per-user secrets, we either want some kind of encrypted field, or we accept the risk that these are compromised.
Also the user needs to trust the website with (one of) their secret tokens.
Secore storage per user is going to be harder than securely storing a site-level secret.

(Currently the consensus seems to be that encryption of tokens is not necessary)

However, if we have site-level secrets, we do need some other way to associate user accounts between localize.drupal.org and the contributing website, while preventing improper impersonation, which leads to the complex data model in #16.

There could be a shortcut for this data model for local sites where each account is basically the same user on localize.drupal.org.

Keypair vs token + hash

A keypair would be generated on the contributing site, and the private key does not need to be copied around. The private key is not sent with requests, only the generated signature.
A token would be generated on localize.drupal.org and does need to be copied around manually. It also has to be sent literally with every request.

In theory we could have per-user key pairs generated on the contributing website, to avoid the copying around of secrets. But this would again require secure storage per user (if we care about that).

This should be fixed as part of 🐛 ModuleHandler::resetImplementations should reset $this->invokeMap Active .

donquixote → created an issue.

Did the PREFIX and SUFFIX need backporting?

I am not sure if we need the change from last commit "Replace modulehandler" in 3514197-fix-moduleHandlerGet
https://git.drupalcode.org/project/drupal/-/merge_requests/12027/diffs

In fact I think it is counter-productive.
The method resetImplementations() is meant to clear the cache property within one ModuleHandler instance.
But the proposed change gets a new ModuleHandler instance after the container was rebuilt, which always has the updated lists.
That new version of the test would pass even if resetImplementations() does nothing.

At first I was thinking that test is not enough.
But it actually failed without the last change, which makes me think it is not so bad :)

#29/#30 seem like unrelated follow ups to me. The object/stdClass example is in the original text and unchanged in the proposed text.

Ok to me if we do this in follow-ups.

I pushed a solution to that old branch, but realized I should rather create a new branch after that old MR was merged and then reverted.

We need to reset more properties now that 📌 Hook ordering across OOP, procedural and with extra types Active has landed.

fwiw I support this move in general, but would not mind to see it be even stronger as mentioned above :)

donquixote → created an issue.

Now we have the path forward to drop a lot of procedural code (OOP hooks, OOP theme preprocess), there is no more need to move existing procedural code into namespaces.
It would be a lot of disruption for no or very little benefit.

So, I am ok to see this closed.

I discussed with @ghost of drupal past in slack about the ModuleHandler constructor BC support.

Technically it could be possible to do it, but it would require some advanced acrobatics to reconstruct the list.
In 11.0.x -> 11.1.x we already broke that signature once.

Our current consensus is to break it again, without BC support for custom or contrib code that extends ModuleHandler.

We only need to make sure that you can successfully run container rebuild (drush cr, or by other means?) after code update.

Actually one round of review feedback could be nice.

It's green!
Now it is time for the BC police.
I am changing the signature of ModuleHandler, we need to see how we can make this BC friendly.

Discussion is here, ✨ Adopt phpstan phpdoc types as canonical reference of allowed types Active .

object (NOT "stdClass")

I would disagree with that point.
Instead:

Use `object` for values that can be instances of any class.
Use `\stdClass` for values that can only be instances of stdClass.

In addition, I would like to see a sentence saying that we should always prefer the most specific type that the context allows.
E.g. `array` or `list` instead of `string[]`.

An exception can be very complex arrays like render arrays, or theme registry items.
Especially when there could be more keys added by modules.
Here the doc would be like `array{'preprocess functions': list, ...}` which is just too much.

I notice that a bunch of arrays in this MR do not have a more specific phpdoc type.
In some cases it is not practical to go all the way, e.g. to document every property of a theme registry item.
But in general, we should always use the phpstan-like array type doc for any new array property, parameter or return we introduce.

Quick observation from my experiments:

We register the hooks as event listeners using service tags.
This is why it is not straightforward to register a function, even though symfony EventDispatcher by itself does allow that.

This means we better postpone this change until after 🐛 HookCollectorPass registers event listeners but they do not follow the required calling convention Active .

donquixote → created an issue.

Some things should be discussed further:
- Breaking changes in ModuleHandler constructor parameter signature.
- Whether the [$hook => ["$class::$method" => $module]] structure is a good idea for the parameter.

First attempt, including changes from 📌 [pp-1] Explore adding an ImplementationList object for hook implmentations Active

donquixote → made their first commit to this issue’s fork.

Let's have one round of review, to see if people agree with the direction or have different ideas where this should go.

donquixote → made their first commit to this issue’s fork.

For now this MR adds a dependency and a bunch of new code.
That new code looks quite complex if you are not familiar with the symfony/runtime package (which I am not, at this point).

The supposed payoff is that we might later be able to split logic out of DrupalKernel.
But the current MR does not make this obvious.

We should have a proof-of-concept MR that shows how this change would help us to refactor and simplify DrupalKernel.
(This is the only reason stated in this issue, I don't see other benefits mentioned)

We also need to evaluate if there are alternatives (as in custom code) that do not add a dependency.

Not quite, procedural hooks are not deprecated yet.

Oh, you are right.

But I struggle to see the use case where one would have both an oop and procedural for the same hook in the same module.

Quite simple, you have the procedural implementation which has not been migrated yet, then you add the OOP implementation which may be needed to cover a different problem.

E.g. you might have a mymodule_node_update() and then you add a \Drupal\mymodule\Hook\MyHook::nodeUpdate() to do something unrelated. You don't want to touch mymodule_node_update() because you were asked (and paid) to work on a very specific problem, and changing existing code is out of scope.
In most cases the order of these two won't matter, if they are for unrelated problems. But you could have funky side effects that depend on the order.

They said that this was for an edge case, so the CR may not be needed.

Whether this is an edge case is in the eye of the beholder.
It applies when a module implements the same hook twice, one time as a function and another time as a method, and without using the #[LegacyHook] attribute. So it would already be doing something deprecated.

Note the proposal I made in https://www.drupal.org/project/l10n_client/issues/3395488#comment-16065118 📌 Localize client authentication with d.o infrastructure Active

donquixote → changed the visibility of the branch 3472140-remove-user-field-on-uninstall to hidden.

I think a README for the submodule would be fine.

I changed my mind about this.

The existing README.txt already needs some work, e.g. to install it mentions Admin / Modules, when since D8 it is "Extend".

If we want to create a README.md or .txt in the submodule, we have to decide what goes there, and possibly change the main README.txt too.

In the spirit of smallest change possible, in this issue we can just add an UNINSTALL section to the existing README.txt.

A lot of parts of the above could be covered as separate modules:
(and some might already exist)

• Public/private key storage with endpoint to get the public key.
• Entities for authorized websites with public key.
  Possibly with different "scopes", where translation contribution is only one scope.
• User field for a linked account on a separate website.
  The field and field type could be in different submodules to avoid the dependency trap.