🇩🇪Germany @donquixote

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.

The change seems simple, but the conditionality is more tricky.

We should have some kind of feature detection support in core.

It could also be nice to have some kind of feature detection possibility, so that a module like field_group can support different versions.
Currently it could check if any field row has a cell key 'machine_name', but this is not 100% reliable if there are no fields for some reason.

donquixote → created an issue.

I like this change. Thanks everybody!

Just some notes:

• The columns gets even more tight when I click "Show row weights".
  I like to do this to manually set row weights to reduce noise in a git commit.
  But it is worth it imo.
• This needs work in field_group and possibly other contrib modules. Currently it looks broken with field_group enabled.
  Again, that's ok with me.
  But it deserves a note in the "API changes".

From a usability perspective:
In all the projects I ever worked on, the site builder who manages displays with field_ui is the same person who will then run "drush config:export" and commit the changes in git.
There is no point in hiding machine names from this person.

donquixote → created an issue.

I just noticed a workaround:

• When you save the site config that has the urls for front, 403 and 404, even if you specify an alias, it stores the `/node/123` url.
• However, if you then manually edit the config files, put the alias instead of `node/123`, and import the config, that actually works.

And btw, if I run "migrate:import --update" with a node or term migration with this patch, it does work, and it does update the entity uuid to the correct value.

We probably do need something else for revision uuids.
But in a first version we can live without them.
Term uuids and some content uuids are important if they are referenced from elsewhere, esp in config.
Revision uuids are quite unlikely to be referenced in config. The main reason to support them would be a sense of technical completeness.

Here is a patch that adds to the #7 patch.
This only covers the first part of the problem, exposing uuid for nodes.

Actually it might be a good idea to treat this as two separate issues:
- Add uuid for node source plugin, if uuid module was installed in D7.
- Follow-up to add uuid mapping to the default migrations.

I like patch #7 as an incremental improvement.
For it to work correctly, we also need to add 'uuid' in the ->fields() method, with the same condition.

As for adding the "uuid: uuid" mapping to migrations:
One option is to conditionally add this mapping. So it would not be present in the default yml files.

If we always want to add the "uuid: uuid" mapping, then we need to auto-fill this if the uuid module was not installed in D7.

The usual random uuid is not great, it will produce a different value each time the migration is run.
This causes issues e.g. with views that reference taxonomy terms by uuid.

I can see two options to generate "stable" uuids, if uuid module was not enabled in D7:
- Store the generated uuids in a yml file somewhere, which would be committed to code. I don't like this, because this file would have to scale with the amount of content.
- Generate fake uuids that are actually hashes based on entity type, entity id, and a fixed salt defined somewhere. E.g. create a sha256, get a substring, insert the dashes for uuid format, done.

Maybe all of this would have to be opt-in, to not mess with exiting migrations.

And yes, test coverage.

donquixote → created an issue.

I added more test classes..

benjifisher → credited donquixote → .

In my local I get these failures with the FAIL branch:

Time: 00:37.938, Memory: 4.00 MB

There were 4 failures:

1) Drupal\Tests\symfony_mailer\Functional\LegacyEmailTest::testSendLegacyEmail
Failed asserting that null is not null.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:56
/var/www/html/tests/modules/symfony_mailer_test/src/MailerTestTrait.php:46
/var/www/html/tests/src/Functional/LegacyEmailTest.php:43
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:729

2) Drupal\Tests\symfony_mailer\Functional\LegacyEmailTest::testSendLegacyEmailWithTheme
Failed asserting that null is not null.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:56
/var/www/html/tests/modules/symfony_mailer_test/src/MailerTestTrait.php:46
/var/www/html/tests/src/Functional/LegacyEmailTest.php:81
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:729

3) Drupal\Tests\symfony_mailer\Functional\OverrideTest::testUpdate
Failed asserting that null is not null.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:56
/var/www/html/tests/modules/symfony_mailer_test/src/MailerTestTrait.php:46
/var/www/html/tests/src/Functional/OverrideTest.php:127
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:729

4) Drupal\Tests\symfony_mailer\Functional\TestEmailTest::testTest
Failed asserting that null is not null.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:56
/var/www/html/tests/modules/symfony_mailer_test/src/MailerTestTrait.php:46
/var/www/html/tests/src/Functional/TestEmailTest.php:27
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:729

