Skip to content

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Aug 26, 2025

closes #6335

if (!$attributes->isEmpty()) {
if (!$annotations->isEmpty()) {
EventFacade::emitter()->testRunnerTriggeredPhpunitWarning(
'PHPDoc annotations will be ignored when attributes are declared',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we need to be more precise here, that we talk only about PHPUnit related annotations.. ?

wording suggestions welcome.

@staabm staabm marked this pull request as ready for review August 26, 2025 10:19
@staabm
Copy link
Contributor Author

staabm commented Aug 26, 2025

when projects support a wide range of php versions, they likely need to run their test-suite across multiple phpunit versions (therefore they might configure their tests with phpdoc and attributes in tandem)

see e.g. PHP-CS-Fixer/PHP-CS-Fixer#8965 (comment)

the warning might get annoying in such cases

Comment on lines +44 to +45
if (!$attributes->isEmpty()) {
if (!$annotations->isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In projects with old PHP versions support, multiple PHPUnit versions needs to be supported.

In order to be able to run multiple PHPUnit versions, annotations might be needed to be supported together with attributes.

The warning should be only if the annotations and attributes does not equal.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The warning should be only if the annotations and attributes does not equal.

This is not possible without parsing the annotations and PHPUnit >= 12 does not have the code for that anymore.

multiple PHPUnit versions needs to be supported

I disagree.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so seems this is ready to land

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

Successfully merging this pull request may close these issues.

3 participants