🇩🇪Germany @donquixote

The page that you are testing, are any hooks called on a warm cache?
And is ModuleHandler instantiated at all in these requests?

If ::addListener() calls are what slows down these requests, maybe we could go even further and have a separate event dispatcher with only those events that are commonly invoked in requests with warm cache?

can we just have two event dispatchers?

We can.
But what does EventDispatcher give us for hooks? We don't really use it for anything other than collecting callbacks.
And then we cannot just use the callbacks, we have to add meta information about which module name to pass to a hook call, which file to include for a procedural implementation, and ordering rules to reorder "mixed" alter implementations.
I still think the change proposed here is an overall improvement.

I don't mind if you want to do this change as an incremental improvement.
It is certainly simpler in terms of how much code is changed.

The conceptual difference is that the class and method targeted by ReorderHook must match the hook assigned to that class and method as well. (in the test reorder is looking for test_b_subtype_alter and the hook for that class is actually test_a_supertype_alter)

In donquixote's approach this reorder still has an effect, I think that is also a bug.

I don't think it should be seen as a bug.
(the cross-hook ordering, not the TypeError)
The desired behavior in this scenario was not discussed in depth at the time we introduced these attributes.
The current behavior seems just as valid as the new behavior proposed by @nicxvan.
Changing it now does somewhat break existing behavior, even though it is a niche case.

This said, I could see one argument to see it as a bug: There is inconsistency with #[RemoveHook], which does not show this cross-hook behavior.

@donquixote mentioned in slack that he'd like to add a test for:

I added the test case for this in 3536470-ReOrderHook-on-non-implemented-hook-current-behavior and in 3536470-ReOrderHook-on-non-implemented-hook.

The goal was to provide a more relevant use case where reordering across hooks might actually be desirable.
In the test it is used to "reinforce" the ordering on a base alter hook implementation so it can overrule ordering for subtype alter implementations.

Overall I feel like our attributes are not well-suited for these competitive situations.
What if you really want to be "last, after X which also wants to be last, but before Y, which also wants to be last"?
And what if X and Y actually come from different hooks, which are all called togeher in ->alter([*, *, *], ..)?

The OrderAfter and OrderBefore were meant for this purpose, but I don't believe in them.
They are going to be very unreliable if the target implementation we order against is also reordered. Now the success of OrderAfter depends on _when_ that rule is applied.
If A.Last is applied first and then B.OrderAfter(A), we get ***, A, B.
If B.OrderAfter(A) is applied first and then A.Last, we get **, B, **, A.

The most reliable solution would be numeric weights/priorities.
We don't need to give weights to implementations, instead we can give weights to ordering rules.
(not in this issue, of course)

If we have a weights system, the use case mentioned before would mostly disappear.

Until then, maybe we can say that the relative order of implementations with competitive Order::First or Order::Last is not fully guaranteed.

donquixote → changed the visibility of the branch 3536470-ReOrderHook-on-non-implemented-hook-current-behavior to active.

donquixote → changed the visibility of the branch 3536470-ReOrderHook-on-non-implemented-hook-current-behavior to active.

The new version is a bit verbose but hopefully more clear.

The test-only branch illustrates the current behavior.

if you try to affect the order relative to a hook that doesn't exist, this still affects the order of other hooks that do exist.

Not really - unless I misunderstand.

When I see "order relative to", I think of #[ReorderHook(..., new OrderBefore(...))].
This is not what this is about.

The best example is in comment #17.

As you know, ->alter() can be called with more than one "alter type", which then maps to more than one alter hook.
E.g. ->alter(['form', 'form_node_form', 'form_node_article_form'], ..).
The hooks are 'form_alter', 'form_node_form_alter', 'form_node_article_form_alter'.

Each of these hooks can have order operations that were declared either with #[ReorderHook], or by setting an order in the #[Hook] attribute for a specific implementation.
All of these order operations will be applied when the implementations from these multiple hooks are "merged" at runtime.
When this happens, an order operation from e.g. 'form_node_form_alter' is allowed to target and reorder an implementation of 'form_alter'.

