@phpstan-ignore-next-line annotation collides with PHPCS

Thanks for reporting!

I think you can use PHPStan ignores like this:

class Schema extends BaseMySqlSchema {

  /**
   * {@inheritdoc}
   *
   * @phpstan-ignore-next-line missingType.return 
   */
  public function addField($table, $field, $spec, $keys_new = []) {
    if ...

Can you try if that works? I would like avoid writing special code to detect and ignore PHPStan comments as that would make a lot of our sniffs more complicated.

Downgrading priority to normal, I don't think this is a major issue as you can do this workaround.

I think Moondrake might have a valid point here. I checked to see what sniff is reported, and phpstan-ignore, when used in this context, gives the message:

---------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------
 12 | ERROR | [x] You must use "/**" style comments for a function comment
    |       |     (Drupal.Commenting.FunctionComment.WrongStyle)
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------

When you run phpcbf, the fixed file is incorrect, it becomes:

class Schema extends BaseMySqlSchema {

  /**
   * This is the original function docblock.
   */

  /**
   * @phpstan-ignore-next-line missingType.return
   */
  public function addField($table, $field, $spec, $keys_new = []) {
  }

So maybe it would be worth trying to disregard the phpstan-ignore token as suggested, then Coder would (a) not produce this invalid fixed file and (b) allow phpstan to properly ignore the next line as required.

I have created PR263 which detects and disregards // @phpstan-ignore comments when searching back from the function definition looking for the function docblock.
https://github.com/pfrenssen/coder/pull/263

If this is going to be OK, then I can add a test case for it, but won't do all that if its a no-goer.

This would be great, having to phpcs-ignore the phpstan ignore declarations is slightly eye watering.

Yes that is tedious. This solution works for the single case reported, but ideally we should try to ignore all 'phpstan-...' inline comments. I think this is what Klausi was referring to, as being a lot of work. phpcs_codesniffer helpfully tokenizes their own phpcs- comments, so it is easy to disregard them using Tokens::$phpcsCommentTokens. But unfortunately the phpstan comments are not pre-tokenized, so we have to detect them individually, as I did in this PR.

If this approach is acceptable then maybe we can make that conditional (the if and preg_match) into a function that is callable from other places, so that the improvement can be utilized in other sniffs.

Thanks, I think the approach makes sense!

Please add test cases to good.php:
1. phpstan ignore before function
2. phpstan ignore before annotation and function
3. phpstan ignore between annotation and function

From #8

If this approach is acceptable then maybe we can make that conditional into a function that is callable from other places

Is there a place where utility functions like this can be added, so they can be used in other sniffs in future? Or shall I just leave it hard-coded in this file for now?

Good question - I think you can put it in a public static function, if that is possible. Then all classes that need it can call into that. Not the most obvious solution but should work for now, we can always try to expose it better in the future.

I have created the separate public static function and added one test case. Ready for feedback before I add the other test cases.

Do we have a 'test-only' way to show that this would fail without the enhancement? I have tested locally and it is working as required, and good.php fails without this change. But it would be nice to have that demonstratable in the Pull Request workflow.

Thanks, approach looks good to me!

The test-only way would be to open a new pull request just with the test additions, can't think of a better way right now.

Yes, please add the other test cases!

The test-only way would be to open a new pull request just with the test additions, can't think of a better way right now.

OK that's fine. I just wanted to know if you already have this type of test automated. I may try to get some ideas together on how to do this automatically in another job, alongside the other tests, in a similar way to how I adapted the core test to make the 'test-only changes' job for Gitlab CI. But that's a separate task.

For the test cases you requested:
1. phpstan ignore before function
2. phpstan ignore before annotation and function
3. phpstan ignore between annotation and function

I have already added:

/**
 * Test for @phpstan-ignore-next-line.
 */
// @phpstan-ignore-next-line missingType.return
public function ignore_phpstan_comment() {
}

This seems to cover 1 and 3

For case 2 are you saying:

// @phpstan-ignore missingType.return
/**
 * Test 
 */
public function ignore_phpstan_comment() {
}

I don't understand how this is affected by the problem. This would pass anway, before these changes. Or maybe I mis-understood your request.

Sorry, I used the wrong words. I meant PHP attributes:

/**
 * Test with attribute before.
 */
// @phpstan-ignore-next-line
#[ExampleAttribute('foo', 'bar')]
public function ignore_phpstan_comment() {
}

and

/**
 * Test with attribute after.
 */
#[ExampleAttribute('foo', 'bar')]
// @phpstan-ignore-next-line missingType.return
public function ignore_phpstan_comment() {
}

Ah, OK yes I see. I have pushed the addition of those two examples. They all pass the Drupal sniffs, but the version with // @phpstan between the function block and the #[attribute] fails with

--------------------------------------------------------------------------
[x] Expected 1 blank line before function; 0 found
(Squiz.WhiteSpace.FunctionSpacing.Before)
--------------------------------------------------------------------------

See https://github.com/pfrenssen/coder/actions/runs/15210892656/job/42784511...

We can't change that as it is an external sniff. But I think that is OK, as it's not really what we want to allow. The main thing is that the modified sniff allows the @phpstan comment to preceed the function definition, both with and without the #[attribute]

I will remove that failing test case then check again. There may be other problems, as the logs are obscured by lots of deprecation messages.

I've removed the unwanted 3rd test case and the pipeline passes all jobs now. Ready for review.

Fully agreed. Merged, thanks!

That's great. Thank you.