Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Symfony 4.4 and 5.x #55

Merged
merged 1 commit into from
Feb 12, 2020
Merged

Support Symfony 4.4 and 5.x #55

merged 1 commit into from
Feb 12, 2020

Conversation

damien-carcel
Copy link
Contributor

@damien-carcel damien-carcel commented Feb 10, 2020

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.

@damien-carcel damien-carcel force-pushed the support-sf-5 branch 5 times, most recently from c4dea11 to 11a7515 Compare February 11, 2020 15:28
@jjanvier
Copy link
Contributor

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
Copy link

ahocquard commented Feb 12, 2020

@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
Copy link
Contributor

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

@damien-carcel
Copy link
Contributor Author

@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 to composer.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.

@jjanvier jjanvier merged commit 630c8ab into master Feb 12, 2020
@jjanvier jjanvier deleted the support-sf-5 branch February 12, 2020 16:31
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