How?

Most of the #[ReorderHook] will target an implementation of the same hook that is passed in the first parameter of the attribute.
E.g.
- #[ReorderHook('form_alter', MyClass::class, 'formAlter', Order::First)], or
- #[ReorderHook('form_node_form_alter', MyClass::class, 'formNodeFormAlter', Order::Last)],
assuming that ::formAlter() implements hook_form_alter(), and ::formNodeFormAlter() implements hook_form_node_form_alter().

However, a #[ReorderHook] can target (and thus reorder) an implementation from a different hook.
This operation is only applied when both hooks are used together in an ->alter() call.

E.g. a completely silly example would be completely unrelated hooks #[ReorderHook('mail_alter', SomeClass::class, 'toolbar', Order::First)], assuming that SomeClass::toolbar() implements hook_toolbar().
These are never used together in ->alter(), and the second one ('toolbar') is not even an alter hook.
Therefore this #[ReorderHook] is completely pointless and has no effect.

However, we can #[ReorderHook('form_node_form_alter', MyClass::class, 'formAlter', Order::First)].
Here the target hook is 'form_node_form_alter', but the target method implements 'form_alter'.

What could be the intent behind this?

• For any other form, when ->alter(['form', 'form_other_form'], ..) is called, MyClass::formAlter() should be in its regular order position, between other implementations.
• For 'node_form', when ->alter(['form', 'form_node_form'], ..) is called, MyClass::formAlter() should be first, before other implementations.

Currently, this works as described _if_ there is at least one implementation of hook_form_node_form_alter().
This implementation could be from a different module that we don't care about.
If there is no such implementation, we get the TypeError as reported in this issue.

So the main difference is this:

• Currently, a #[ReorderHook] targets an implementation (class + method) whenever it is invoked with the specified hook as part of the hooks/types passed to the ->invokeAll() or ->alter() call.
• Instead, nicxvan suggests that a #[ReorderHook] should target an implementation (class + method) only if it is registered with the specified hook.
  This means the scenario in #17 would no longer be supported.

The distinction between "invoked with" and "registered with" is relevant only in multi-type alter calls like ->alter('form', 'form_myform').
An implementation is (typically) registered for only one hook, but the alter call can add more hooks into the mix.

The proposed change would a BC break.
The time to suggest this would have beeen pre 11.2.0 :)

What I would propose is to only fix the error case, and make it align with the non-error case.

Haven't fully read 17, but this only happens for alter calls, this does not occur as far as I tested for non alter hooks.

The "steps to reproduce" show how this applies to non-alter hooks.

Simply add this attribute (and nothing else) on a class in a Hook namespace:

use Drupal\Core\Hook\Attribute\ReorderHook;
use Drupal\Core\Hook\Order\Order;

#[ReorderHook('non_implemented_hook', 'NonExistingClass', 'nonExistingMethod', Order::Last)]

(don't forget the imports, or it won't work)

It also does not matter if the target class and method exist:
- If the hook has (other) implementations, but the target class and method do not exist, there is no error.
- If the hook has no implementations, but the target class and method do exist, just registered for a different hook (or no hook), we do get the TypeError during discovery.

This is really about targeting a hook that (currently) has no implementations.
If you target a hook that does have implementations, but the targeted implementation does not (currently) exist, there is no problem.

Also it is not "fatal" per se: A TypeError is catchable, it is up to Drupal whether to catch it.

I don't think that is quite right, ReorderHook can only target one implementation, if that implementation doesn't exist then it's rules should not apply at all.

Specifically:

so you target a form_alter impl in the scope of a form_form_id_alter

but if the form_form_id_alter doesn't exist then no ordering should happen, if the module want's to also order the form_alter they should target that separately.

The "if the form_form_id_alter doesn't exist" does not mean anything.
A hook may never be called or it may have no implementations.
There is no such a thing for a hook as to "not exist".

