Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow ignoring "@group legacy" annotated test methods when Symfony PHPUnit bridge is in use #64

Closed
mondrake opened this issue Feb 11, 2022 · 12 comments · Fixed by #99
Closed

Comments

@mondrake
Copy link

It would be useful to allow skipping analysis of @legacy annotated PHPUnit test methods test in PHPUnit.

Drupal runs deprecation tests via PHPUnit and the Symfony's PHPUnit bridge. Current policy for runtime deprecated code is to write a @legacy annotated test that explicitly contains calls to deprecated code. PHPStan would now report these calls as errors, which is a duplicated check vs. existing tools. We cannot ignore entire files since it may well be that normal and deprecation tests are part of the same test class.

See also:

@ondrejmirtes
Copy link
Member

This PHPDoc tag is specific to Drupal, it's very non-standard.

You have two options here:

  1. Switch @legacy to @deprecated - phpstan-deprecation-rules don't report deprecated calls from already deprecated scope.
  2. Baseline the current errors: https://phpstan.org/user-guide/baseline

@mondrake
Copy link
Author

No, it's not a drupal-ism, it's Symfony

https://symfony.com/doc/current/components/phpunit_bridge.html#mark-tests-as-legacy

@ondrejmirtes
Copy link
Member

Doesn't matter - it's still PHPUnit/Symfony specific.

@alexpott
Copy link

@ondrejmirtes is there anyway that you'd consider making this somehow configurable? Like maybe a solution would be to allow projects to configure which annotations mean deprecated so projects that use make use of @legacy could make phpstan-deprecation-rules consider that the same as @deprecated.

@alexpott
Copy link

The suggestion:

allow projects to configure which annotations mean deprecated
does not work :( it's not @legacy it's @group legacy (this was pointed out to me by @catch56)

So maybe this is a won't fix and our only choice is to mark the tests @deprecated too.

@mondrake mondrake changed the title Allow ignoring @legacy annotated test methods when Symfony PHPUnit bridge is in use Allow ignoring "@group legacy" annotated test methods when Symfony PHPUnit bridge is in use Feb 16, 2022
@ondrejmirtes
Copy link
Member

I'd accept a PR for a new extension type - it can be an interface similar to this one: https://github.com/phpstan/phpstan-src/blob/master/src/Rules/Exceptions/ExceptionTypeResolver.php

It would allow users to have a custom logic called to decide if a scope is deprecated or not.

@mglaman
Copy link
Contributor

mglaman commented Jul 20, 2022

Coming back to this:

So we'd need a new extension type for deprecations. Like DeprecationTypeResolver which has isDeprecated and this would allow adding support for checking the @legacy tag in phpDoc alongside the existing @deprecated tag check. Which would also help answer/solve #73.

And this all goes into phpstan-src, not here.

Edit: I read more, so the extension would go here. Would we add configuration to this extension then, allowing to specify additional deprecated tag strings?

deprecatedTags: listOf(string()),

Or something which received the type, so more thorough inspection could be done?

@mglaman
Copy link
Contributor

mglaman commented Jul 28, 2022

@ondrejmirtes I know you're super busy chasing down some performance items in PHPStan, but I wanted to ping and see if you had more guidance on how you'd like to see the extension. I'll have work time available to add this.

I'd like to support @group legacy and a custom node visitor to flag nodes as being in a deprecated scope based on if/else for backward compatibility support.

@bbrala
Copy link

bbrala commented May 30, 2023

Still quite relevant this.

@mglaman
Copy link
Contributor

mglaman commented May 31, 2023

I gave this a shot in #99. It's quite a bit of changes but adds scope resolvers to extend what is considered a deprecated scope.

@github-actions
Copy link

github-actions bot commented Jul 9, 2023

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 9, 2023
@ondrejmirtes
Copy link
Member

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants