tobyzerner / json-api-php

JSON-API (http://jsonapi.org) responses in PHP.
MIT License
436 stars 79 forks source link

PHP 5.3 / 5.4 compatibility #77

Closed alessandrofrancesconi closed 8 years ago

alessandrofrancesconi commented 8 years ago

I've updated this great lib after some months and sadly I've discovered that the compatibility with PHP 5.3 has been dropped. Is there a chance to get it back?

byCedric commented 8 years ago

I don't think it's a great idea to support PHP 5.3. Basically the main reason is that is no longer being supported by PHP itself. It does NOT receive updates or even security patches. Therefore both PHP 5.3 and PHP 5.4 should NOT be used anymore...

For more info, check the timeline of all PHP versions. http://php.net/supported-versions.php

alessandrofrancesconi commented 8 years ago

I agree. It is "just" because I'm using json-api on a project that's intended to run on a wide range of PHP versions.

alessandrofrancesconi commented 8 years ago

Sorry for reopening but correct me if I'm wrong. Does the support to at least 5.4 magically returns if you simple replace Relationship::class with get_class(...) in https://github.com/tobscure/json-api/blob/master/src/AbstractSerializer.php#L65 ?

GrahamCampbell commented 8 years ago

:-1: for supporting unmaintained php versions due to security reasons.

alessandrofrancesconi commented 8 years ago

If "supporting" means "give support", I'm with you. But, in this case, we are talking about letting people use this library on a platform (5.4) that is still present in many hosting services. To be precise, according to w3techs, PHP 5.4 is used by the 30% of the market. If I was the author of a library of that is suitable for a lot of needs, like json-api, I'd friendly keep the support to at least the "most recent discontinued" version, or I'll lose an important user base.

GrahamCampbell commented 8 years ago

If "supporting" means "give support", I'm with you.

No, I mean forcefully preventing.

To be precise, according to w3techs, PHP 5.4 is used by the 30% of the market.

I don't know where they have their data from, but it looks like BS to me. If it's via the php version header, than anyone with a non-crappy setup will have disabled that header broadcasting the php version.

GrahamCampbell commented 8 years ago

https://seld.be/notes/php-versions-stats-2015-edition is a more accurate reflection.

alessandrofrancesconi commented 8 years ago

Well... just look at the intro of your article:

A quick note on methodology, because all these stats are imperfect as they just sample some subset of the PHP user base. I look in the packagist.org logs of the last 28 days for GET /packages.json which represents a composer update done by someone. Composer sends the PHP version it is running with in its User-Agent header, so I can use that to see which PHP versions people are using Composer with. Of course this data set is probably biased towards development machines and CI servers and as such it should also be taken with a grain of salt.

Seems waaay more inaccurate to me. Instead, w3techs website is even suggested by the PHP team itself: http://php.net/usage.php

Don't say "BS!" for things to which you disagree :smiley:

GrahamCampbell commented 8 years ago

http://php.net/usage.php

This doesn't reflect the usage of the target audience. We're not targeting wordpress users using php 4, we're targeting people using composer on real applications.

alessandrofrancesconi commented 8 years ago

We're not targeting wordpress users using php 4

No one said that, c'mon.

GrahamCampbell commented 8 years ago

Well, those version stats include those people.

GrahamCampbell commented 8 years ago

Well, tbh, my real view is I don't care what version of php people are using, if it's not a maintained version, then they need to upgrade, end of story.

franzliedke commented 8 years ago

If it's just about the ::class constant, then I agree it doesn't hurt to make this small change. This is a library, not an application. Application creators / maintainers are responsible themselves to stay on supported PHP versions. If for some reason they can't make that switch, then we have no reason to put any obstacles in their way.

byCedric commented 8 years ago

I agree with GrahamCampbell. If the creator of the language deprecates a version, it should not be used anymore. It may be prone to bugs and or security issues. The only outcome is being hacked or not able to maintain due to unexpected behaviour. Maybe not soon, but it has no other outcome I'm afraid.

Now the question you should ask is; "Should the whole community stick with an old, outdated, syntax just to please anyone who is unable to upgrade?". My personal answer is no, we should not.

If some one is using an old outdated version of PHP, his main concern should be how to upgrade instead of making new features. I can go on and on about the current state of the world and the many security issues we encounter daily. But I hope you get the point... Simply don't use EOL PHP...

byCedric commented 8 years ago

So before everything gets all emotional and people start fighting I would propose a solving solution. I think both parties have made their arguments clear. This is essentially a project of @tobscure, so lets ask his opinion. Also I looked to all people involved in the discussion and placed them into two groups as some kind of voting mechanism.

supports EOL PHP does not support EOL PHP
@alessandrofrancesconi @byCedric
@franzliedke @GrahamCampbell
@normance @jbrooksuk
@vinkla
@pedrommone

If your name is listed, and don't want to be listed, drop me a pm.

So let's end this by making a decision.

jbrooksuk commented 8 years ago

:+1:

vinkla commented 8 years ago

@byCedric :+1: GitHub should add votes to issues and pull requests.

franzliedke commented 8 years ago

It would be nice if we could disagree with each other without getting impolite.

As with everything in development, this decision is one of tradeoffs. Both sides have their merits, obviously. It is simply not true that there's only one true way to handle this.

In the end, it's @tobscure who calls the shots on this repository. Voting won't change that. ;)

