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 5.3 #76

Merged
merged 2 commits into from
Mar 4, 2015
Merged

Drop support for PHP 5.3 #76

merged 2 commits into from
Mar 4, 2015

Conversation

colinodell
Copy link
Member

No description provided.

@colinodell colinodell added the feedback wanted We need your input! label Feb 25, 2015
@colinodell
Copy link
Member Author

Support for 5.3 will be dropped by the end of the week. If you feel it's too early for this change, please let me know why, otherwise I shall proceed with this merge.

@cebe
Copy link

cebe commented Feb 25, 2015

What's the point in dropping support for a version without any technical reason? I would understand if you'd want to use traits now which are not supported in 5.3...

@GrahamCampbell
Copy link
Member

php 5.3 is eol, so anyone using it now is using it at risk of being exploited. There are known, unfixed security vulnerabilities.

@cebe
Copy link

cebe commented Feb 25, 2015

I know that and I agree that one should not use it anymore and upgrade as soon as possible.
But imo that is not the job of a composer.json requirement, this has to be done on a different level.
The requirements in composer.json should reflect the technical requirements of a library and not what the author thinks is an appropriate php version to be installed on users system.

It is good to point out that 5.3 is old and should not be used but patronising people that may be stuck with 5.3 for some reason and may be in the progress of upgrading is an unnecessary hurdle.

@colinodell
Copy link
Member Author

We had a good discussion about this on the League's mailing list: https://groups.google.com/d/msg/thephpleague/632Kzor2hu8/rwJXs-wLWc8J

There have been at least three instances where supporting 5.3 prevented us from implementing positive changes:

  • A user-contributed PR which used traits to simplify the codebase (5.4)
  • The old block-closing closure couldn't reference $this, so we had to set $self = $this; and then use ($self) to workaround that limitation (5.4)
  • The Environment configuration references element types using their full class name. Would be nice if we could use the special ::class constant instead of hard-coding the full namespaced class name (5.5)

So while you're right that there's no immediate technical need, I do feel we are unnecessarily restricting ourselves from positive enhancements by supporting an unsupported version.

Anyone who can't upgrade from 5.3 is welcome to use the current 0.7 release which is fairly stable and feature-packed, so it's not like we're completely abandoning them.

@colinodell colinodell mentioned this pull request Feb 25, 2015
@colinodell colinodell added this to the Version 0.8 milestone Feb 25, 2015
@philsturgeon
Copy link
Member

+1 to that last message.

It's not like you're doing it for no reason, or purely for short arrays, but you're doing it to make larger useful changes which will benefit code cleanliness and performance.

@cebe
Copy link

cebe commented Feb 25, 2015

I am totally fine with doing it for technical reasons. It was not obvious from the changes in the PR and I did not know about the discussion thread. Go ahead and merge! :)

@philsturgeon
Copy link
Member

philsturgeon commented Feb 25, 2015 via email

@cebe
Copy link

cebe commented Feb 25, 2015

👍 cebe/markdown#8 (comment)

@colinodell
Copy link
Member Author

Just encountered a 5.3 bug: 5747822

Unfortunately this workaround is only needed for versions < 5.4.8 but it results in a ~5% performance hit to all versions :-/

My current plan is to address all remaining issues and release a final 0.7.x version. After that I'll start working towards 0.8, which will drop 5.3 support and undo that workaround.

@GrahamCampbell
Copy link
Member

👍 for only requiring newer versions of 5.4 and upwards. :)

colinodell added a commit that referenced this pull request Mar 4, 2015
@colinodell colinodell merged commit 0877e51 into master Mar 4, 2015
@colinodell colinodell deleted the drop-php-53 branch March 4, 2015 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback wanted We need your input!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants