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

Fix the PHP version check with the use of Semver #52

Merged
merged 2 commits into from Jul 27, 2018
Merged

Fix the PHP version check with the use of Semver #52

merged 2 commits into from Jul 27, 2018

Conversation

tvbeek
Copy link
Contributor

@tvbeek tvbeek commented Jul 25, 2018

The current check only takes the PHP version from the composer.json file and checks if the current version is the same or higher. But composer has more options, that if used can be conflicted or incorrect tested. See: https://getcomposer.org/doc/articles/versions.md#summary

I have changed the behaviour by using the Semver package from composer to check it.

The only question I have is the change on the getRequiredPhpVersion function. Because it is a public function I didn't want to change it and added a new function getRequiredPhpConstraint. That means that there is now a function that isn't used by the check. If wanted I can mark it as deprecated

@Namoshek
Copy link
Contributor

Namoshek commented Jul 25, 2018

In my opinion, the code can be removed because your suggested changes are already breaking the backwards-compatibility, so why bother about one function? A compromise could be though to keep the old check logic as well and make the check configurable. I.e. introduce something like 'semver_comparison' => true as configuration for the check. But personally, I would say the semver comparison is the better one and should be used in future.

Just a quick thought right now: We are only checking the php-cli version currently. Is there any way we could also check the php-fpm version (if php-fpm is used)?

@mpociot
Copy link
Member

mpociot commented Jul 26, 2018

Thanks for the PR. I also think that the semver check is better and that we can remove the old method.
Can you adjust the PR?

@tvbeek
Copy link
Contributor Author

tvbeek commented Jul 26, 2018

Thanks for the feedback, I have remove the old function (And the test for that function)

@mpociot mpociot merged commit 67d5dda into beyondcode:master Jul 27, 2018
@tvbeek tvbeek deleted the correct_version_installed_lower_php_fix branch October 1, 2018 11:44
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.

None yet

3 participants