Introducing PHP8 compliancy

[giovannialbero1992]

No description provided.

[elasticmachine]

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

[deguif]

@ezimuel Is there anything we can do to help moving this forward.

ruflin/elastica added support for PHP 8.0 recently but had to skip platform requirements while testing in its CI as elasticsearch/elasticsearch doesn't support PHP 8.0 in its composer requirement.

I think this should target branch 7.x?

[mfn]

The title of the PR

Introducing PHP8 compliancy

I'm starting to test run this library on PHP 8 in my company and found some issues (using v7.10.0) but was surprised this PR does not touch any of the code parts I saw errors 🤷‍♀️

For example, without a vendor patch hack I can't connect at all:

ErrorException: Elasticsearch\Connections\Connection::Elasticsearch\Connections\{closure}(): Argument #2 ($key) must be passed by reference, value given

/vendor/elasticsearch/elasticsearch/src/Elasticsearch/Connections/Connection.php:358
/vendor/elasticsearch/elasticsearch/src/Elasticsearch/Connections/Connection.php:218
/vendor/elasticsearch/elasticsearch/src/Elasticsearch/Transport.php:110
/vendor/elasticsearch/elasticsearch/src/Elasticsearch/Client.php:1637
/vendor/elasticsearch/elasticsearch/src/Elasticsearch/Client.php:310
…

In this case this is the diff for the "fix" (removed the unused ❗ &$key variable) in

elasticsearch-php/src/Elasticsearch/Connections/Connection.php

Line 345 in dfd2b62

function (&$value, &$key) {

Index: vendor/elasticsearch/elasticsearch/src/Elasticsearch/Connections/Connection.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/vendor/elasticsearch/elasticsearch/src/Elasticsearch/Connections/Connection.php b/vendor/elasticsearch/elasticsearch/src/Elasticsearch/Connections/Connection.php
--- a/vendor/elasticsearch/elasticsearch/src/Elasticsearch/Connections/Connection.php	(revision 8f1256488b3c58bec0e3027bbc4d74a2408204f0)
+++ b/vendor/elasticsearch/elasticsearch/src/Elasticsearch/Connections/Connection.php	(revision 1c323624f0f741645ab9af0c66d6deffd01553e9)
@@ -349,7 +349,7 @@
         if (isset($params) === true && !empty($params)) {
             array_walk(
                 $params,
-                function (&$value, &$key) {
+                function (&$value) {
                     if ($value === true) {
                         $value = 'true';
                     } elseif ($value === false) {

Anyone else seeing this? Should I create an issue or PR directly? I'm just not sure as it seems these tests here seem to be green on PHP 8 nevertheless.

I'm also running tests on GHA using setup-php but I did run them recently, so maybe it's a new regression (or it's just simply not covered in the suite 🤷‍♀️)

Thanks

[deguif]

@mfn good catch.
See https://3v4l.org/ZmrdO to confirm that the code underlined by @mfn is breaking in PHP 8.

[mfn]

Smart 😄 Thank you @deguif 🙇

[deguif]

@mfn this should also be fixed by #1075

[mfn]

@deguif Thank you, I confirmed it #1075 (comment) !

[ezimuel]

@giovannialbero1992, @deguif, @mfn, @OskarStark thanks for your contribution! I'll update soon, thanks!

[pascaldevink]

@ezimuel @deguif PHP 8 was released today, is there any update on the progress of this PR?

[reedy]

I guess with 8.0 being table, we shouldn't need to use RC4 anymore (I'm presuming newer images exist)...

[giovannialbero1992]

Hi @reedy, I updated the image used 😃

[ezimuel]

Hi @giovannialbero1992 sorry for the delay, I finally have some time to review the PR. I noticed that you sent it to master, can you rebase and sent to 7.x branch? Thanks!

[ezimuel]

@giovannialbero1992 I fixed some PHP 8 compatibility issues:

• added version 3 of cpliakas/git-wrapper in composer;
• removed the annotation for expecting exceptions in ClientBuilderTest.php, for PHPUnit 9.

Moreover, I reintroduced the support of PHP 7.1, we still need to support it 😱 until next major of elasticsearch-php.

[ezimuel]

Welcome PHP 8!!!

[mfn]

Awesome!

FTR, I won't be really working with PHP8 until #1075 is in, too!