🇩🇪Germany @donquixote

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.

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.