FAILURES!
Tests: 26, Assertions: 274, Failures: 4.

With the 1.x branch I get 1 failure:

Time: 00:47.724, Memory: 4.00 MB

There was 1 failure:

1) Drupal\Tests\symfony_mailer\Functional\OverrideTest::testUpdate
Failed asserting that null is not null.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:56
/var/www/html/tests/modules/symfony_mailer_test/src/MailerTestTrait.php:46
/var/www/html/tests/src/Functional/OverrideTest.php:127
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:729

FAILURES!
Tests: 26, Assertions: 312, Failures: 1.

With the fix branch, all tests pass.

This is interesting.
The "FAIL" merge request does not actually fail the phpunit test.
But in my local env (with ddev) I do get test failures, even without the usleep().

One thing I suspect is that usleep()/sleep() does not really work in the pipeline.

Also, the current solution with using state assumes that all emails are sent during a single request.
If there are multiple requests that happen before the test can collect the emails, e.g. as a batch process, and each of them is sending emails, then there will be a problem in the __construct() which expects the state collected mails to be empty.

A database table with one row per mail and auto increment would solve this.
There can be a counter in the test trait that contains the last read auto increment id.
In the ::setUp() method the table can be truncated, and the auto increment reset, OR the internal counter variable/property can be set to the current auto increment counter from the db.

We also want this to work with ExistingSite tests, where we cannot rely on the module being freshly installed each time.

Actually...
this is not a ::__destruct() method, but a ::destruct(), which is called from $kernel->terminate().

We see this in index.php:

$response->send();

$kernel->terminate($request, $response);

So this explains the order :)

donquixote → created an issue.

donquixote → created an issue.

Hello,
it makes sense to remove compiler passes that are purely for optimization, and that don't change behavior.

However, the RemoveUnusedDefinitionsPass is not designed purely for optimization.

In a symfony project, the following use case is typical:
- Scan a directory and register every class as a private service. This will include classes that absolutely cannot be used as services.
- Process all the services with autowire, autoconfigure etc. This will also populate the error list for broken services.
- Remove unused private services. This will usually get rid of the broken services that were added earlier.

This means RemoveUnusedDefinitionsPass is essential if we have any mechanism that adds random classes as private services.
If we don't have this mechanism, then we never need it: Not during the installer, but also not really later.

I mention all this because I am currently experimenting with service discovery.

We simply need to drop the second commit :)

Or we could keep the test case from the second commit, but change the expected behavior.

Mostly accept the PR, except lets just stop at isInstantiable, and just ignore abstracts. (tests and fixtures need updates as a result)

We simply need to drop the second commit :)

But I want to be clear: Currently the abstract class create() does work.
So if we don't have the second commit, we lose functionality, and theoretically break BC.

The example in the test is quite obscure. Perhaps somebody has a more valid use case.
Of course such cases would be easy to fix.

On the other hand, I can also imagine cases where this behavior would be clearly unintentional.

I think it is good practice that a class with hook methods shall be either final or abstract, to clarify intent.
Otherwise, if a hook method is inherited from a non-abstract base class, it is not clear if the developer intended this to be registered as a hook or not.
But we can't really enforce this now.

The test cases cover some "hidden" features which already work today, but the support of which may not have been by design.

MR is for 1.7.x.

The change record is weird.

Docblocks for __construct() can be omitted, as is already documented in the PHP official documentation.
https://www.php.net/manual/en/language.oop5.magic.php.

What does the "PHP official documentation" have to do with doc comments?

donquixote → created an issue.

donquixote → created an issue.

Good catch, I was going to open the same issue!
Changing issue title and description, to hopefully make it more searchable, and distinguishable in issue lists.

As a follow-up we could add autowire and constructor property promotion.
For now the module still aims to be compatible with Drupal 9.1, I don't remember if we have autowire there.

donquixote → created an issue.

This might be true.
But I actually had an example where visiting /cron had a different effect than calling $cron->run() from the service.

Surely there is a module somewhere that is doing things wrong, e.g. not reset a cache variable etc.
I have not figured it out yet.
But in a functional test we want to be realistic.
So we want at least to keep the option to run cron as a separate request.

donquixote → created an issue.

Hello, this is nice to hear!

