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

Hi, thanks

No that does not work - there's the PHPDoc ending token */ in an isolated line and PHPStan does not take it into account - it says that there are no errors to ignore on that line.

I think Moondrake might have a valid point here. Not only for the PHPStan problem, but we also the phpstan-ignore line 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 becomes:

class Schema extends BaseMySqlSchema {

  /**
   * Something.
   */

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

The phptan-ignore has been converted into a function comment. 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 cerated 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 conditiona (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?

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.

I do not think

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

needs to be supported - PHPStan will fail in that case as the ignore is not in the line immediately above the concrete code failing.

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.

Thank you!