6.x compatible with php8.0 #1977

AskaoAhmedSaad · 2021-09-10T10:12:28Z

make version 6.x compatible with php8.0

ruflin

@AskaoAhmedSaad It would be nice if we could make 6.x compatible with PHP 8.0 but we should not make it a breaking change for existing users. Any ideas on how we can make it non breaking? I remember we had similar challenges with other PHP versions and reserved terms. Can you check it how we have solved it in other places / PR?

ruflin · 2021-09-13T06:18:31Z

lib/Elastica/Query/Match.php

@@ -1,216 +0,0 @@
-<?php


This looks like a breaking change?

It definitely is :)

reedy · 2021-09-22T14:46:05Z

I was going to try and make a similar patch...

Don't remove the Match class; if it exists and isn't executed (thanks to the magic that is the autoloader; unless new Match or similar exists, it will be ignored), it won't be a problem for PHP 8.0.

Swap Match and MatchQuery around (so the Match alias extends MatchQuery instead of the other way around), and then update documentation.

If people are still using Match as the canonical in their own code, they'll need to update that to work on PHP 8.0 anyway, so that's not a problem. Anyone still using Match and not needing to support PHP 8.0 doesn't need to make any changes.

reedy · 2021-09-22T14:46:47Z

composer.json

@@ -12,7 +12,7 @@
        }
    ],
    "require": {
-        "php": "^7.0",
+        "php": "^7.0 | ^8.0",


Use || as per https://github.com/ruflin/Elastica/blob/master/composer.json#L18 for consistency

reedy · 2021-09-22T14:47:39Z

lib/Elastica/Query/Match.php

- * @deprecated since version 6.1.2, use the MatchQuery class instead.
- * @see https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-match-query.html
- */
-class Match extends AbstractQuery


This should become class Match extends MatchQuery and otherwise be empty of content

Keeping the comments above too, of course.

reedy · 2021-09-22T14:48:21Z

lib/Elastica/Query/MatchNone.php

@@ -3,7 +3,7 @@
 namespace Elastica\Query;

 /**
- * Match none query. Returns no results.
+ * MatchQuery none query. Returns no results.


This shouldn't be replaced. See master https://github.com/ruflin/Elastica/blob/master/src/Query/MatchNone.php#L6

reedy · 2021-09-22T14:48:24Z

lib/Elastica/Query/MatchAll.php

@@ -3,7 +3,7 @@
 namespace Elastica\Query;

 /**
- * Match all query. Returns all results.
+ * MatchQuery all query. Returns all results.


This shouldn't be replaced.

reedy · 2021-09-22T14:48:29Z

lib/Elastica/Query/MatchPhrase.php

@@ -3,7 +3,7 @@
 namespace Elastica\Query;

 /**
- * Match Phrase query.
+ * MatchQuery Phrase query.


This shouldn't be replaced.

reedy · 2021-09-22T14:49:27Z

lib/Elastica/Query/MultiMatch.php

@@ -94,7 +94,7 @@ public function setOperator($operator = self::OPERATOR_OR)
    }

    /**
-     * Set field minimum should match for Match Query.
+     * Set field minimum should match for MatchQuery Query.


This shouldn't be replaced.

reedy · 2021-09-22T14:50:01Z

lib/Elastica/Query/MultiMatch.php

@@ -106,7 +106,7 @@ public function setMinimumShouldMatch($minimumShouldMatch)
    }

    /**
-     * Set zero terms query for Match Query.
+     * Set zero terms query for MatchQuery Query.


This shouldn't be replaced.

reedy · 2021-09-22T14:50:09Z

lib/Elastica/Query/MultiMatch.php

@@ -120,7 +120,7 @@ public function setZeroTermsQuery($zeroTermQuery = self::ZERO_TERM_NONE)
    }

    /**
-     * Set cutoff frequency for Match Query.
+     * Set cutoff frequency for MatchQuery Query.


This shouldn't be replaced.

reedy · 2021-09-22T14:50:25Z

test/Elastica/Query/PercolateTest.php

@@ -26,7 +26,7 @@ public function testPercolateQueryOnNewDocument()
        $doc = new Document(2, ['message' => 'A new bonsai tree in the office']);
        $this->documentType->addDocument($queryDoc);
        $this->index->refresh();
-        //Match a document to the registered percolator queries:
+        //MatchQuery a document to the registered percolator queries:


This shouldn't be replaced.

reedy · 2021-09-22T14:50:29Z

test/Elastica/Query/PercolateTest.php

@@ -40,7 +40,7 @@ public function testPercolateQueryOnNewDocument()
        $this->index->refresh();
        $resultSet = $this->index->search($percolateQuery);

-        //Match a document to the registered percolator queries:
+        //MatchQuery a document to the registered percolator queries:


This shouldn't be replaced.

reedy · 2021-09-22T14:52:14Z

Looks like this has just been done as a literal find and replace. Only really the code uses of the Match query needed updating. Replacing most (all?) of the Match comments with MatchQuery make no sense and don't align with what's on master.

And reverting them makes the diff smaller for people to review.

AskaoAhmedSaad · 2021-09-22T15:37:00Z

@reedy @ruflin having a class with the name "Match" gives a parser error at PHP 8 environment, do you have a suggestion to overcome this and at the same time not to break the other PHP versions ?

I think maybe we can have a special version name with removing Match class totally and replace it with MatchQuery

reedy · 2021-09-22T15:53:30Z

@reedy @ruflin having a class with the name "Match" gives a parser error at PHP 8 environment, do you have a suggestion to overcome this and at the same time not to break the other PHP versions ?

I think maybe we can have a special version name with removing Match class totally and replace it with MatchQuery

It should only error if you execute the file using it (or lint it etc)... What are you doing to cause/see that error?

reedy · 2021-09-23T03:51:41Z

I re-made your commit without the comment changes in https://github.com/reedy/Elastica/tree/6xphp8

composer.json:

{
	"require": {
		"ruflin/elastica": "dev-6xphp8 as 6.2"
	},
	"repositories": [
		{
			"type": "git",
			"url": "https://github.com/reedy/elastica.git"
		}
	]
}

Test PHP script:

<?php

require __DIR__ . '/vendor/autoload.php';

use Elastica\Query\MatchQuery;

$mq = new MatchQuery();
echo "Done\n";

Run it

reedy@MacBook-Pro elasticatest % php --version    
PHP 8.0.10 (cli) (built: Aug 26 2021 15:36:17) ( NTS )
Copyright (c) The PHP Group
Zend Engine v4.0.10, Copyright (c) Zend Technologies
    with Zend OPcache v8.0.10, Copyright (c), by Zend Technologies
reedy@MacBook-Pro elasticatest % php test.php 
Done

reedy · 2021-09-23T05:56:17Z

CI seems fine with the Match class existing too...

I've got #1981 to the point where I think I'm just fiddling with fixing up some minor test breakages (function calls that need updating)

ruflin · 2021-09-23T06:48:35Z

As #1981 goes more into the direction we want to go I'm going to close this PR for now. @AskaoAhmedSaad Thanks for kicking this off and I would appreciate if you could chime in on #1981 In case we change direction, we can still reopen this one.

AskaoAhmedSaad changed the base branch from master to 6.x September 10, 2021 10:12

make version 6.x compatible with php8.0

87c72eb

AskaoAhmedSaad force-pushed the 6.x-compatible-with-php8.0 branch from 6b63a53 to 87c72eb Compare September 10, 2021 10:21

ruflin reviewed Sep 13, 2021

View reviewed changes

reedy suggested changes Sep 22, 2021

View reviewed changes

This was referenced Sep 22, 2021

Allow PHP 8 in composer.json #1914

Closed

More Match class PHP 8 fun (in 6.x) #1978

Closed

ruflin closed this Sep 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

6.x compatible with php8.0 #1977

6.x compatible with php8.0 #1977

AskaoAhmedSaad commented Sep 10, 2021

ruflin left a comment

ruflin Sep 13, 2021

reedy Sep 22, 2021

reedy commented Sep 22, 2021 •

edited

Loading

reedy Sep 22, 2021

reedy Sep 22, 2021

reedy Sep 22, 2021

reedy Sep 22, 2021

reedy Sep 22, 2021

reedy Sep 22, 2021

reedy Sep 22, 2021

reedy Sep 22, 2021

reedy Sep 22, 2021

reedy Sep 22, 2021

reedy Sep 22, 2021

reedy commented Sep 22, 2021 •

edited

Loading

AskaoAhmedSaad commented Sep 22, 2021

reedy commented Sep 22, 2021

reedy commented Sep 23, 2021

reedy commented Sep 23, 2021 •

edited

Loading

ruflin commented Sep 23, 2021

6.x compatible with php8.0 #1977

6.x compatible with php8.0 #1977

Conversation

AskaoAhmedSaad commented Sep 10, 2021

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reedy commented Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reedy commented Sep 22, 2021 • edited Loading

AskaoAhmedSaad commented Sep 22, 2021

reedy commented Sep 22, 2021

reedy commented Sep 23, 2021

reedy commented Sep 23, 2021 • edited Loading

ruflin commented Sep 23, 2021

reedy commented Sep 22, 2021 •

edited

Loading

reedy commented Sep 22, 2021 •

edited

Loading

reedy commented Sep 23, 2021 •

edited

Loading