search

liip / LiipFunctionalTestBundle

Some helper classes for writing functional tests in Symfony
http://liip.ch
MIT License
640 stars 181 forks source link

Added a fix for symfony 5.3 container. #593

Closed biozshock closed 2 years ago

biozshock commented 2 years ago

Fixes #592

alexislefebvre commented 2 years ago

Thanks for the PR.

Is $env something that was added in Symfony 5.4?

biozshock commented 2 years ago

No, that's the code needed to properly boot a kernel with the selected environment with static function createKernel

alexislefebvre commented 2 years ago

Why $environment was removed?

biozshock commented 2 years ago

it's not static.

alexislefebvre commented 2 years ago

Ok, thank you.

So we can't keep $environment, even as a fallback, because it can't be accessed in createKernel() ?

Please add a note in https://github.com/liip/LiipFunctionalTestBundle/blob/master/CHANGELOG.md to explain how to upgrade. It looks like it's a BC break so it may require a new major release.

biozshock commented 2 years ago

That's not a BC break, because you may still use old environment property without any consequences, except you'd get a silenced deprecation message. Next major release should remove the deprecation and environment property.

alexislefebvre commented 2 years ago

Could you please fix the failing test? Do you need help?

alexislefebvre commented 2 years ago

Thank you! These changes are available in the last release: https://github.com/liip/LiipFunctionalTestBundle/releases/tag/4.5.0

mikysan commented 2 years ago

That's not a BC break, because you may still use old environment property without any consequences, except you'd get a silenced deprecation message. Next major release should remove the deprecation and environment property.

This is actually a BC break, in my company test suite the WebTestCase class was extended and $environment protected property overwritten, updating this library broke all the functional tests because the environment was once again defaulted to "test" (without us knowing it). Also the tests errors were about missing configuration without the environment explicitly showed by phpunit and no deprecation warning raised.

Also note that overriding protected static $env = 'test'; in the extend class doesn't fix the issue due to the use of self::$env instead of static::$env. So there are actually two issues...

I didn't manage to find any issue about this, should I open them? If there's one already opened could you please link it?

alexislefebvre commented 2 years ago

Thanks for the report and sorry for the inconvenience.

There's no issue about this regression yet, feel free to add an issue and/or open a PR.

We could restore protected $environment = 'test'; and a method to read self::$env, or $this->environment if the former has the default value?

mikysan commented 2 years ago

Ok, I'll open 2 issues and details there. Btw I won't be able to contribute with any PR until Friday hope it's ok.

Edit: issues opened, see https://github.com/liip/LiipFunctionalTestBundle/issues/594 and https://github.com/liip/LiipFunctionalTestBundle/issues/595