🇩🇪Germany @donquixote

An alternative would be to put #[TrustedCallback] on the class itself. Which means we would have to change the #[Attribute] attribute on that attribute class.
But I think it is simpler and less "special" to just have the attribute on the __invoke() method.

@mdsohaib4242
Actually this does not do anything, because at the time you add your own '#process' callback, the '#process' array is still empty.
The would-be-replaced callbacks are added later by the form builder which is after hook_form_alter() based on $element['#type'].

@donquixote which MR? The newer one in draft?

Well, I am proposing to use the newer MR which is in draft.
Imo the old MR is a dead end, because it breaks existing behavior and makes it impossible to fully remove/replace the callbacks from #type.

But I don't want to remove the old one yet, unless everybody agrees on the new direction.
Having both means we can discuss which direction is preferable.

I notice that '#pre_render' does not allow object with __invoke() method, if it is not a closure.

See 📌 Support object with __invoke() for #pre_render. Active

(We can eventually handle #pre_render in a separate issue, but for now I find it interesting to include it here as proof-of-concept, to see the bigger picture of where this is going.)

donquixote → created an issue.

If multiple operations (e.g. form alter hooks) want to add '#process' callbacks, while preserving the element type default callbacks, they need to make sure that the element defaults are not executed more than once.

The following pattern works in most cases:

          $element['#process'] ??= [new ElementTypeProcessCallback($this->elementInfoManager)];
          $element['#process'][] = 'my_process_callback';

Can't Element::PROCESS_CALLBACK_PLACEHOLDER be a method that just does this:

Yes, this would also do the trick.
However, it does require a call to \Drupal::service(), if it is to be a constant.
Or alternatively, we could make this a class with __invoke() and DependencySerializationTrait. Because these things tend to get serialized. Actually not so bad!

Actually one more problem with this approach.
If multiple operations (e.g. form alter hooks) want to add '#process' callbacks, while preserving the element type default callbacks, they need to make sure that the element defaults are not executed more than once.

If the callback is a constant, we can easily check for in_array().
But if it is an object?
Actually, in_array(*, *, FALSE) applies weak object comparison, and this actually seems to work in this case.
But I am a bit afraid of weak object comparisons, I don't trust them.

Perhaps we want to move the ElementProcessCallback* classes to `Drupal\Core\Form\` namespace?
For now I had it in `Drupal\Core\Form\Render\Callback`, because form element type plugins are typically in the same namespace as render element type plugins.
I ma not changing it now because I want to see more feedback first.

We can't use readonly in combination with DependencySerializationTrait.

I am setting to "Needs review" for one round of feedback, but I am sure we will have to change some parts.

Here we go, some unit tests.
Let's discuss if this is a good direction, and how we can refine it.

I created a new MR with these placeholder objects following the idea by @joachim.

I want to add tests, but not sure yet where to put them.

call ELEMENTCLASS::getInfo()

Just to clarify, we would not call it like this, because it is not a static method.
Instead, we would call `$info = $this->elementInfoManager->getInfo($element['#type'])` as we already do in FormBuilder and Renderer.
I am working on something, just need to add tests..

@joachim

Can't Element::PROCESS_CALLBACK_PLACEHOLDER be a method that just does this:

1. look at #type in the render array it's working on
2. get the Element plugin class for that #type
3. call ELEMENTCLASS::getInfo()
4. get the callback names from the returned info array
5. call them

Yes, this would also do the trick.
However, it does require a call to \Drupal::service(), if it is to be a constant.
Or alternatively, we could make this a class with __invoke() and DependencySerializationTrait. Because these things tend to get serialized. Actually not so bad!

The bigger issue is that what you really want is a response containing a predictable interface.

So maybe a name of Drupal::serviceByInterface() would be better?

Indeed Drupal::serviceByInterface() would be a more accurate.

Basically you use the type that you would also use as a parameter type in a constructor that expects that service.
So it is the equivalent to autowire.

The reason I named it serviceByClass() in the MR is that:

• Some services don't have an interface. Especially in custom code or in early versions of a module this might be the case. If constructor parameter types use the class name, then any decorators also need to extend that.
• The term 'class' is used as a catch-all term in many places, where traits and interfaces are also included. E.g. "ReflectionClass" (php native), "class-string" (phpstan), "ClassLoader" (composer), etc.

In my practice, I definitely find myself wanting something like this but also my IDE knows wha to expect once I put in an assert() on the next line.

I tend to do either assert(* instanceof *) or an inline /** @var */ doc.
The problem with both of these is that they are more verbose, and they require a local variable, where otherwise I could use method chaining like Drupal::service(Abc::class)->foo();.

This has the benefit, also, of validating that I am getting the service I expect during tests and local development.

I use assert() in particular to check the order of services when I have multiple layers of decoration

Ok there are two different goals / cases here:

• In a test, I may want to assert the exact class of the service, to make sure my decorators work correctly. In that case I would use $this->assertSame(MyClass::class, get_class($service)); rather than native assert(* instanceof *) or PhpUnit ->assertInstanceOf().
• In my regular code, I would only ever assert($service instanceof SomethingInterface), because other modules might want to add further decorators. This verifies that the DI is not completely giving me the wrong service, but it does not verify that specific decorators are applied.

Changing issue title to be more findable, hopefully

It seems for all the callback types #process, #after_build, #pre_render and #post_render, the placeholder can be a simple identify function.
fn ($x) => $x
(but as regular named function or static method, not as closure)

Also, with this proposed change, it is no longer possible to fully replace or remove the callbacks from '#type'.

A solution could be to introduce a placeholder callback, so that we can:

$element = [
  '#type' => 'radios',
  '#process' => [
    'my_custom_process_callback',
    Element::PROCESS_CALLBACK_PLACEHOLDER,
    'my_custom_late_process_callback',
  ],
];

and we get this:

$element = [
  '#type' => 'radios',
  '#process' => [
    'my_custom_process_callback',
    [Radios::class, 'processRadios'],
    'my_custom_late_process_callback',
  ],
];

The placeholder should be a valid callback that, if not replaced, just returns the first parameter.
At the same time, it should be a constant that is easy to compare against or look up in an array.

This leaves some design choices to make:
- What exactly is the placeholder value? If it is a static method, how do we call the class and method?
- Where do we do the placeholder replacement? I was thinking of a new service ElementTypePopulator, which handles all of the '#type' replacement, so that we don't have to clutter up Renderer and FormBuilder.

(I did post some proof of concept code in a slack convo, but I am still too undecided about the class naming.)

I removed imports reordering in those files where no new imports are added.

I'd say this issue needs an explanation why the problem occurs with state, but not with keyvalue.
From the comments here I understand it is a race condition, and from looking at the code, I assume it is caused by the cache layer in state, because otherwise state is just a wrapper around keyvalue.

as suggested in the Slack thread

So, this should be in the issue summary..

I do find this issue from April 2024, which might be related:
🐛 [random test failures] Race condition in state when individual keys are set with an empty cache Fixed

Some people were confused by the title..

Not sure what to make of the test failure.
Let's get one round of feedback first.

Seems like most issues are for 8.x-3.x.

donquixote → created an issue.

Let's see what others think.

It can be a good idea to replace complex inline expressions with variables, but I don't think this is such a case.
I suspect it is mostly historic reasons, to keep the #2619482: Convert all get_called_class()/get_class() to static:: → footprint small.

donquixote → changed the visibility of the branch 3495495-inline-class- to hidden.

donquixote → created an issue.

This use of randomString() looks suspicious.

    $this->drupalGet(\Drupal::service('file_url_generator')->generateAbsoluteString($directory . '/' . $this->randomString()));

For those tests with real random-generated values, it would help if these values were reported, so that hopefully it can be reproduced with exactly these values.
One idea could be to pass random values from a data provider, but perhaps there are easier solutions.

Exactly this. That was done on purpose, as we otherwise have two classes with the same name that we have to alias.

I removed the questionable commit.

In case of name clash of the class alias (e.g. attribute vs annotation) we can use other shortcuts like `Annotation\Something`, where we might import the respective Annotation namespace.

Too bad...

Namespaced classes/interfaces/traits should be referenced with use statements

Alternative is to use an alias.

An example where the namespace import is used is Drush commands, where we have `use Drush\Attributes as CLI;` and then `#[CLI\Command(...)]`.

donquixote → created an issue.

donquixote → created an issue.

Seems we are suffering this one, 📌 [random test failure] BlockCacheTest Active
And also another random test failure which happened earlier.

@mile23

Searching through Drupal 11, I see that: A) LoggerChannelTrait is unused within core, and B) therefore no services follow this pattern.

