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

Drop support for PHP 7.1 #1703

Merged
merged 1 commit into from
Nov 19, 2019
Merged

Drop support for PHP 7.1 #1703

merged 1 commit into from
Nov 19, 2019

Conversation

deguif
Copy link
Collaborator

@deguif deguif commented Nov 18, 2019

Follow up of #1701

@deguif deguif changed the title Dropped support for PHP 7.1 Drop support for PHP 7.1 Nov 18, 2019
@deguif deguif force-pushed the update-php-version branch 2 times, most recently from 89b2dbc to a4d1f23 Compare November 18, 2019 14:24
Copy link

@philkra philkra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -16,7 +16,6 @@ env:
- DOCKER_VERSION=17.09.0
- DOCKER_COMPOSE_VERSION=1.17.1
matrix:
- TARGET="71"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this all work but keep this target + the Docker file in? Or will all the tests fail because of the composer requirement?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this won't work because of the composer requirement, at least if --ignore-platform-reqs is not used

@deguif deguif force-pushed the update-php-version branch from a4d1f23 to 3c39813 Compare November 18, 2019 15:36
@ruflin
Copy link
Owner

ruflin commented Nov 19, 2019

A few general questions:

  • How widely is 7.1 still used?
  • Is it easy for users to upgrade to 7.2 or later?
  • What are the things you expect we introduce in Elastica which will not be compatible with 7.1?

I'm good with moving forward with this PR. In the past what I did was that what we test and what is "supported" was not necessarily the same. This meant as soon as we introduced a feature that actually broke a previous PHP version we knew and then removed the testing. Like this it was easy to communicate what broke it.

@deguif
Copy link
Collaborator Author

deguif commented Nov 19, 2019

@ruflin I'm not an expert on the subject but here some stats from composer: https://blog.packagist.com/php-versions-stats-2019-1-edition/

Upgrading to PHP 7.2 should be pretty straightforward.
And some big packages on composer already switched to PHP 7.2 as minimum requirement, see @symfony 5 for example.

I think it could be judicious to really remove PHP 7.1 support as you are about to publish a new major version. This also brings opportunity to boost PHP 7.2 adoption if users of packages need this new version, which is good for the PHP community.

I see two PHP 7.2 features that could be used: object type-hint and basic contravariance.

@ruflin ruflin merged commit 1909c67 into ruflin:master Nov 19, 2019
@ruflin
Copy link
Owner

ruflin commented Nov 19, 2019

@deguif Thanks for sharing the additional details. I especially like the point around boosting adoption of 7.2.

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