Problem/Motivation

There are currently 3 arguments.count errors in PHPStan baseline.

Proposed resolution

Fix them.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#40 3520730-nr-bot_gaq9z9ob.txt1.57 KBneeds-review-queue-bot
#31 3520730-nr-bot_vicyuvbp.txt91 bytesneeds-review-queue-bot
#29 3520730-nr-bot_0l9c78a7.txt91 bytesneeds-review-queue-bot
#8 3520730-nr-bot.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3520730

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mondrake created an issue. See original summary.

mondrake opened merge request !11925

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Needs review

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

smustgrave’s picture

smustgrave commented

Aren't these considered API changes for some?

mondrake’s picture

mondrake
Location 🇮🇹
commented

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...

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Related issues: +#3354672: [Meta] Adjust parameters in interfaces

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

mondrake’s picture

mondrake
Location 🇮🇹
commented

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€.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3520730-nr-bot.txt91 bytes

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.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Reviewed & tested by the community

rebased

xjm’s picture

xjm
she/her
Primary language English
commented
Title: Fix PHPStan arguments.count error » Fix PHPStan arguments.count error related to interface argument additions in the next major
Status: Reviewed & tested by the community » Needs work

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?

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

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.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

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

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

Sorry, one more suggestion now that (apparently) 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 and target branches so that the recently reverted unstable Ajax test thing stops making functional JS tests blow up? Thanks!

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Reviewed & tested by the community

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.

xjm’s picture

xjm
she/her
Primary language English
commented

Strange? OK, thanks @mondrake!

xjm’s picture

xjm
she/her
Primary language English
commented

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!

xjm’s picture

xjm
she/her
Primary language English
commented

Saving credits. Almost accidentally credited the bot.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

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. 🐹

smustgrave’s picture

smustgrave commented

@xjm this good to go now?

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Needs work

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 change record @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!

mondrake’s picture

mondrake
Location 🇮🇹
commented

@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.

xjm’s picture

xjm
she/her
Primary language English
commented

@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.

xjm’s picture

xjm
she/her
Primary language English
commented
Assigned: Unassigned » xjm

xjm opened merge request !12630

xjm’s picture

xjm
she/her
Primary language English
commented
Assigned: xjm » Unassigned

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. 🎆

xjm’s picture

xjm
she/her
Primary language English
commented

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

mondrake closed merge request !11925

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

What more work is needed here?

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3520730-nr-bot_0l9c78a7.txt91 bytes

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.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3520730-nr-bot_vicyuvbp.txt91 bytes

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.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

rebased

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Going on a limb but believe feedback for this one has been addressed with the new MR. If I'm wrong sorry!

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

Question about the deprecation skips (and I have similar questions about other generic skips that we have added)

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

I checked the regex in the edited ignore statements based on the feedback from #34. The statements now target specific namespaces and classes in Drupal Core. So the feedback was addressed.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

I think we need two change records here, one for each interface; if there are any contrib or custom implementations of these interfaces they will break when we actually add the new argument, so we should tell them what to do?

mondrake’s picture

mondrake
Location 🇮🇹
commented
Version: 11.x-dev » main
mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Reviewed & tested by the community

Rebased and added the two draft CRs.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3520730-nr-bot_gaq9z9ob.txt1.57 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Reviewed & tested by the community
Issue tags: +no-needs-review-bot

bot fluke

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 13d18b6bd92 to main and 7b931dcb3fa to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

longwave closed merge request !12630

  • longwave committed 7b931dcb on 11.x 
    task: #3520730 Fix PHPStan arguments.count error related to interface...

  • longwave committed 13d18b6b on main 
    task: #3520730 Fix PHPStan arguments.count error related to interface...
mondrake’s picture

mondrake
Location 🇮🇹
commented
Related issues: +#3570908: Fix signatures of ExecutableInterface and CategorizingPluginManagerInterface

Filed follow-up.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.