Fix PHPStan arguments.count error

Huh, harder than I thought. This is adding future additional interface parameters, since it seems that they are just missing currently.

Aren't these considered API changes for some?

Future changes, yes, following the process:
https://www.drupal.org/node/3376455 →
https://www.drupal.org/about/core/policies/core-change-policies/how-to-d... →

I know we were looking to close that meta but guess it’s been used another round.

Going to comment there I suggest closing the meta in 12

Chances here seem fine then

Though without a dedicated CR may be a surprise to some when the parameter just appears

Agree we should close that meta issue, made changes there accordingly.

wrt to the CR, I am unsure. tbh I find the as-is a bit weird, many implementations of the interface methods rely on those params that are not in the interface signature - to the point that I was confused, started trying to remove them in the implementations just to realize afterwards I should do the other way around i.e. adding them to the interfaces. So for me we are just getting things right as they should be here, not changing conceptually the API. My 0.02€.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

rebased

Thanks @mondrake for working on this.

My main concern here is disabling the entire commenting standard over whole methods just because of (I think) the commented-out parameters inside the method params. Can we find a more targeted way to do that?

Narrowed the PHPCS uncovered code by replacing phpcs:(dis|en)able with targeted phpcs:ignore single line ignore directives, specifying the rule to be ignored.

Seems feedback from @xjm has been addressed to disable specific rules vs disable

Sorry, one more suggestion now that (allegedly) Coder may have fixed the single-line ignore thing for method signature lines. If it doesn't work, we can take your current approach.

Also, can you rebase the source branch so that the recently reverted unstable Ajax test thing stops making functional JS tests blow up? Thanks!

If we remove the three lines

   * @todo Uncomment new method parameter before drupal:12.0.0.
   * @see https://www.drupal.org/project/drupal/issues/3354672
   * phpcs:ignore Drupal.Commenting.FunctionComment.ParamNameNoMatch,Drupal.Commenting.DocComment.ParamGroup

from the MR and run phpcs on the file, we get

FILE: [...]/core/lib/Drupal/Core/Executable/ExecutableInterface.php
------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------
 15 | ERROR | Doc comment for parameter $object does not match actual variable name <undefined>
------------------------------------------------------------------------------------------------

Time: 168ms; Memory: 10MB

i.e. phpcs tells us the failing line is the one in the docblock, not the actual method declaration.

And, per https://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage#ignorin...,

You can [...] ignore a single line using the phpcs:ignore comment. If placed on a line by itself, this comment will ignore the line that the comment is on and the following line.

So, adding the ignore like this

  /**
   * Executes the plugin.
   *
   * @param object|null $object
   *   (Optional) An object to execute the plugin on/with.
   */
  // phpcs:ignore Drupal.Commenting.FunctionComment.ParamNameNoMatch,Drupal.Commenting.DocComment.ParamGroup
  public function execute(/* ?object $object = NULL */);

I checked it does not work.

Strange? OK, thanks @mondrake!

Oh ah, I get it of course. Because as far as phpcs is concerned, the @param refers to something that does not, as yet, exist. Sorry for being slow on the uptake!

Saving credits. Almost accidentally credited the bot.

Sorry, I thought I was about to commit this, but then had another question about the formatting. You are my guinea pig for improving this standard. 🐹

@xjm this good to go now?

Well I disagree with:

No I just thought this would be best given that this is all temporary, and the endgame is to have a list of @param annotations that should not have space in between.

To me it seems we are adding PHPCS overrides in order to make the section hard to read. We do not do this with the deprecation and @see for API removals; those are spaced out properly. I think these should be too. I think we can simplify it a lot by spacing it normally and removing the unnecessary PHPCS overrides, right?

Can we try changing it to my suggestion? Then we can compare the two and if needed get feedback from the coding standards folks. :D

Thanks!

@xjm it would help if you could post how you would envisage a docblock to look exactly like in this case, WITHOUT the phpcs caveats. With that, we can try to decorate it with the phpcs:ignore directives that phpcs will tell us are necessary.

@mondrake, OK, that's fair, will try to take a look myself when I can. :) Sorry for adding time and scope to the issue but I think we can improve the existing policy a lot.

I pushed up a branch with the formatting I had in mind. I think this is far more readable, requires fewer phpcs ignores, and also allows for the fact that there might be multiple interface parameters or typehints added in a given major.

We'll see what the bot says and if this format just works or if there would be blockers. Forgive me for not scanning it locally; I'm watching several cities' worth of fireworks from my deck. 🎆

That passed pipelines. I shall now propose the standard be adjusted accordingly!