I do find it used in the following places in core:

core/lib/Drupal/Core/Controller/ControllerBase.php
core/lib/Drupal/Core/Form/FormBase.php
core/modules/layout_builder/src/Plugin/Derivative/FieldBlockDeriver.php
core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php
core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
core/modules/views/src/ViewExecutable.php
core/modules/language/src/LanguageNegotiator.php

At least LanguageNegotiator is a service.
But the bigger usage is through base classes ControllerBase and FormBase, which also have other Drupal::*() calls.

I wonder if we should stop using these base classes with their hidden magic.

Patches for people to apply in their composer.json.

Actually right now it is 4.x, but perhaps 3.x has the same problem.

donquixote → created an issue.

The MR is a proof of concept for now.
I am not going to attempt to make anything pass.

I don't know if all the `@template-extends` and `@template-implements` is really needed.
Also not sure about all the `@var` docs, if that is really the best way.

donquixote → created an issue.

We want to avoid that a service gets instantiated only to reset its internal cache property.

Would that not apply only to static properties being used as an internal cache? Most of these internal caches I've seen use regular properties, meaning the service is instantiated either way.

E.g. in drupal_flush_all_caches() we call `\Drupal::theme()->resetActiveTheme();`, which does get an instance of ThemeManager, only to set `$this->activeTheme = NULL;`.
But if at the time this is called, ThemeManager had not been instantiated yet, `$this->activeTheme` would already be NULL / uninitialized.

