laminas / laminas-di

Automated dependency injection for PSR-11 containers
https://docs.laminas.dev/laminas-di/
BSD 3-Clause "New" or "Revised" License
36 stars 20 forks source link

enable convertDeprecationsToExceptions #30

Closed remicollet closed 2 years ago

remicollet commented 3 years ago

convertDeprecationsToExceptions="true"required by phpunit 9.5.10

Yes, this looks like a terrible BC break...

ping @sebastianbergmann

sebastianbergmann commented 3 years ago

The change in PHPUnit you are refering to was made to reduce the friction for developers upgrading to PHP 8.1, see https://externals.io/message/116127#116154 and https://externals.io/message/116127#116148 for reference.

This change only breaks backward compatibility for what I consider an edge case situtation: a situation where you want to test that your code triggers an E_USER_DEPRECATED event, for instance.

Looking at the result I get when running the tests for the 3.3.x branch of this repository as-is with PHPUnit 9.5.10 ...

$ ./vendor/bin/phpunit
PHPUnit 9.5.10 by Sebastian Bergmann and contributors.

...............................................F
Deprecated: Detected legacy DI configuration, please upgrade to v3. See https://docs.laminas.dev/laminas-di/migration/ for details. in /home/sb/laminas-di/src/Container/ConfigFactory.php on line 34
...............  63 / 268 ( 23%)
............................................................... 126 / 268 ( 47%)
........................F
Deprecated: Full qualified parameter positions are no longer supported in /home/sb/laminas-di/src/LegacyConfig.php on line 56
...F
Deprecated: Laminas\Di\Resolver\AbstractInjection is deprecated, please migrate to Laminas\Di\Resolver\InjectionInterface in /home/sb/laminas-di/src/Resolver/AbstractInjection.php on line 18
.................................. 189 / 268 ( 70%)
..................................F
Deprecated: Laminas\Di\Resolver\TypeInjection::getType is deprecated. Please migrate to __toString() in /home/sb/laminas-di/src/Resolver/TypeInjection.php on line 71
............................ 252 / 268 ( 94%)
...............F                                                268 / 268 (100%)
Deprecated: Laminas\Di\Resolver\ValueInjection::getValue is deprecated, please migrate to Laminas\Di\Resolver\ValueInjection::toValue(). in /home/sb/laminas-di/src/Resolver/ValueInjection.php on line 121

Time: 00:00.088, Memory: 12.00 MB

There were 5 failures:

1) LaminasTest\Di\Container\ConfigFactoryTest::testLegacyConfigTriggersDeprecationNotice
Failed asserting that exception of type "PHPUnit\Framework\Error\Deprecated" is thrown.

2) LaminasTest\Di\LegacyConfigTest::testFQParamNamesTriggerDeprecated
Failed asserting that exception of type "PHPUnit\Framework\Error\Deprecated" is thrown.

3) LaminasTest\Di\Resolver\AbstractInjectionTest::testUsageIsDeprecated
Failed asserting that exception of type "PHPUnit\Framework\Error\Deprecated" is thrown.

4) LaminasTest\Di\Resolver\TypeInjectionTest::testGetTypeIsDeprectaed
Failed asserting that exception of type "PHPUnit\Framework\Error\Deprecated" is thrown.

5) LaminasTest\Di\Resolver\ValueInjectionTest::testGetValueTriggersDeprecatedNotice
Failed asserting that exception of type "PHPUnit\Framework\Error\Deprecated" is thrown.

FAILURES!
Tests: 268, Assertions: 557, Failures: 5.

... it appears that exactly this is going on here.

I am sorry that reducing friction for the majority of users causes friction for projects such as this.

boesing commented 3 years ago

We will probably re-enable this feature in the CI container while making it opt-out using .laminas-ci.json for non-laminas projects. This way, we do not need to modify 140+ components. I keep this open just to keep track of it. No ETA so far but as I don't think that this one here is urgent (as phpunit released yesterday?) that should be fine.

remicollet commented 3 years ago

BTW, still BC break...

Isn't possible to re-enable this configuration option when this exception is explicitly expected, ie, when

$this->expectDeprecation("PHPUnit\Framework\Error\Deprecated::class);

boesing commented 3 years ago

Not sure if I can follow. I can see tests passing after adding this flag to the phpunit.xml.dist. What do you think is BC breaking here exactly?

remicollet commented 3 years ago

@boesing I mean BC break in phpunit (not in this project)

So the question is for @sebastianbergmann

sebastianbergmann commented 3 years ago

$this->expectDeprecation("PHPUnit\Framework\Error\Deprecated::class);

Code like that makes no sense. And please stop discussing PHPUnit in non-PHPUnit repositories.

tux-rampage commented 3 years ago

This component uses runtime deprecations. Either we drop these entirely and annotate with deprecated (which I prefer) or we need to enable this explicitly.

I've also done this in #21 tdh

tux-rampage commented 3 years ago

$this->expectDeprecation("PHPUnit\Framework\Error\Deprecated::class);

Code like that makes no sense. And please stop discussing PHPUnit in non-PHPUnit repositories.

Agree, we should definitely address this here.

Ocramius commented 2 years ago

Overall, laminas uses convertDeprecationsToExceptions="true", to prevent deprecation warnings to leak into end-user code.

We do test our deprecations on purpose.

Ocramius commented 2 years ago

Covered in #37 - keeping in milestone to make sure it makes it to release notes though :)