And finally, to add something constructive: how about this? We could merge the proposed PR, but leave the PHP version requirement in composer.json intact. That way, PHP 5.4 compatibility is maintained (as there's no real reason to break it - we're currently just saving a few keystrokes), but we still encourage the use of supported PHP versions. If somebody "knows what they're doing" or has no choice but to use an unsupported PHP version (for whatever reason), they can explicitly use this library on PHP 5.4 by installing it with Composer's --ignore-platform-reqs.

normance commented 8 years ago

I am happy this discussion is close to reach a conclusion. My company is using PHP 5.4 because of some third parties libraries, including APC. Of course we are going to move forward to the head of PHP by Sept 2016 using PHP 7, but this is a major upgrade meaning changing our Zend Framework 1.12, APC, etc. Until now and then, I can not enjoy all the improvements you have made since approx August 2015.

pedrommone commented 8 years ago

As developers we should encourage version upgrade, not downgrade it.

:-1: for downgrade.

alessandrofrancesconi commented 8 years ago

As developers we should encourage version upgrade, not downgrade it.

No one is encouraging a downgrade. Will you go back to 5.4 after this fix? We are talking about extended compatibility, it's way different.

pedrommone commented 8 years ago

@alessandrofrancesconi if someone support old versions people don't mind on upgrading it.

alessandrofrancesconi commented 8 years ago

if someone support old versions people don't mind on upgrading it.

Not when the new version brings clear advantages (as for PHP) and the product can easily support both the old and the new release without affecting its functionalities (as for json-api). I will be the first to upgrade.

I'm wondering why @tobscure didn't express his opinion yet... this discussion is getting hilarious :dancers:

maxowar commented 8 years ago

Consider that RedHat comes with php 5.4 (guess why) and enterprise companies use it.

Anyway, you are right guys, but the point is that you cannot use this API just because the use of a non-necessary feature of the language.

pedrommone commented 8 years ago

@alessandrofrancesconi now I understand your point. JSON API can easly support it without major modifications, right?

alessandrofrancesconi commented 8 years ago

Yes @pedrommone, just look at the push request #78: it's just one line of code.

bnchdrff commented 8 years ago

If you need to support PHP 5.3, why not maintain a patched private repository for your own project as shown in https://getcomposer.org/doc/05-repositories.md#using-private-repositories -- this way the rest of the world doesn't need to worry about it, and this issue can die a quiet death :skull: :smile_cat: :skull:

edit: more specific instructions: https://getcomposer.org/doc/05-repositories.md#loading-a-package-from-a-vcs-repository

tobyzerner commented 8 years ago

Alright folks, sorry about the delay here. Thanks for the informative discussion.

To be honest, my personal preference would be to keep using ::class and not support PHP 5.4. But because I like to please, and because it's such a simple one-off change, I've decided to remove the use the use of ::class (2e1d78e311f6c5879e529ec2310aeec1efe0873d) and thus maintain compatibility with PHP 5.4. As per @franzliedke's suggestion though, the support is unofficial in that the version constraint in composer.json remains unchanged:

If somebody "knows what they're doing" or has no choice but to use an unsupported PHP version (for whatever reason), they can explicitly use this library on PHP 5.4 by installing it with Composer's --ignore-platform-reqs.

In only unofficially supporting it, I'm not making any guarantees about PHP 5.4 compatibility in the future. If too many more instances of potential ::class usage pop up, or other features from later PHP versions are going to be useful, then PHP 5.4 support may disappear.

alessandrofrancesconi commented 8 years ago

Thank you!