The same would happen if we use regular event subscription to call a ->reset() method to reset an object property.

I think most of the solutions proposed here avoid this problem in one or another way.
Which is good :)

However, I would argue we could have a "default" cache for memory like we do for the DB and encourage everyone to use that cache for their simple property caching. Then there's no overhead whatsoever because there will always be something using said memory cache and therefore the service will already be instantiated.

There is overhead in reading and writing these cache items, which is always going to be more php operations than with a property cache.
All of it is just basic php operations. So it will only become relevant for caches where we read or write many small values.

If we were using the Symfony core, we would already have such a mechanism.

We want to avoid that a service gets instantiated only to reset its internal cache property.
I did a little experiment and it seems this works.

The ServicesResetter does skip lazy services.
The ResettableServicePass adds IGNORE_ON_UNINITIALIZED_REFERENCE to the service references which it adds to the IteratorArgument.
This seems to do the magic.

What I don't see is how we could use this to reset different services for different events.
It's all or nothing.

All classes and all of their methods (including private methods but excluding constructors) must be documented.

This line is far too generic.
It means we can either omit the full phpdoc on a constructor, or we have to put a complete phpdoc.

Maybe there are some parameters that we do want to document, but others where it would be redundant.
In that case we might want to omit the parameter description on some parameters but not all.

This really is about removing redundant documentation and condensing the doc section afaict.

As mentioned before, it would be useful to have a proof of concept MR that shows how this would look like in core.

@drunken monkey

On the contrary, it seems like a more descriptive parameter name could make the description less necessary:

Arguably, your example could also be read in the opposite way:
The reason why you name your parameter as "$reviewer" instead of "$account" is also the reason why you want to have a doc description.
Yes you could game the system and choose a worse parameter name to be allowed to avoid the parameter description.

It's a causation vs correlation thing.
E.g. an individual will be better off if they take medicine. But statistically, people who don't take medicine are healthier.

