🇩🇪Germany @donquixote

We'll definitely need sign off on collapsing the alters.

We should have asked for this before 11.2.0 release.
(And we should have added a test for this case)
Now if we change it back to have implementations repeated, we break with the behavior of 11.2.0.

The workaround exists since Drupal 9.0.* (or earlier).

Behavior in older Drupal versions

In Drupal prior to 11.2.x, when calling ->alter(['a', 'a'], ..) with the same alter type repeated, the implementations would simply be repeated.
The order was by module and then by alter type:

class HookAlterTest extends KernelTestBase {
  protected static $modules = [
    'procedural_hook_test',
    'system',
  ];
  public function testAlter(): void {
    $this->assertAlterCallOrder([
      'procedural_hook_test_test_main_alter',
      'procedural_hook_test_test_other_alter',
      'procedural_hook_test_test_main_alter',
      'procedural_hook_test_test_main_alter',
      'procedural_hook_test_test_other_alter',
      'system_test_main_alter',
      'system_test_main_alter',
      'system_test_main_alter',
    ], ['test_main', 'test_other', 'test_main', 'test_main', 'test_other', 'test_x']);
  }
  [...] (not sharing the full class here)

Behavior since 11.2.x

Since 11.2.x, every implementation is only executed once, and we see the notice as reported here.

