Fix compatibility with PHPUnit 11

twigphp / Twig

Twig, the flexible, fast, and secure template language for PHP

https://twig.symfony.com/

BSD 3-Clause "New" or "Revised" License

8.18k stars 1.25k forks source link

Fix compatibility with PHPUnit 11 #4313

Closed MauricioFauth closed 2 months ago

MauricioFauth commented 2 months ago

When PHPUnit\Framework\Attributes\DataProvider attribute is found, PHPUnit ignores the @dataProvider annotation. This behavior can be used to fix compatibility with PHPUnit 11.

Adds static data providers for Twig\Test\IntegrationTestCase. Running PHPUnit 10+ will require Twig\Test\IntegrationTestCase::getFixturesDirectory() to be implemented.
Removes the Twig\Test\NodeTestCase::getTests() data provider from the DataProvider attribute. This way it doesn't get called with PHPUnit 10+ causing the tests to fail.

Related to:

stof commented 2 months ago

No go for this feature as it does not make tests compatible with PHPUnit 10+ if projects have not been migrated yet away from the deprecated APIs, which goes against the way deprecations should work.

If you read the discussion on https://github.com/twigphp/Twig/pull/4266, you will see that support for newer PHPUnit versions in the IntegrationTestCase will be part of Twig 4.0

MauricioFauth commented 2 months ago

@stof Would it be acceptable if tests are skipped if the method has not been implemented yet and is running PHPUnit 10+?

MauricioFauth commented 2 months ago

@stof Would it be acceptable if tests are skipped if the method has not been implemented yet and is running PHPUnit 10+?

Or skipped only for PHPUnit 11 if the method is not implemented, however that will require adding a new test method that only runs on PHPUnit 11 (no impact to the user).

stof commented 2 months ago

The issue is not the test method. It is the fact that the old methods used inside the data provider were not static, so we cannot implement a static data provider that is compatible with projects that haven't migrated.

Having a changelog entry saying Add compatibility with PHPUnit 11 in IntegrationTestCase while existing usages of IntegrationTestCase are not compatible with PHPUnit 11 would be totally weird. That's why the plan is to have this Add compatibility with PHPUnit 11 in IntegrationTestCase in Twig 4.0 instead.

MauricioFauth commented 2 months ago

@stof I opened a new pull request as I couldn't push to this one anymore. The new PR does not causes an error when getFixturesDirectory is not implement and triggers a deprecation instead.

https://github.com/twigphp/Twig/pull/4317

stof commented 2 months ago

@MauricioFauth but even if you trigger a deprecation, this is not properly backward compatible as it does not run the expected tests (while running those tests is the expected behavior of the IntegrationTestCase)

MauricioFauth commented 2 months ago

@stof I understand, but I don't think this is a BC break, because the expected tests don't run already. It's not like they are perfectly running before and now they don't, which is a BC break. So instead of causing errors and failing the test suite, these tests are now skipped and a deprecation is triggered, if the method is not implemented.

stof commented 2 months ago

@MauricioFauth the current state of Twig is "supports PHPUnit 10 and lower for IntegrationTestCase", not "supports PHPUnit 11 without running test"