Personally I am not really invested in the heuristic.
I see the biggest possible benefit in reducing redundant service constructor parameter descriptions.
For other methods I don't care, I think I would rather keep the parameter descriptions.

For $container->getByClass() this is going to be more difficult, if we want to maintain full BC.

I was going to introduce a new interface ServiceByClassInterface and ConvenientContainerInterface, which would be implemented by our container classes.
But then in every place we need to check if the container we have is actually implementing that interface.
If instead we add the method directly to our ContainerInterface, any custom implementations will break which don't have this method.

----

Alternatively we could introduce a function like this:

$time = service(TimeInterface::class, $container);
assert($time instanceof TimeInterface);

so the same as above but we pass the $container object, so it is cleaner.

or

$time = ServiceFromClass::create($container)->getByClass(TimeInterface::class);
assert($time instanceof TimeInterface);

or we have something that returns a closure.

$time = resolver($container)(TimeInterface::class);
assert($time instanceof TimeInterface);

$resolve = resolver($container);
assert($resolve(TimeInterface::class) instanceof TimeInterface);

But currently a lot of the $container->get() calls should rather be replaced by some kind of autowire solution.

I would like to add tests, but not sure where they would go.

For PhpStorm you can generate meta information about services.

Thanks!
In the past I tried the symfony plugin for phpstorm which was also supposed to do this.
But it slowed everything down.

I think what I am proposing here is nicer, and more universally available.

donquixote → created an issue.

donquixote → created an issue.

I suspect none of the fails (phpstan, phpunit, phpcs) are caused by this MR.

I have a simpler approach.
I did not see how to rename the MR, and also I don't want to destroy others' work.
So I created a new branch with new MR.

Note that this issue only covers a part of the problem.
There are still other scenarios when JwtTranscoder is not fully initialized, and we are sending NULL values or accessing uninitialized property:

- The JwtTranscoder->algorithm should also be initialized with NULL, because it can be accessed before it is initialized.
- In JwtTranscoder->encode(), we need to check if $key is NULL.

For now I consider these problems as out of scope for this issue.

Rebase and code cleanup.
I still have some concerns with the branch, which I will address later.

I am attaching a patch for people to apply in their composer.json.

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

What about something like this:

Each parameter must be documented with a @param tag with a fully-qualified type and a description, except:

