Elasticsearch-php: PHP 5.6.6 minimum requirement?

[ellisio]

I am currently working on helping update a package the depends on this package. It is targeted at the Laravel community. Seeing as how Laravel 5.3.x requires PHP >= 5.6.4, this causes breaking changes not only for the Laravel packaging, but on Travis-CI.

Travis currently installed PHP 5.6.5, and requiring 5.6.6 is against Travis' recommendation to force a particular version.

Is there a specific reason why PHP 5.6.6 is required? Would it break the library if we bumped it down to >= 5.6.4?

[ellisio]

Digging into this a little more and I noticed that your dev-master will require PHP >= 7.0 as PHP 5.6.x ships with JSON 1.2.1 and you're requiring JSON >= 1.3.7.

Maybe drop support for 5.6 entirely?

[polyfractal]

Sorry for the delay. I wrote more about this requirement here: https://github.com/elastic/elasticsearch-php/issues/529#issuecomment-273533587

Basically, we need JSON_PRESERVE_ZERO_FRACTION, which was added in 5.6.6. Unfortunately, this also means we need json-ext 1.3.7 or higher. It's caused a ton of headache, since it appears a number of distros are shipping old versions (even with newer versions of PHP sometimes).

Personally I'd love to drop support for 5.6, but I think that'd be a bwc break and not possible in the 5.x branch.

Edit: Forgot to reply to this:

Would it break the library if we bumped it down to >= 5.6.4?

If you really need to, you can require a lower version and inject your own serializer (like in that linked comment) which doesn't use the flag. That's the only bit in the library that depends on 5.6.6+

[ellisio]

Roger. Is there any consideration on dropping JSON_PRESERVE_ZERO_FRACTION while you're supporting 5.6.x?

You could even do something like this:

$version = phpversion();

if (version_compare($version, '5.6.6', '<') || ! defined('JSON_PRESERVE_ZERO_FRACTION')) {
    // Use old method.
} else {
    // Use new method.
}

This would allow you to drop to 'php': '>= 5.6.4'.

Thoughts?

[polyfractal]

Hmm, the bigger issue is the json extension. E.g. we can polyfill the constant:

if(!defined('JSON_PRESERVE_ZERO_FRACTION'))
{
    define('JSON_PRESERVE_ZERO_FRACTION', 1024);
}

but the json-ext needs to be high enough to support it. And it's possible to have a 5.6.6+ without the appropriate json-ext, so that requirement would have to stay. I'm assuming that won't help your situation any (removing the PHP requirement but leaving json-ext)?

We could do what you propose -- revert to old behavior on unsupported platforms -- but I'm conflicted about this. It's essentially fixing buggy behavior, so allowing the old method to continue operating is basically reverting a bug. Especially since 5.6 is EOL now.

I'm not sure... it's a pickle.

FWIW, I just removed 5.6 support from master. So at least that will be squared away.

[ellisio]

Yeah, unfortunately this still breaks the Laravel 5.3.x support as they require a minimum of 5.6.4. I'm in the process of rolling out a new product over the next couple days. Once I'm past this chaos I'll put some thought into a possible way to make this friendly for everyone.

Maybe I can convince the package maintainer to bump their minimum requirements to 5.6.6 as well. Most Laravel developers are conscious enough upgrade their PHP versions. It has been a while since I've actually seen anyone using 5.6.x; most of them seem to have upgraded to 7.0 for the performance gains.

Worst case scenario, we wait until January 1, 2018. That's the EOL for Laravel 5.3.x haha.

[polyfractal]

Heh. Lemme think about it. Perhaps we do something like:

• Remove php and json-ext version requirement
• Add the conditional to use the correct method if possible
• Add a check during client initialization that throws an exception, along the lines of "Your version doesn't support a needed constant, see $builder->optIntoJSONBug()"
• Once the user willingly acknowledges they are opting into the buggy behavior, we allow everything to proceed as before.

So in your case (where the user isn't instantiating the client directly), you could just opt into that flag if you detect an old version. And for the rest of the users, they get a warning in the face before proceeding.

[polyfractal]

Ok, so I just tagged v5.1.1. This allows the user to set $builder->allowBadJSONSerialization() on the ClientBuilder, which opts into the old behavior. If you don't set this, and your version doesn't have the right json-ext, it'll throw a big exception admonishing you to upgrade :)

Lemme know if that works for you!

[zim32]

Where to get php5-json higher than 1.3.6 for Debian? Even in unstable Debian the highest version is 1.3.6
https://packages.debian.org/search?keywords=php5-json&searchon=names&suite=all&section=all

[polyfractal]

Oops, forgot to remove the PHP version dependency too... will go do that and tag a release. Sorry!

[polyfractal]

Tagged in v5.1.2. Note that if you are using a 'bad' version of json-ext it'll still throw an exception at runtime. You'll need to do the following:

$client = ClientBuilder::create()           
                    ->setHosts($hosts)
                    ->allowBadJSONSerialization() // Allow bad versions of json-ext
                    ->build();   

[ellisio]

Nice work man! Solid solution :)

@zim32 You will need to use the ppa:ondrej/php repo:

sudo add-apt-repository ppa:ondrej/php
sudo apt-get update
sudo apt-get upgrade
sudo apt-get install php7.1   # for PHP 7.1
sudo apt-get install php7.0   # for PHP 7.0
sudo apt-get install php5.6   # for PHP 5.6

[coraxster]

@polyfractal
in 5.1.1
Method allowBadJSONSerialization() does not not provide fluent interface.
I cant find branch with this code to commit fix. 5.1.2 has not this method.

lines 401-404

public function allowBadJSONSerialization()
{
    $this->allowBadJSON = true;
}

Thank you for fix!

[polyfractal]

Argh, I'll fix that in just a sec, thanks

[polyfractal]

Fixed in 5.1.3. :)

[polyfractal]

Closing as I think this has been resolved. Feel free to re-open or file a new issue if some bug comes up with the implementation!