Allow php8 #18
Allow php8 #18
Conversation
|
I'm not sure it's ok to merge it since the lib elasticsearch/elasticsearch doesn't support it currently. |
|
You have right |
|
Looks like PR for es lib for PHP8 already exists elastic/elasticsearch-php#1063 |
|
yeah it doesn't look it's a top priority on elasticsearch side :/. Maybe if more people are complaining about this issue, it will go faster. In the meantime, I was looking to the CI status to see what it is failing. It looks like the code / the tests are a bit outdated. Do you want to refresh it ? (Would suggest to do it in another PR) |
|
I'm not familiar with Circle CI so I can help with test too much. Looks like now CI run only on PHP 7.2, right? |
|
yes that's right. But that's something you can try on your own environment I believe. After looking at the different issue, it seems it's because of the new version of PHP and also because of the new version of PHPUnit that causes the trouble. The setup of circleci is not really important at this point. I believe this is something quite easy to reproduce on a local environment :) |
|
ah looks like elasticsearch is supporting now php8. |
|
I will wait that elasticsearch is releasing a new version that supports php 8. And after I think we will be good |
|
@Nexucis elasticsearch is out https://github.com/elastic/elasticsearch-php/releases/tag/v7.11.0 |
|
\o/ nice. Do you want to finish this PR ? |
Codecov Report
@@ Coverage Diff @@
## master #18 +/- ##
============================================
+ Coverage 98.43% 98.86% +0.43%
+ Complexity 195 192 -3
============================================
Files 4 4
Lines 510 527 +17
============================================
+ Hits 502 521 +19
+ Misses 8 6 -2
Continue to review full report at Codecov.
|
|
@Nexucis finally works! :-) |
There was a problem hiding this comment.
Thanks a lot for the changes @snapshotpl !
I just added some comments, there are more questions that actually required changes.
| @@ -700,20 +702,14 @@ protected function existsAlias($alias) | |||
| 'name' => urlencode($alias) | |||
| ); | |||
|
|
|||
| $response = $this->client->indices()->existsAlias($params); | |||
|
|
|||
| if (is_array($response) && array_key_exists('status', $response)) { | |||
There was a problem hiding this comment.
a bit weird you removed this code and it is still working. Was it a dead code ? Or am I missing something new in ElasticSearch ?
There was a problem hiding this comment.
existsAlias always return bool, so it was a dead code
| { | ||
| // load static data | ||
| self::$documents = json_decode(file_get_contents('http://data.consumerfinance.gov/api/views.json')); | ||
| self::$documents = self::$documents === null |
There was a problem hiding this comment.
definitely better like that indeed. Thanks for changed that !
There was a problem hiding this comment.
Faster and stable! :-)
| class IndexActionTest extends AbstractIndexHelperTest | ||
| { | ||
| use AssertionRenames; |
There was a problem hiding this comment.
just for my knowledge, what does it do or why is it needed ?
There was a problem hiding this comment.
To support old phpunit for php 7.1 and 7.2
No description provided.