During discovery, we can only determine whether a hook has implementations or not.
But it may still be called.

#[ReorderHook('form_myform_alter', self::class, 'formAlter2', Order::First)]
class MyHooks {
  #[Hook('form_alter')]
  public function formAlter1(array &$form): void {
    $form['#alter_calls'][] = __METHOD__;
  }
  #[Hook('form_alter')]
  public function formAlter2(array &$form): void {
    $form['#alter_calls'][] = __METHOD__;
  }
}

class MyTest extends KernelTestBase {
  public function testFormAlterReorder(): void {
    $this->assertFormAlterCallOrder([
      MyHooks::class . '::formAlter1',
      MyHooks::class . '::formAlter2',
    ], 'form');
    $this->assertFormAlterCallOrder([
      MyHooks::class . '::formAlter2',
      // The #[ReOrderHook] rule is applied if 'form_myform' is within the alter types.
      MyHooks::class . '::formAlter1',
    ], ['form', 'form_myform']);
  }
}

Currently this will cause the TypeError for which this issue exists.
But we can easily craft a version of this scenario that does not crash: We simply add an implementation for hook_form_myform_alter().

#[ReorderHook('form_myform_alter', self::class, 'formAlter2', Order::First)]
class MyHooks {
  #[Hook('form_alter')]
  public function formAlter1(array &$form): void {
    $form['#alter_calls'][] = __METHOD__;
  }
  #[Hook('form_alter')]
  public function formAlter2(array &$form): void {
    $form['#alter_calls'][] = __METHOD__;
  }
  #[Hook('form_myform_alter')]
  public function formMyformAlter(array &$form): void {
    $form['#alter_calls'][] = __METHOD__;
  }
}

class MyTest extends KernelTestBase {
  public function testFormAlterReorder(): void {
    $this->assertFormAlterCallOrder([
      MyHooks::class . '::formAlter1',
      MyHooks::class . '::formAlter2',
    ], 'form');
    $this->assertFormAlterCallOrder([
      MyHooks::class . '::formAlter2',
      MyHooks::class . '::formMyformAlter',
      // The #[ReOrderHook] rule is applied if 'form_myform' is within the alter types.
      MyHooks::class . '::formAlter1',
    ], ['form', 'form_myform']);
  }
}

Imo we should merge the tests-only branch "3533944-alter-non-unique-current-behavior" before we do anything else.
https://git.drupalcode.org/project/drupal/-/merge_requests/12754/diffs
Maybe this needs a separate issue?

Alternatively if we change it back we fix the behavior of 11.2.

We could still do that as a follow-up, if we really want to.
I think for now the least disruptive solution is to preserve the 11.2.x behavior while removing the notice.

Btw currently we use FormattableMarkup to produce that notice, and as a result we get ugly escaped html in the placeholder variables.
Any other trigger_error() call in core uses raw string operations.

I would advocate for array_unique but that is forest impression without deep consideration.

Btw I notice that it matters where we call array_unique().
If we call array_unique() directly in ->alter(), it will change how the #[ReOrderHook] order operations are applied.
If instead we call array_unique() in ->getCombinedListeners() to get the listener lists, but we use the original (non-unique) hook names for order operations, we can keep the current behavior.

However this distinction is quite theoretical.
Most of the relevant cases are with form alter, and here the pattern is only ever ['form', 'form_x', 'form_x'], so ['a', 'b', 'b'].
To reproduce the order operation glitch, we need a pattern like ['a', 'b', 'a'].

I found this new issue while working on this one:
🐛 #[ReOrderHook] crashes on non-implemented hook. Active

donquixote → created an issue.

I created multiple MRs to demonstrate how the changes compare to current behavior.

Related discussions happening in 🐛 Order of alter hooks is not always respected by ModuleHandler Needs work .

In addition to array_unique() we could deprecate passing a non-unique list, so that it will _also_ be fixed in all calling code.

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.