• A parameter description can be omitted, if:
  • The parameter type is a single class or interface name, AND
  • The parameter name is derived directly from the type name, AND
  • The description would not add any new information.
    (This last condition won't be checked by any code quality tools, and is up to the developer to decide.)
• If none of the parameter have a description, and the types in the @param tags only replicate the parameter type in the php function signature, then all @param tags on that function can be omitted.

I am sure this can be further improved:
- We need to find a better wording for "native php parameter type".
- The "derived from" could be more precise. The current "take the ..." looks a bit out of place though.
- For the second part, I wonder if we allow a function that has a `@return` doc but no param docs.

Also it would be interesting to see a proof-of-concept patch/PR with param docs removed from core methods and functions, to see how this would look like, and to verify that we are really all on the same page.

The proposed text still needs to be more clear:
- What exactly can be omitted: The param tag, or just the param description? What about the type?
- If all parameter descriptions are optional, and all parameters have a native type declaration, we can drop all the param tags in phpdoc? what if we still have a return doc?
- Where in the doc page does it say that a param doc needs a description? The part being changed does not mention the description at all.
- Does this also allow to drop the entire doc comment, or is it out of scope?

Sorry, I don't think this is rtbc with the wording currently proposed.

If the result is the same as the type then the doxygen is optional (see other exceptions below).

I don't think the term "doxygen" should be part of the doc page.
And, it is not clear what "doxygen is optional" means: Is the parameter description optional, or the parameter type, or the entire `@param` tag, or the entire doc comment?
The text should explicitly say this, without using the term "doxygen".

catch → credited donquixote → .

@krystalcode (#103):

So, what is the purpose of sorting use statements?

For me, the main purpose of ordering anything, including imports, is to avoid noise and conflicts in git, and duplicated entries resulting from faulty merges. These problems can be avoided if there is one canonical order that is followed by all developers and tools.

I don't like fast-forward merge.

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

donquixote → created an issue.

I think I was wrong in the other issue.

Actually, I think I made noise for nothing.

If I understand correctly now, procedural implementations are added as listeners with prio like -1, -2, -3 etc.
So you can always set a prio of "ridiculously negative" like -9999, to make sure it will run last, after the procedural hooks.
So, apologies for the distraction.

@catch

I think the problem with legacy hooks first, is that all of the existing modules that use hook_module_implements_alter() to run last, are currently using procedural hooks.

Actually I don't advertise for legacy hooks first.
Just for the possibility to have some new hooks run last.
So maybe we should set the priority of legacy hooks to -1, not 0, if that achieves the goal.
Then you can set yours to -2 or lower to have it run after the legacy hooks.

An alternative could be to have a dedicated event listener for the procedural hooks, with priority 0.
This way, OOP hooks can run before or after all procedural hooks.

With this, we still disrupt contrib modules that rely on hook_module_implements_alter() to run first.
But we no longer force modules to use procedural implementations if they want to run last.
The former group of modules / use cases will be smaller.

Actually..
I have concerns about the order of legacy hooks.
(nothing new, but I changed my mind about whether we should care)

The order imposed by this change puts all OOP implementations first, and legacy/procedural implementations second.

This means:
If you want your implementation to run last, after all other implementations from contrib, you need to make it procedural.
As a contrib author, to be maximally compatible, you still need to follow this advice even if a lot of contrib has converted to OOP.

An alternative could be to have a dedicated event listener for the procedural hooks, with priority 0.
This way, OOP hooks can run before or after all procedural hooks.

Why should we care?
- Cost of upgrading major versions can cause projects to jump ship.
- I have seen more cases where order of hook implementations was important.
- A lot of contrib and custom modules will keep their procedural hooks for a long time.

(My other solution attempted to address this problem, but was arguably overengineered.)

Reverting the change means that the original set of people get the problem again. Which is an option, for sure, but it's not making the problem go away.

I would propose a more flexible solution:
- We choose one default sort order that will also be used for Drupal core. This should follow what existing tools already support, and what is common in existing 3rd party code.
- Contrib modules or website projects are allowed to use their own order. We can reference documentation how to configure this with Coder and phpcs.xml.

In the issue summary I don't see any argument why the proposed order (case sensitive) would be preferable to something else.
I also don't see a statistical analysis for existing code in and outside of Drupal.
(might be in the comments)
I only see the argument that having a convention is better than having no convention.

As for existing code, the following two regex searches can be used to search in a vendor directory:
(I did the search in PhpStorm with case sensitivity and multiline enabled)

use ((\w+\\)+\w+)[A-Z]\w+.*;
use \1[a-z]\w+.*;

use ((\w+\\)+\w+)[a-z]\w+.*;
use \1[A-Z]\w+.*;

The "\1" references a previously captured substring.

For all the code I found, it seemed that case insensitive sorting was used.
But even in a typical Drupal project these were just a few matches overall.

Drupal is an outlier because module names in namespaces are lowercase.
(I remember back in the days I argued for this, so that we see the literal module name in the namespace, not a captitalized form. I still think this was the right choice.)

catch → credited donquixote → .

so actually the patch in #2 is the way to go :)

On the other hand it is questionable why any logged-in user should see this message which is quite technical.

Actually a relevant part of the problem is that user.module is not loaded when adaptivetheme_preprocess_maintenance_page() is called.
So we could fix it with module_load_include('module', 'user').

Also, I expect that any software developer knows what case-sensitive ordering is so we don't need the last line. Correct me if I am wrong.

I would disagree.
A case-sensitive comparison is clear.
But a case-sensitive order, not so much. It could be derived from a character set (e.g. ASCII) where letters have numeric representations, but this is by no means clear. It could also be based on whatever specific php sort functions do.
E.g. for regular letters it could be "A, B, C, ..., a, b, c, ..." or it could be "A, a, B, b, C, c, ...".
For special letters like 'ä' it is less clear. Although I think we don't allow them in namespaces or module names, or people just avoid them and stick to a...z.

slevomat uses strcasecmp() and strcmp() depending on the setting.
This will result in "A, B, C, ..., a, b, c..." for regular chars.

Here are some experiments, https://3v4l.org/iUKZh

With my current version of PhpStorm, the auto sort with Ctrl+Alt+O (Optimize imports) puts the imports in the "wrong" order according to the new standard.
But I also see cases of the wrong order in Drupal core (maybe because I am not in latest version).

It seems the following happened:
- The 3.x branch was merged into the issue MR branch.
- The 3.x branch was rebased or reset to the tip of the issue MR branch.

Actually more likely it was just merged with fast-forward.

Hi
Thanks for merging,
but I think it would be better to reset (and force-push) to an older version to get rid of the faulty commits that caused this.
Let's talk on slack.

The commits from this MR made it into the 3.x branch in a bad way, bypassing the pipeline tests.
It seems the following happened:
- The 3.x branch was merged into the issue MR branch.
- The 3.x branch was rebased or reset to the tip of the issue MR branch.
- More minor changes were pushed to the MR branch.
- The minor changes were then squash-merged into the 3.x branch, resulting in https://git.drupalcode.org/project/l10n_client/-/commit/ff75bc814e058add...

Since this we have failing tests.

I think the correct way is to hard-reset the 3.x branch to before this issue, which is 3.0.0-alpha3.
I don't remember if drupalcode.org allows us to hard reset a branch (that is, force push).

Another import went missing in the same commit, for PoItem.
Adding it in the MR.

I could add tests, but this functionality is still quite fluid, so we should avoid unnecessary effort.

Hello.
I was able to reproduce with latest version of this module both with Drupal 10 and Drupal 11.

For now the way to resolve it is to manually remove the field before uninstall.
Fingers crossed :)

We need to think about what would be the optimal solution to implement in this module.

One thing to consider is that removing the field will cause data loss, because all the tokens per user are lost.
This is why there is some merit in letting the user manually delete the field.

On the other hand, this deletion also would need to happen in a deployment. So it would need a custom update hook in the project to delete the field, which would be a burden on the site builder.

donquixote → created an issue.

Personally, I’ve been using “Constructs a new class instance.” as the first line, which still follows the “start with third-person verb” rule

even though it’s a bit longer, it’s in line with the almost universal format for method doc comments and once you read “Constructs” you already know what’s going on.

For me, magic methods and especially the constructor are special because you (typically) don't call them directly.
(For the constructor, an exception would be calling the parent class constructor.)

The "start with third-person verb" gives you a promise what you can expect by calling the method.
It would be justifiable if we decide to not do this on some or all magic methods.

donquixote → created an issue.

I would not put too much emphasis on the performance argument.
The medium article from #20 looks interesting, but I have not looked into it enough to assess it.
Certainly it's good to understand what we are optimizing for.

Currently we live in a world where most functions are in the global namespace.
This applies to native php functions and to functions from Drupal modules.
Many packages don't want to add functions, they only want to add classes.
(there have been interesting discussions in the php internals list about static methods vs functions)

If more packages and modules embrace and provide namespaced functions, the potential for ambiguity becomes bigger.

One thing here is technical ambiguity, where actually the wrong function is called.
The other thing is confusion when reading code, because you don't immediately see if a function is in the global namespace or not.

I was long convinced that backslash is the way to go, but maybe we should also consider imports, and check how projects outside of Drupal do it.

had no idea this was added.

Yeah this is quite new and not part of any stable release yet if I saw correctly.
I only found it because I really wanted to have machine names.

Perhaps core should provide better detectability of this change, before anything is committed to ds or field_group.
Until then, people can use this patch if it is useful to them.

For now I am using the class 'machine-name' for feature detection.
Ideally, core should provide a better way for contrib modules to detect if the machine name column is present.

donquixote → created an issue.

This patch is a first proof of concept, definitely not to be committed as-is.
It lacks the conditionality, and it lacks tests.

It is ok for people who want to patch both core and field_group to get the machine name column.