Unfortunately I don't use backdrop so I don't really have an incentive to maintain a module.
Unless you just want to add me for credit :)
In that case, my github username is "donquixote", like here.

Otherwise, good luck with the module.

I also created a coding standards issue regarding the line breaks.
#3458314: Convention or recommendation for line breaks in constructor parameter signature with promoted properties →

donquixote → created an issue.

omit the first line comment.

How do you want to handle constructors that need additional doc text as explanation?

E.g. Drupal\Component\Annotation\Plugin

class Plugin implements AnnotationInterface {
[..]
  /**
   * Constructs a Plugin object.
   *
   * Builds up the plugin definition and invokes the get() method for any
   * classed annotations that were used.
   */
  public function __construct($values) {

This would become

class Plugin implements AnnotationInterface {
[..]
  /**
   * Builds up the plugin definition and invokes the get() method for any
   * classed annotations that were used.
   */
  public function __construct($values) {

I used this regex to find examples:

/\*\*(
 *\* [^@].*|
 *\*){3}(
 *\* .*)*
 *\*/
 *public function __construct

In general, logic in constructors should be avoided.
But sometimes it can't, or it is already there and we can't remove it for BC reasons.

it states the obvious.

It is true.
But it also gives some visual orientation, if the actual method name is further below.
And it is easy to generate or just copy/paste.

Other projects just add extra annotations (including phpstan or psalm ones) for the parameters where it's relevant to add them.

Or they omit documentation, and then are super confusing to read.
(just to make the point that the grass is not always greener elsewhere)

As for the "extra annotations .. for the parameters", I assume you just mean regular old `@param` docs, just not for all parameters?
There are already issues talking about the option of an incomplete collection of param docs.

Imo we can still do the change proposed here and then further relax in future issues.

@catch (#13)

So I think the main thing here is whether we want to drop parameter documentation at the same time as we apply constructor promotion to old code so we can make all the changes at once, if so we probably need to postpone this one on a coding standards issue to do that, if not we could go ahead with the initial change to property promotion here and do the param docs separately.

To me this seems to me like an unnecessary scope creep and distraction.
The issue has already filled up with comments that are not related to constructor property promotion.

Imo it is good practice to do incremental changes in separate issues, which reduce the scope of discussion and decisions in each issue.

Regarding the existing MR:
I prefer very much to have these constructor signatures broken to multiple lines:

  public function __construct(
    protected RootPackageInterface $rootPackage,
    protected string $eventName,
  ) {

This keeps it more readable, and also makes future MRs easier to review that want to add `readonly`, or `#[Autowire(..)]` attribute, or that want to add or remove parameters.

Then for parameter attributes we can either keep them on the same line or on a separate line.

  public function __construct(
    protected RootPackageInterface $rootPackage,
    #[Autowire(..)]
    protected string $eventName,
  ) {

or

  public function __construct(
    protected RootPackageInterface $rootPackage,
    #[Autowire(..)] protected string $eventName,
  ) {

I created this follow-up: #3457897: Simplify first line in constructor doc comments to "Constructor." →

Some constructors won't have their doc comment removed, and they need to keep a first line comment.
For these, I would like to see a simpler first line comment.

What I propose is what I already do in my own code.
Maybe others have different ideas.

donquixote → created an issue.

I did wonder why Symfony didn't just globally enable it

I would imagine this is for BC reasons, same as here.
There are probably plenty of existing packages that would break if symfony would enable autowire and autoconfigure by default.

::fromFileContents() has @param tag for $file parameter only because it needs extra clarification for Psalm (non-empty-string). All other parameters in that class have no tags. Also note missing @return tags as the return type is clear from typehint.

Interesting example.
For the $contents parameter, the comment description should explain that, if it is provided, the actual file contents are ignored, but the $file is still passed to FileChangeDetector. Maybe in less words.
The ChangeDetector::changed() could have a doc description saying "TRUE, if the resource has changed since the detector was created.".
Also the classes could have doc descriptions.

In this example the code is actually relatively simple, so we can make sense of it without comments.
And tbh there is a risk that a poorly written description is more misleading than helpful. I am already unhappy with my proposed comment above.
Still, in general I tend towards having doc descriptions on most symbols over not having them.

I think it's ok to only document parameters that need clarification. That's quite typical case when parameter type is extended via psalm/phpstan.

It would look quite alien and unexpected in a Drupal codebase, to have param docs for 3 out of 5 parameters of a method.
Having the complete list of parameters gives a sense of symmetry and completeness.
Also it is easier for an IDE to generate the doc comment, and warn if it is incomplete.

But maybe it is just a question of familiarity.
Do you know of 3rd party code with incomplete parameter docs?
(finding such code is the first step, we might still decide that we don't like it)

Maybe we should create an MR with example doc removals.

It would be redundant if the method had a return type though wouldn't it?

The only native return type could be `*(): static`, which is less specific than `@return $this`.
E.g. `): static` would still be correct on a wither method that returns a clone. `@return $this` must not return a clone, it must return the object itself.

Btw, to see how a lack of documentation looks like, we only need to look at the symfony codebase.
I find this painful to read :)

(ignore the previous comment)

That's not , however , how real world code looks like. For example, in EntityFormInterface you can find:

The `setEntityTypeManager()` example is interesting.
The `@param` doc and its description are redundant, because we already have the native type declaration and the first line of the doc comment.
The `@return $this` is _not_ redundant.
The method doc description looks a bit redundant, but actually, I might want more information:
This method needs to be called before the class can be used.
In fact this method reveals a design flaw, where we don't have automatic dependency injection for this class, and instead there are various places where the method is called explicitly.

Each parameter of a function must be documented with a @param tag unless the type already documents it: take the name of a function parameter. Convert it to CamelCase. If the type is an interface then append Interface after. If the result is the same as the type then the doxygen is optional (see other exceptions below).

Again "doxygen is optional" can be confusing. What exactly do we omit? The description, or the entire `@param` tag, or the entire doc comment?

I think the "take the name of a function parameter..." logic is a placeholder for "The parameter is a service".
Usually, if it is not a service, then we should assume the parameter to play a specific role in the class, which deserves to be described.

Problem/Motivation

This is a followup to #3238192: Allow omitting @var for strictly typed class properties → . That has this code snippet:

  /**
    * Where one can order a soda.
    */
  protected Bar $baz;

That's not , however , how real world code looks like. For example, in EntityFormInterface you can find:

  /**
   * Sets the entity type manager for this form.
   *
   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
   *   The entity type manager.
   *
   * @return $this
   */
  public function setEntityTypeManager(EntityTypeManagerInterface $entity_type_manager);

The entire doxygen is completely superflous although the proposal would only drop @param. We should strive dropping it in its entirety by typing the return then dropping the return and then finally the text.

Or in Drupal\block_content\Routing\RouteSubscriber::childRoutes you can find

   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_type
   *   The entity type.

This is a waste, completely pointless and it is actually harder to read. Because

EntityTypeInterface $entity_type

already totally tells you everything there is.

Benefits

If we adopted this change, the Drupal Project would benefit by ...

Replace 99% of the current doxygen with typing the properties. Rejoice as the amount of boilerplate collapses to a fifth.

Three supporters required

1. @pfrenssen →
2. @nilsdestoop →
3. @claudiu.cristea →

Proposed changes

Drupal API documentation standards for functions →

Each parameter of a function must be documented with a @param tag (see exceptions below).

Each parameter of a function must be documented with a @param tag unless the type already documents it: take the name of a function parameter. Convert it to CamelCase. If the type is an interface then append Interface after. If the result is the same as the type then the doxygen is optional (see other exceptions below).

@var: Class property data types →

Typed class properties may omit the @var declaration. It is recommended to use typed properties whenever possible.

Typed class properties may omit the @var declaration. It is recommended to use typed properties whenever possible. It is also permissible to completely omit the doxygen if the name of the variable -- append the string Interface as needed -- equals the the type.

Remaining tasks

2. /li>
4. Review by the Coding Standards Committee
5. Coding Standards Committee takes action as required
6. Tagged with 'Needs documentation edits' if Core is not affected
7. Discussed by the Core Committer Committee, if it impacts Drupal Core
8. Documentation updates
   1. Edit all pages
   2. Publish change record
   3. Remove 'Needs documentation edits' tag
9. If applicable, create follow-up issues for PHPCS rules/sniffs changes

For a fuller explanation of these steps see the Coding Standards project page →

I would argue that "omit the doxygen" is unclear terminology.
There is no "doxygen" in php, and even outside php, I don't think "doxygen" means a specific part of the doc comment, but rather the mechanism to generate a documentation from doc comments.
For "omit the doxygen" it is not clear whether we omit the param description, the entire `@param` tag, or the entire doc comment.

So instead we should say "omit the param description" or "omit the param tag" or "omit the entire doc comment".

So do we need to add explicit language that doxygen can only be dropped when every parameter is self-documenting?

Absolutely.
Something like this:
- A function doc must have either _all_ `@param` tags or none of them.
- A param doc description can be omitted if the type is a single class or interface, and the parameter name is a derivative of the type name.
- A param doc tag is "trivial" if it only replicates the native parameter type.
- If all parameters are "trivial", then all of them can be omitted.
- If all parts of the doc comment are trivial, then the entire doc comment can be omitted.

I very much support this issue.
The fact that the base fields are enabled by default is a show-stopper for this module on existing websites.

The solution proposed here seems to work in initial tests.
I have not done in-depth testing yet.
Also, I am not sure about BC impact on sites that already use this module.

And this needs tests.

Wonder if this is ready for review?

I don't remember all the details, but at least setting it to review increases the chance of progress :)

The ->setPublic(TRUE) in ->setAlias() makes this quite difficult.

Actually it is possible by replacing the relevant aliases with a custom class 'ResilientAlias extends Alias', which refuses to be made public.
I will probably publish something which does this.
Anyway I still think the implementation from this issue is quite aggressive..

I just ran into this.
I am trying to use the automatic discovery feature from symfony, to discover services in a module and in some composer packages.
This feature relies on unused services and aliases being removed, which only works if they are private.

The ->setPublic(TRUE) in ->setAlias() makes this quite difficult.

Actually it could be useful to add the revision id or the revision date to the file name of a pdf.

I pushed some what I think are improvements - TBD.
See the individual commit messages.

I should also add tests, but that requires a bit more thinking and searching.

See also 🐛 Wrong usage of trim() in entity_print_entity_view_alter() Active .
If we solve that, it will conflict with the patches here..

donquixote → created an issue.

+   * @param int $revision_id
+   *   The entity revision id.

I think the type needs to be "int|null".
This is what I see a lot in core for parameters with NULL default value.

-  public function viewRedirectDebug($export_type, $entity_type, $entity_id) {
+  public function viewRedirectDebug($export_type, $entity_type, $entity_id, $revision_id = NULL) {
     return $this->redirect('entity_print.view.debug', [
       'export_type' => $export_type,
       'entity_type' => $entity_type,
       'entity_id' => $entity_id,
+      'revision_id' => $revision_id,
     ]);
   }

This seems weird:
The redirect route is only meant for legacy support, so we should not expect this to be called with a revision id.
On the other hand: If we support a revision_id parameter, then we need to conditionally change the route for the redirect to use the revision route.

-        '#access' => $access_manager->checkNamedRoute('entity_print.view', $route_params, NULL, TRUE),
+        '#access' => $access_manager->checkNamedRoute('entity_print.revision.view', $route_params, NULL, TRUE),

With the latest patch we are using access check for the revision page even for the regular non-revision link.
I think it would be cleaner to have two separate paths of control flow.
I am going to prepare an MR.

There is an older discussion in #1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines → where we also mention conditions with && and ||.

And for the record, I support:
- No line break before first condition.
- Operator && or || at the beginning of a line.
- Line breaks around the ") {".

if ($long_condition_a
  && $long_condition_b
  && ($long_condition_c1
    || $long_condition_c2
  )
) {
  return 'yes';

I do see the appeal of line break before the first condition, but somehow it feels wrong.. but this could be just a preference familiarity.

In the past I would have proposed something like below, but now I think it is awkward..

if (true
  && $long_condition_a
  && $long_condition_b
) {

@scott_euser
Could we come up with a testing scenario to confirm that this works correctly on a given website?

Sorry for changing the status!

I have not tested, but the change looks reasonable to me.
I assume/hope the render element will contain all the necessary cache metadata, even on empty result.

The non-lazy formatter uses addCacheableDependency($view) yet the addCacheableDependency() method bails as soon as it sees that the object does not implement CacheableDependencyInterface (which views does not).

Ouch. My mistake.
I thought I had seen this somewhere else, but in fact it was not the ViewExecutable that was added, but the View config entity itself.
I see the correct solution seems to be to add the `$render_array` which should contain the '#cache' data.

I think we should do housekeeping in the following situations:
- When the entity view or form display form is submitted.
- As a separate command (e.g. drush) or on a dedicated page to do this kind of clean-up.

I don't think we can rely only on specific events.
E.g. when we change the code of a custom module, no event will fire. Especially if we start with an older database dump that knows nothing about fields that were previously added.

I think we don't need to do anything here, at least not for the scenario that I encountered.

donquixote → created an issue.

So....
AnnotatedClassDiscovery actually uses StaticReflectionParser instead of loading the file directly.
This way it can avoid loading a class where the base class might not be defined.
It then adds the plugin definition, without ever loading the respective class.
The plugin is never used only because the field type 'address' does never occur while 'address' module is not installed.

The 'plugin' module does include the plugin classes for all formatters, because the plugin field does not (and cannot) filter them by field type.

So, not sure if anything needs to be done here.

Actually.... I think I am wrong on this one.
The plugin type is just "FieldFormatter", nothing to do with address module, so there is something else wrong here.

donquixote → created an issue.

Special named classes are only for the "implements alter" because I am just not sure whether it's doable or desirable to fire off an event in the middle of DrupalKernel::compileContainer().

Alright. Still not sure that magic naming is the best solution, but..

Other question, do we want `hook_implements_alter()` to work for non-procedural implementations?
With the current PR this would not be the case.
This means whenever a procedural hook gets converted, any code that targets it with hook_implements_alter() would need to be converted too.

Now that the event dispatcher itself allows any object to be passed in rather than requiring an Event class, I think they count as events again. But as soon as we get to multiple parameters and alters etc. that's no longer true either.

So the main differences are:
- by-reference parameters
- multiple parameters
- return value (and the mechanism to combine the return values)

So... yeah mostly not events then but definitely an extension of them.

So we could say Drupal has an extended events system that supports single-parameter classical event subscribers, but also multiple-parameter dispatching with return etc.
So, symfony event system could be seen as a subset of the extended Drupal event system.

Ofc the term "event" implies that there is an actual event object. Or at least this would be a typical understanding of it.
So do we call it an "extended event system" or rather a "dispatching system"?
(Conceptually, "event" does not necessarily mean the object, it can also just mean a thing that happens)

My previous comment suggested creating an implements alter magic named class for the purpose of, well, implements altering the new style ones.

In Hux you can put `#[Hook('something_alter')]` or `#[Alter('something')]` on any method of a hux-enabled class.
I think this DX is preferable over a magic named class.
(The #[AsEventListener] would also still be better than any kind of magic naming.)

In practice, I like to name these classes after their purpose, and often have one class implement multiple hooks with related functionality, or multiple classes implement the same hook for separate functionality.

Typical class names might be "AssignSourceId" or "CoAuthorNodeAccess" or "HideSomethingField".
The class name does not reveal which hook(s) are used to achieve the result, and we might in fact decide to use a different hook without renaming the class.
(Of course others might have different conventions how to name their classes)

I am happy with the feedback so far, thanks catch and donquixote. I do not see any major pain points raised nor anything which would require adding a lot of code. My goal here is to keep both boilerplate and added core complexity down to an absolute minimum.

My previous attempt at this at 📌 Hux-style hooks, proof of concept Needs work had _a lot_ more code.
Unfortunately I did not have consistent time and attention for it.

The main reason for the complexity, from what I remember,
It was a lot more ambitious in terms of BC, and also tried to cover wishlist items like relative ordering etc.

I like the simpler approach proposed here, but it does mean we lose _some_ functionality and BC, especially if we convert existing hook implementations. There might be more nasty details that I no longer remember atm.

I wouldn't worry much about BC, of course old hooks can't just die from one release to the next but I expect the conversion to be automated and very swift.

I am not worried about the effort of the conversion.
My BC concern is about custom and contrib code which:
- relies on a specific order of execution
- uses hook_module_implements_alter() to add, remove or rearrange functions
- relies on conditionally included files ($module.$group.inc)
- implements a hook on behalf of another module
- (perhaps more stuff which I don't remember)

But, if we aim for 11.x then I think this is fine.

So, perhaps like this:
- In 10.x we introduce a BC-friendly mechanism that allows contrib and custom code to define hooks in the new way.
- In 11.x we convert the core hooks.
- In 12.x we drop the old system (as you already proposed).

I do not know much about phpstan but we have excellent people who do -- but if push comes to shove I can write it with nikic/php-parser in less than a day -- I would find out the event name and the function starting and ending line with it but do the actual conversion with just string operations.

You could have a look at the conversion commits here, https://git.drupalcode.org/project/drupal/-/merge_requests/4347/commits

Could we have a subclass of event dispatcher that has a method (or methods) with the same logic? Then obviously you couldn't call dispatch() on those events, but (once the hook bc layer is dropped) it would eventually be possible to call those methods directly instead of via the module handler.

Or we could keep it as a separate class, which just uses the symfony event dispatcher internally.
This would follow the composition over inheritance idea, and avoid interface creep.

Having one big event dispatcher class and interface which also supports hook-style dispatching would mean that components that need both only need one dependency injected. But is this a good reason? Not sure.

One conceptual / philosophical question is whether we think of these new OOP hooks as events, or as something separate, which just uses the event system to collect subscribers.
I think it matters for DX:
If we advertise it as one system, then developers will have the same expectations for hooks and events. This could lead to a situation of surprise or disappointment.
If we advertise it as separate systems (as it is now), then developers can have separate mental models with separate expectations. In this case we should also keep a separate attribute #[Hook(..)].

A developer only sees the dispatcher methods and the subscribed method (signature and return), and both feel different in hooks vs events, even if internally we use the event system for both.

This looks very interesting!

So essentially we use the events mechanism to manage the hook implementations, but we use ModuleHandler to actually call/invoke them.

This also means you are not supposed to invoke these special "hook" events via the event system.
At least those hooks that have return values or that receive more than one parameter.

This is probably fine.
But do we want to "pollute" the event list with pseudo-events that cannot really be used as such?
Or should we have a separate instance of the event system for hooks?

I can't really think of any gotchas here except for hook_module_implements_alter() - but that would still work, implementations might just have to adjust and to run last you'd need to use the new API but that is fine.

This also means that implementations of hook_module_implements_alter() that _remove_ an implementation will no longer work if that implementation gets converted.

I assume this means that class hooks always run after function hooks? It will potentially mean some hook_module_implements_alter() would need updating as implementations get converted, but also I think that is fine.

This is the same as currently in Hux.

If we start replacing core hook implementations, there will be a BC impact due to the changed order.
Maybe we truly target 11.x, or we accept the damage.
Or we introduce the API but don't convert any existing core hook implementations before 11.0.

An interesting question will be alter hooks in general, especially the combined ones like "form" + "form__somethingform".
We could go for a similar solution, if we don't care to replicate the order of execution from the old system, which is quite complex.
Again, the main BC impact will happen if we try to convert existing implementations.

Adding two more scenarios:
- One that is very easy to reproduce.
- One that is explicitly about base fields.

I notice this also happens with entity base fields.

I had the following scenario:
- I worked on a feature that adds a new custom entity type.
- I add base fields.
- I exported the view and form display.
- Now I add entity bundles and replace the base fields with field UI fields.
- I export the view and form display again.

Expected: Base fields are removed from form and view display.
Actual: Base fields remain in the form and view display, even after re-saving the form or view config.

I found that even the old mechanism would not have worked.
So nothing was made worse in this issue.
Sorry for the noise.

The Router::matchCollection() does not call the access checks.
It only calls $route->getCondition() which seems to provide some additional regex checks, but this does not really help to distinguish bundles.

So this is a dead end: We should not have multiple routes with the same path.

The solution implemented here does not work if we have multiple routes with same path, but for different bundles.

The ParamConverter is only called _after_ the route was already picked in Router::matchCollection().
So it is too late at that point.

Something needs to happen with core, as ThemeManager and friends are hell to work with in 2024.

The ThemeManager is a beast indeed, especially the global state around the "current theme".

But adding preprocess callbacks via hook_theme_registry_alter() is quite transparent.
I have done this a lot in Drupal 7, and the mechanism still is the same.

Is something above worth extracting to its own project? Is it reliable/predictable?

Yes, it seems this could go into a separate project.
It could even be fully independent of Hux, although it would be nice to reuse the discovery mechanism.

It seems quite reliable so far.
One thing I don't like is the `\Drupal::service()` calls.
Perhaps theme preprocess will be changed to support service methods, similar to '#pre_render'.

One tricky part could be the more specialized theme hooks, like `region__content`. I will have to investigate a bit more what happens if we add a `$registry['region__content']['preprocess functions'][] = ...`, if there was not such a hook originally.

I like to put multiple hook implementations (and preprocess) into the same class if all belong to the same "behavior".
So I think I would activate the `Hooks` namespace for preprocess.

The following already works with current hux:

#[\Attribute(\Attribute::TARGET_METHOD)]
class Preprocess {

  /**
   * Constructor.
   *
   * @param string $hook
   *   Name of the theme hook.
   */
  public function __construct(
    public readonly string $hook,
  ) {}

}

/**
 * Base class for classes with theme preprocess callbacks.
 */
abstract class PreprocessBase {

  /**
   * Constructor.
   *
   * @param \Drupal\Component\DependencyInjection\ReverseContainer $reverseContainer
   */
  public function __construct(
    private readonly ReverseContainer $reverseContainer,
  ) {}

  /**
   * Implements hook_theme_registry_alter().
   *
   * @param array<string, array> $registry
   *   Theme registry.
   */
  #[Alter('theme_registry')]
  public function themeRegistryAlter(array &$registry): void {
    $reflection_class = new \ReflectionClass(static::class);
    $methods = $reflection_class->getMethods(\ReflectionMethod::IS_PUBLIC);
    foreach ($methods as $method) {
      $attributes = $method->getAttributes(Preprocess::class);
      if ($attributes === []) {
        continue;
      }
      if ($method->isStatic()) {
        $preprocess_callback = [static::class, $method->getName()];
      }
      else {
        $service_id = $this->reverseContainer->getId($this);
        assert($service_id !== NULL);
        $preprocess_callback = [static::class, $service_id . '->' . $method->getName()];
      }
      foreach ($attributes as $attribute) {
        $attribute_object = $attribute->newInstance();
        assert($attribute_object instanceof Preprocess);
        $hook = $attribute_object->hook;
        if (!isset($registry[$hook])) {
          continue;
        }
        $registry[$hook]['preprocess functions'][] = $preprocess_callback;
      }
    }
  }

  /**
   * Invokes a preprocess method on a service object.
   *
   * @param string $name
   *   Synthesized method name, contains the service id and actual method name.
   * @param array $arguments
   *   Arguments to pass to the actual method.
   */
  public static function __callStatic(string $name, array $arguments): void {
    [$service_id, $method_name] = explode('->', $name);
    $service = \Drupal::service($service_id);
    call_user_func_array([$service, $method_name], $arguments);
  }

}

class MyPreprocessHooks extends PreprocessBase {

  /**
   * Theme preprocess for 'block'.
   */
  #[Preprocess('block')]
  public function preprocessBlock(array &$variables): void {...}

}

For a proper solution we would not rely on ReverseContainer and inheritance, instead this would work out of the box for all classes with hooks.

Could you implement hook_preprocess in hux itself (as in hux_preprocess) and then have that call the manager?

There is a nicer way to implement preprocess:
Use hook_theme_registry_alter() to register the preprocess callbacks.

Unfortunately as it is now these functions cannot be methods on services.
From ThemeManager::render():

      foreach ($info['preprocess functions'] as $preprocessor_function) {
        if (is_callable($preprocessor_function)) {
          call_user_func_array($preprocessor_function, [&$variables, $hook, $info]);
        }
      }

But we could use tricks with __callStatic().
https://3v4l.org/SlUU8
https://3v4l.org/kPUuQ
https://3v4l.org/JBk1u

class MyService {
    function preprocess(array &$variables): void {
        $variables['x'] = 'X';
    }
}

class Drupal {
    static function service($id): object {
        return new MyService();
    }
}

class ServiceCallback {
    static function __callStatic(string $f, array $args) {
        ['service' => $service_id, 'method' => $method] = unserialize($f);
        $service = \Drupal::service($service_id);
        $service->$method(...$args);
    }
}

$variables = [];

$callback = ['ServiceCallback', serialize(['service' => 'myservice', 'method' => 'preprocess'])];
call_user_func_array($callback, [&$variables]);

assert($variables === ['x' => 'X']);

donquixote → created an issue.