  public function testAlter(): void {
    $this->assertAlterCallOrder([
      'procedural_hook_test_test_main_alter',
      'procedural_hook_test_test_other_alter',
      'system_test_main_alter',
    ], ['test_main', 'test_other', 'test_main', 'test_main', 'test_other', 'test_x']);
  }

However, both the deduplication and the notice were actually meant for a different case, where the same method is registered for two distinct alter hooks, which are then executed together with ->alter().
E.g. ->alter(['a', 'b'], ..) with #[Hook('a_alter')] and #[Hook('b_alter')] on the same method.
The case with ->alter(['a', 'a'], ..) was not really considered at the time. This is why the notice looks so strange.

Solution

Now we need to decide what should be the "correct" behavior.
I think it makes sense to execute every implementation only once.
But we don't want the same notice, if the duplication is caused by a duplicate alter type passed to ->alter().

The easiest might be to simply call array_unique() in ->alter().

class ModuleHandler ... {
  [..]
  public function alter($type, &$data, &$context1 = NULL, &$context2 = NULL) {
    [..]
    if (is_array($type)) {
      $type = array_unique($type);
      $cid = implode(',', $type);
    }
    else {
      $cid = $type;
    }

Going for a simpler and more confident title.

I think for now we want to make sure it works as advertised for for a limited set of supported scenarios.
Later we can incrementally add support for advanced autowire features.

donquixote → created an issue.

From a functional perspective, it is fine to have field group as part of Drupal CMS.

From an architectural perspective, the structure of entity form and view display should support grouping out of the box, so that field_group does not need the acrobatics that it currently does.

We should either reopen this and move to core queue, or create a follow-up issue.

I just added something in "Proposed text".
But tbh, perhaps we can just drop this part instead, as it is already implied by other parts, as pointed out by #19.

I think I would rather disallow type-hinting with \stdClass at all to resolve this, then it not being allowed in the PhpDoc would not be a problem. Type-hinting on \stdClass seems like bad practice to me and it should be discouraged if it isn’t yet in our standards.

This proposal from #10 seems odd.
The doc type (and also the php type hint) should describe what is, not what we would wish to see instead.
At the time the code is refactored to allow different object types, the doc and the php type can change accordingly.

Thanks!

Actually on second thought this might need a CR since we're adding a new object and we're changing some methods.

Do we write change records for protected methods and protected properties?
And do we write change records for new classes marked as internal?

It was a helper for Drupal install and updating and wasn't even working there for ages.

It was working before the OOP hooks via EventDispatcher. So in 11.0.

Unless someone pops up though with a valid use case this is already deprecated. I'm not sure it's worth the effort to change.

Normally "deprecated" code still works as advertised or as expected, you should stop using it to prepare for the next major version.

yes.

Looks good.
Do we need to add tests?

My main concern here is whenever we refactor this, e.g. in 📌 [pp-1] Explore adding an ImplementationList object for hook implmentations Active , we are replicating severely broken functionality.

I think this is a dead end issue :)
There are always services in the call stack, and so far this seems to be ok.
The services from the old container create a new container, and that's how it works.

I think this type of config was never really meant to be handled in hook install but worked.

I don't think the creators of this hook had any preference whether or not people use it to alter configuration from config/optional from another module.

I'm wondering if it's worth considering a new hook: hook_post_install_optional

To me it seems like a clear regression.
We should fix core, rather than asking people to change their code.

Here is a list of commits for ModuleInstaller since 11.1.8.

* 4837c52b Issue #3497105 by quietone, borisson_: Fix LineLength for inline comments
* 101e0606 Issue #3496558 by alexpott, godotislate: Install entity definitions for all modules in batch install before simple config
* 6e7715a8 Issue #3492282 by nicxvan, longwave, catch: Refactor away ModuleInstaller::invokeAll()
* 59ce5ede Issue #3494212 by godotislate, alexpott, larowlan: Modules that provide fields for entities of other modules can't be installed
* 57583222 Issue #3487154 by quietone, daffie, smustgrave: Fix Drupal.Commenting.FunctionComment.MissingReturnComment in core/lib/Drupal/Core/A-E
* 58be5f12 Issue #3492235 by catch, longwave, mikelutz: Default container_rebuild_required to FALSE
* 16967de8 Issue #3492705 by catch, nicxvan, mikelutz, longwave: Install modules with container_rebuild_true set by themselves
* f8090e61 Issue #3416522 by catch, alexpott, longwave, phenaproxima, nicxvan, wim leers, smustgrave, larowlan, berdir, godotislate, dww: Add the ability to install multiple modules and only do a single container rebuild to ModuleInstaller
* eab46c49 Issue #3486995 by nicxvan: Clean up how ModuleInstaller invokes hooks around installing other modules
* 76323d65 Issue #3490296 by nicxvan: Mark hook_install_tasks and hook_install_tasks_alter as procedural only
* 2c9e1fa9 Issue #3486462 by nicxvan: Support #Hook for several hooks called by ModuleInstaller

donquixote → created an issue.

donquixote → created an issue.

nicxvan → credited donquixote → .

The "fully-qualified type" has been kept because that was agreed to earlier in the issue and by a committer meeting.

My problem with this "fully-qualified" is that this can only apply to classes/interfaces.
E.g. if you have this:

/**
 * @param int $count
 * @param callable(array<string, \Drupal\node\NodeInterface>): void $callback
 * @param \Drupal\node\Whatever $whatever
 * @param \Closure $another_callback
 */

Here some parts of these @param docs have fully-qualified names, for other parts this term does not apply.

To recap from https://www.php.net/manual/en/language.namespaces.rules.php:

• A "fully qualified name" can be a class, interface, trait, enum, namespace, function or constant (or other, perhaps?) name with namespace and leading namespace separator '\".
• A "qualified name" can be a class, interface, trait, enum, function or constant name with namespace and without leading namespace separator '\".
• (A "fully-qualified class name" (FQCN) is a class name (or interface, trait, enum) with namespace and leading "\".)
• (A "qualified class name" is a class name with namespace but without the leading "\".)
• A built-in type does not have a fully-qualified name.

As such, it does not really make sense to ask for the entire type to be "fully qualified", only specific namespace-aware symbols within the type can be fully-qualified.
And here I thought it is preferable to just define in one place that all namespace-aware symbols in any doc type must be fully-qualified, instead of repeating this everywhere.
(Of course we might at some point relax this requirement, but this is out of scope for this issue.)
(I think the main argument to use FQN is to not have imports only for doc types.)

We are still losing parts of the existing text, without an explicit intention to do so.
Especially the points about @return tag being required.

Let's try again.

• Each parameter of a function must be documented with a @param tag (see exceptions below).
• If a function has a return value, it must be documented with a @return tag (see exceptions below).
• If there is no return value for a function, there must not be a @return tag.
• For most functions (see exceptions below), summary lines must start with a third person singular present tense verb, and they must explain what the function does. Example: "Calculates the maximum weight for a list."
• Functions that are easily described in one line may be documented by providing the function summary line only (omitting all parameters and return value).

• Each parameter of a function must be documented with a @param tag (see exceptions below).
• A @param tag must have a description, unless all of the following are true:
  1. The parameter is type-hinted.
  2. The parameter type is a single class, interface, or scalar and does not allow null.
     • For a class or interface, the parameter name is derived directly from the type name. For example, >code>QueueWorkerManagerInterface $queueWorkerManager.
     • For a scalar, the parameter name is self-documenting.
  3. A description would not add useful information.
  4. The type in the @param tag is identical to the parameter type in the PHP function signature.
• If a function has a return value, it must be documented with a @return tag (see exceptions below).
• If there is no return value for a function, there must not be a @return tag.
• 1. The return is type-hinted.
  2. The return type is a single class or interface and is not nullable.
  3. A description would not add useful information.
• For most functions (see exceptions below), summary lines must start with a third person singular present tense verb, and they must explain what the function does. Example: "Calculates the maximum weight for a list."
• Functions that are easily described in one line may be documented by providing the function summary line only (omitting all parameters and return value).

donquixote → created an issue.

Another problem with the current proposal is what is being replaced.

In fact, the "Current text" says nothing about parameter description or return description.

Further below we have separate sections about "@param" and "@return":

@param: Function parameters

The @param tag is used to document a parameter within a function docblock.

Syntax examples:
[...]

Syntax notes:
The @param tag is followed by an optional data type indicator, then the variable name of the parameter, and then a newline. The following paragraph is considered by the API module to be documentation. The API module formats the information on the same line as the @param using a strong HTML tag.

Drupal standards: The documentation is indented two spaces (see example). Data types are required to be included as of Drupal 8.x. Optional parameters are indicated by (optional); include information about the default value only if it is not obvious from the function signature.

Even here it is not fully clear to me that the param description is required.
It says "The following paragraph is considered by the API module to be documentation.", but does this mean it is required?

Perhaps some of the changes can move here. But I am not really sure.

Maybe 2 should be

the parameter is a single type

That covers is not null able, classes and escalates right?

It should, but I can see how some people might find it ambiguous when looking at `?MyClass` types.
I always prefer the explicit "don't make me think" style, even at the cost of having some perceived redundancy.
It is like in Monopoly go to jail card, which says "Do not pass go" and then "Do not collect $200".

I have updated the IS with the proposed text in #154 and the suggestion in #159.

The current version is contradictory.
We first talk about "class or interface", then later we talk about scalars.
Either we omit scalars from the proposal (which I prefer), or we have to include them in condition 2.

-----

Another problem with the current proposal is what is being replaced.
If we truly replace the entire section in "Current text", then we start with "An @param tag must have a description, unless ...", without having specified when a "@param" tag is needed.

The "Current text" says "Each parameter of a function must be documented with a @param tag (see exceptions below).". We seem to drop this completely.

Even if we do want to relax this requirement (which I would consider wrong for the scope of this issue), there should at least be something before we start talking about the description for a @param tag.

If would be nice to be able to omit a (IMO useless) return doc like this:

* @return string|null
  The title, or null if there's not one.

I don't think it is useless.
In many cases, the NULL case is actually more interesting.

E.g. "The entity identifier, or NULL if the object does not yet have an identifier.".
For entities, this generally means that the object is a "stub" and has not been saved in the database yet.

"The title, or null if there's not one."
This could mean the specific entity does not have a title, or that entities of this type never have a title.
Given that this description will be in an interface method, it is worthwhile to have a description.

I think it is best to move piecemeal on relaxing the doc requirements, and for each step we can look at specific examples.

(I was going to post the below yesterday but forgot to submit)

@donquixote, I really think saying "class or interface name" is a lot clearer than "type name", even with your example with ducks.

I assume it would apply to this part:
"The parameter name is derived directly from the type name." -> "The parameter name is derived directly from the class or interface name."
In that case, +1.

I think we do want to include scalars in the proposal; one of the biggest usecases for this is for constructor property promotion I think. Maybe we have a separate issue coding standards about that, but I thought this was part of a list of related policy changes for it.

When I search `@param int \$\w+` in Drupal core and in vendor/symfony, I see many places where I either want to keep the existing description or see it improved. And when just looking for `int \$\w` in vendor/symfony, to find undocumented parameters, in many cases I would like to see a description.

Of course there are cases when the parameter name is clear enough, or when a description would be redundant with the description on the method itself (e.g. for a setter method).

Having a general rule to always require a description is annoying, but if we leave it to the developer, it cannot be enforced by automatic tools. A rule at least forces the developer to think, and it gives the reviewer some leverage.

Also keep in mind that, with named argument calls, it is harder than before to rename an existing parameter that is considered part of a public API. As a consequence, we will have parameters names that are not ideal.

However, since it is easy for me to explain what the names should be for classes and interfaces, but more difficult to explain it for scalars, maybe we should split the policy proposal into two steps?

I think this would be a good idea, yes.
The rule proposed here when this issue was started to determine when to omit the description does not really work for scalar types.
The newly proposed rule "the parameter name is self documenting" would work but is not enforceable.

So I would prefer to handle this in a separate issue.

@donquixote, I don't understand. I didn't think we were talking about phpdoc; why did you think I did?

This was not about you, it was about others who may read these rules and think that "type-hinted" could refer to the param doc type.
But maybe saying "type-hinted" is clear enough.

Can we add redirects?

I see the "scalar types" was added in #129, and I don't see why.
https://www.drupal.org/node/3376518/revisions/view/13999604/13999612 →

For my taste, we should simply drop that part:

A "@param" tag must have a description, unless all of the following are true:

• The parameter is type-hinted.
• The parameter type is a single class or interface (without allowing null).
• The parameter name is derived directly from the type name.
  • E.g. "QueueWorkerManagerInterface $queueWorkerManager".
• A description would not add new information.
• The type in the @param tag exactly replicates the parameter type in the PHP function signature.

For the first condition, "The parameter is type-hinted.", how could we make it more clear that this is about the php signature and not the phpdoc type?

@xjm

Like so:

A parameter description may be omitted, if all of the following are true:

The parameter is type-hinted.
The parameter type is a single class, interface, or scalar type.
The parameter name is derived directly from the type name.
A description would not add new information.
The types in the @param tags exactly replicate the parameter types in the PHP function signature.
Otherwise, a parameter description must be included.

That removes heaps of negation, and reduces it from two sections to one. We could do likewise with the return section.

I like reducing the negation.
We could also drop the last line of "otherwise, ..." if we use "unless".

A "@param" tag must have a description, unless all of the following are true:

The parameter is type-hinted.

The parameter type is a single class, interface, or scalar type.

The parameter name is derived directly from the type name.

A description would not add new information.

The type in the @param tag exactly replicates the parameter type in the PHP function signature.

@xjm

on arrays we have "The parameter type is not a single class, interface, or scalar type." which means all arrays would need @param docs still.

Yes the way I understand it, this would only require description on arrays.

I would disagree. I would argue that on most scalar types there is something to document:
- For arrays we want to know the structure, and what the keys and values represent.
- For strings, we may want to know if it is a machine name or a human-readable label, whether it is raw input or has been sanitized already, etc.
- For boolean, we want to know what is the meaning of true or false.
- For integer, we want to know whether it is seconds or milliseconds.

See what I said in #16.

The parameter name is derived directly from the type name.

The idea here was this, if that is still valid:
"DuckInterface $duck" -> does not need a description.
"DuckInterface $animal" -> needs a description.
"QueueWorkerManagerInterface $queueManager" -> needs a description
"QueueWorkerManagerInterface $queueWorkerManager" -> does not need a description
"UnroutedUrlAssemblerInterface $url_assembler" -> needs a description
"UnroutedUrlAssemblerInterface $unrouted_url_assembler" -> does not need a description

Now up to you what you think about it :)

@krystalcode (#106)

The way Git merges changes does not have much to do - if anything - with the sorting of the contents of the code. Conflicts most commonly happen when you edit the same lines.

The problem with git vs imports is not just about actual conflicts.
If there is no strict order, there can also be duplication:

commit 1:

use N\A
+ use N\NewClass;
use N\B
use N\C

commit 2:

use N\A
use N\B
use N\C
+ use N\NewClass;

merge 1 + 2:

use N\A
use N\NewClass;
use N\B
use N\C
use N\NewClass;

Let's say we merge this, we notice the problem, and we want to clean it up.
But we actually have two branches that do this clean-up:

cleanup branch 1:

use N\A
- use N\NewClass;
use N\B
use N\C
use N\NewClass;

cleanup branch 2:

use N\A
use N\NewClass;
use N\B
use N\C
- use N\NewClass;

on merge, both of them are gone, and the import will be missing:

use N\A
use N\B
use N\C

Often enough, this does not happen in actual merges, but in a rebase or cherry-pick.
E.g. commit 1 above might be from a feature branch, then commit 2 is from a different feature that is merged upstream. Then branch 1 gets rebased on upstream, and what appears as "merge 1 + 2" above instead occurs as a commit in the rebase.

Having a strict order avoids these problems.
When there is ambiguity, the merge or rebase will see that, and the developer will be forced to sort it out.

And what would cause Core to not come first? Every contrib module import is already after Core, as its namespaces are lowercase.

https://git.drupalcode.org/project/aws/-/blob/2.0.x/src/Entity/Profile.p...

In other words, code outside the Drupal namespace, mostly packages that live in /vendor/.
In fact, some 3rd party package namespaces will come before "Drupal\...", others will come after.
All of this is totally ok, and is not any different from e.g. symfony packages, where imports are sorted like "use Composer\...", "use Psr\...", "use Symfony\...", "use Twig\...".

These imports are going to be mostly managed and read by tools like IDEs or phpcbf.
As a developer, I don't want to scroll up and look at the imports.
When I write "new MyClass", the autocomplete will propose a matching qualified class name (class with namespace), and insert the import at the top.

Any convention we introduce here has to be one that is supported by commonly used tools, and does not require new custom sorting specific to Drupal.

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.