search

akeneo / php-coupling-detector

[Experimental] Detect all the coupling issues of your project with respect to the coupling rules you have defined.
MIT License
42 stars 6 forks source link

Support Symfony 4.4 and 5.x #55

Closed damien-carcel closed 4 years ago

damien-carcel commented 4 years ago

This PR proposes an update to Symfony 4.4|5.0. The main issue in the update was with the event dispatcher "dispatch" method which has a different signature in SF5 compared to SF3.

The application is also tested against the currently supported versions of PHP: 7.2, 7.3 and 7.4. As a result, phpspec was updated to version 6 (the only one to support PHP 7.4) and support for PHP 7.1 was removed as it is not supported anymore and not compatible with phpspec 6.

phpstan was also updated to 0.12 as the previously used version doesn't work with Symfony 5.

Finally, composer.lock was removed as composer will generate a different one for each tested PHP version. Thanks to cache management, the CI still takes less than one minute.

jjanvier commented 4 years ago

Hello @damien-carcel ! Thanks for the PR :) Really cool :)

One question: where does Symfony\Contracts comes from? Is it a dependency of the event dispatcher? If not, we should require it with composer.

One remark: I disagree with "Finally, composer.lock was removed...". composer.lock helps to install exactly the dependencies that we developed the tool with. I don't see any value in removing it.

GTM otherwise

ahocquard commented 4 years ago

@jjanvier actually the composer.lock download the dependencies you want, but it's created with the current PHP version you run the composer install. Changing the PHP version can impact the dependency version. For example, Doctrine added a requirement on PHP version in a minor release and not in a major release. So, you could have this new Doctrine library version if you use a supported PHP version to run the composer, or stick with the previous Doctrine library version if you don't.

In other tems, composer.lock is correct for only one version of PHP which is the one you run the composer install.

jjanvier commented 4 years ago

So maybe the best is to remove the php requirement from composer. I don't know.

damien-carcel commented 4 years ago

@jjanvier I think you mix up 2 concepts. I removed the composer.lock because Travis tests the application against different PHP versions (as it is installed in projects using different PHP versions: PIM 3.x uses PHP 7.2, PIM 4.0 and Serenity and the next version of the Supplier Onboarder use PHP 7.3 and the Onboarder middleware uses PHP 7.4). As Composer will not install the same set of dependencies regarding the version of PHP, it is impossible to use a composer.lock, as it will be compatible with only one of them.

Concerning the PHP requirement in composer.json, this is a different matter: I just set the minimum version to 7.2 as it is the one required by phpspec v6 (phpspec v5 doesn't support PHP 7.4, so no way to use it to test this PHP version). However, I could probably define the dev requirements with "phpspec/phpspec": "^5.0|^6.0" and be able to run and test PHPCD with PHP 7.1. But nothing in this requirement relates tocomposer.lock`.

Concerning Symfony/Contracts, it comes as a dependency of the Event Dispatcher component, for both Symfony 4.4 and 5.0. Here is the doc https://symfony.com/doc/current/components/contracts.html. In my opinion, there is no need to explicitly add them to composer.json.

My only concern here is the CI always tests against Symfony 5.0. It could be cool to find a way to force the install of Symfony 4.4 to run the tests against it too.