Up to elasticsearch 8

[pawelkeska]

I have marked people from PR 2168 because i'd like to continue upgrade to ES8.

Please verify my changes. Now we've got one transport from Elastica and client has been extended by endpoints trait from elasticsearch library. I had to correct some tests.

I didin't use transport from elasticsearch lib because it would require a large number of changes and I marked it as not supported and I think this change will be too big.

@ruflin @VincentLanglet @oleg-andreyev @coreation

[coreation]

Great work @pawelkeska ! I thought refactoring the Client would've been much more work... What I find a bit difficult is to follow the flows because there's little to no typehinting in the function parameters or return statements so I'll have to ask :D How is for example the Client->index($params) returning an Elastica Response object? I can't seem to find the index() function on the Client and there's no typehinting to go on...

I'm just trying to understand the changes, but aside from that, thanks so much for the contribution @pawelkeska !
Is this a complete PR or are there still things you need help with?

[sidz]

It looks promissing @pawelkeska. Thanks for your efforts.

My 2 cents:
Can't we use elasticsearch/elasticsearch a bit lower version ? ClientInterface was added since 8.4: https://github.com/elastic/elasticsearch-php/releases/tag/v8.4.0 .

So I'd suggest to set minimum version equal to 8.4

[pawelkeska]

Up.
Rebuild transport.
From now we have only one transport from elasticsearch lib. It required a lot of changes but client has all funcionality like elasticsearch client.
@coreation now typehinting is fixed and i think it'is complete PR, In the future i will rebuild only this
@sidz it seems to me we can use but i have to check this.

[ruflin]

@pawelkeska Thansk for picking this up. Trying to find the time to give you a detailed review.

[pawelkeska]

@ruflin i fixed tests percentiles because above elasticsearch 8.9 version there are a few changes.

Some information about percentile calculations is here

[pawelkeska]

Up.
@ruflin i've removed elasticsearch response from ResultSet now i don't need Guzzle response in MultiBuilder.

[ruflin]

Thank you @pawelkeska for the massive amount of work you put into this! I like the direction this is taking. There will be breaking changes for users going to 8 but there is no way to prevent it. Also looking at the tests, the changes users have to do are less then I expected. There are 2-3 changes where I wonder if we could with low effort make the migration easier. Could we for example "wrap" Elasticsearch object into the old Response object so the Response object would stay the same? The second is around asArray() -> getData(), could getData() be a proxy for asArray() meaning users would have to change less code? Have not played with the code directly to see if it would be feasible.

Seems like along the way, you also cleaned up some of the code, thanks! One nice thing would be having a very short write up on how users would have to migrate from 7.x to 8.x. What do 80% of the users have to change to be successful and get things working quickly? The tests already show part of this pretty well but it would be nice to have it summarised.

The most important thing at the moment is I think getting a version out of the door (even beta) that works with 8.x. We can add more "convenience" functions later on that help with migration if many users bump into it. @pawelkeska How close are we to have such a first version?

[ruflin]

I guess this is not available anymore?

[pawelkeska]

Yes, i have to remove property $_queryTime from Response object.

[ruflin]

Is there a way to at keep this method at least with the parameters? I expect this to be used a lot.

[pawelkeska]

Yes, of course we could keep this method but i think users should start using actions from trait ClientEndpointsTrait. This gives them greater consistency and benefits in further maintaining the application.

[ruflin]

This was for debugging purpose if I remember correctly. Not compatible anymore?

[pawelkeska]

yes, it was used in utils test function testConvertRequestToCurlCommand but this function was not used anywhere in the tests.

[ruflin]

If we would have a wrapper around result, get we keep getData as a proxy for asArray?

[pawelkeska]

We need to think about it, I explained everything in the comment.

[pawelkeska]

Thanks @ruflin

Yes, we can "wrap" Elasticsearch by old object Response but only Elastica actions like "updateDocument", "addDocuments" etc
From now we use in our client trait ClientEndpointsTrait and in this trait all actions have return Elasticsearch i dont't know why they don't rely on interface (I guess this library is automatically generated). If we wrap Elasticsearch object in sendRequest function typehinting for actions from ClientEndpointsTrait will point to the wrong object. Maybe it's a good idea have old Response object for Elastica actions and for actions from trait ClientEndpointsTrait response will be Elasticsearch object. What do you think @ruflin ? Now, we use old Response object only for Bulk and Multi ResultSet class. For Elasticsearch object response i created helpers classes ResponseChecker and ResponseParser which have some functions from Response object.

In my free time, I will try to write in changelog about what has changed.

After making a decision, what do we do with Response object i think we can release the first beta version.

[ruflin]

If we wrap Elasticsearch object in sendRequest function typehinting for actions from ClientEndpointsTrait will point to the wrong object. Maybe it's a good idea have old Response object for Elastica actions and for actions from trait ClientEndpointsTrait response will be Elasticsearch object. What do you think @ruflin

I like the idea but I don't know how much work it is. The main reason I would wrap it is for migration purposes. How much work does it take for a user migration from 7.x to 8.x? If we save everyone with this changes a few minutes and troubles searching for the solution, it sums up quickly and creates happier devs :-) But if it makes our code much more complex and hard to maintain, it is not worth it. It's all a trade off. @pawelkeska Can you anticpate how much work it would be to do the "wrapper" approach for response?

I could also see us releasing a first version as alpha without the response wrapper and actively ask for feedback from first users (not sure how, maybe through the changelog) on how hard the migration was for them and what they would like to be changed / made easier. If everyone is happy, the additional code might not be needed. At the same time, I have a strong feeling it will help many users.

[limenet]

@ruflin @pawelkeska

I could also see us releasing a first version as alpha without the response wrapper and actively ask for feedback from first users (not sure how, maybe through the changelog) on how hard the migration was for them and what they would like to be changed / made easier.

We - @pryserv and I from valantic - make heavy use of Elastica through valantic/pimcore-elastica-bridge and would be more than happy to test an alpha version, provide feedback, and contribute code.

What would you like us to test, what would be the best approach for us to contribute and help getting support for Elasticsearch 8 in a stable version of Elastica?

[ruflin]

@limenet I would really appreciate yours and @pryserv help on this. Step one to get us moving forward is getting this PR over the line and then go for the testing. Please have a look at the code and discussions and provide your opinions / insights. There is also some failing tests on this PR, could help fix these. I guess you will not have permissions to push to the branch but open a PR against the branch and we can get it in quickly?

[pawelkeska]

@ruflin @limenet up

Fix all tests and CS/DX.
"Wrap" elasticsearch response, from now we have old Response object for Elastica actions and for actions from trait ClientEndpointsTrait response will be Elasticsearch object (helper class has been created to convert elasticsearch response ResponseConverter)

@ruflin we can release an alpha version but someone has to write the changelog.

[limenet]

@ruflin @pawelkeska Thank you! I'm working on valantic/pimcore-elastica-bridge#55 and upgrading a Pimcore project to Pimcore 11 and Elasticsearch 8. If/when we encounter issues during testing, I'll try to fix the issue and submit a PR or comment here, if that works for you?

Please have a look at the code and discussions and provide your opinions / insights. There is also some failing tests on this PR, could help fix these. I guess you will not have permissions to push to the branch but open a PR against the branch and we can get it in quickly?

This sounds like a good plan and I'll try to get to it this/next week.

[ruflin]

@pawelkeska Thanks for the very quick turnaround. I can think of three options moving forward:

• Merge this into 8.x and continue development from there
• Keep it open for 1-2 more weeks and have @limenet doing testing on the branch and contribute PR's with fixes
• Merge into a branch 8.x-alpha

The only reason I would do 8.x-alpha is in case some users are running based on 8.x and this would break their code, at the same time what they have with 8.x would not work against 8.x properly anyways. If we keep the branch open, it causes work fro @pawelkeska to merge PR's instead of doing the work on my side. So I'm leaning towards option 1, get this in and take it from there. What we should do on top of Option 1 is likely update the README to indicate that the current branch is no stable yet. Thoughts?

[ruflin]

@pawelkeska @limenet Lets make a quick call on merging this branch. I'm proposing to get it in as is and take it from there. Any concerns?

[sidz]

Just would like to highlight #2181 (comment) one more time. It's okay to do it in a separate PR but before any releases.

[limenet]

@ruflin agree on merging this branch into 8.x which also simplifies downstream composer.json when testing the "alpha" version.

[ruflin]

I created the following Github issue to track all the work and also took the comment from @sidz so we don't forget: #2185 Going to hit merge on this PR now!

[ruflin]

PR is in! Lets continue with follow up PR's / testing etc.

[ruflin]

@pawelkeska THANK YOU for all the effort into this. Also thanks to everyone that contributed code / discussion / ideas to get to this point!

[coreation]

@pawelkeska amazing work, really!

[pawelkeska]

@ruflin Thanks, I'm glad I could help.

[pawelkeska]

@limenet i saw your changes in your repo. From now we dont't have request in Client because we use ClientEndpointsTrait

You should use functions from this trait like that $this->esClient->indices()->existsAlias(['name'=>$indexConfig->getName()]) in line Index.php#L232

[limenet]

@pawelkeska thank you for checking out my changes!

Other than PHPStan being unhappy about the return type of Elasticsearch|Promise by \Elastic\Elasticsearch\ClientInterface::sendRequest, I haven't been able to find any issues yet. Testing on our side will start in the next couple weeks and I'll follow up with PRs / issues.

[pawelkeska]

@limenet In our client we have return type Elasticsearch|Promise and all endpotints in elasticsearch-php library have return type Elasticsearch|Promise i don't know why your PHPStan show wrong type and all return types should be correct.

[limenet]

@pawelkeska the return types are correct but when I run PHPStan (level 8) on:

$this->esClient->indices()->existsAlias(['name' => $indexConfig->getName()])->asBool()

I get

Call to an undefined method Elastic\Elasticsearch\Response\Elasticsearch|Http\Promise\Promise::asBool().

because asBool() is only present in Elasticsearch and not in Promise. This is not something caused by Elastica, but might be worth mentioning in an upgrade guide nevertheless.

[pawelkeska]

@limenet Yeap, you are right but we don't use Promise in our Elastica client, but this does not change the fact that we need to check whether a given function exists. The only solution will be use null safe operator ?->.

($this->esClient->indices()->existsAlias(['name' => $indexConfig->getName()]))?->asBool()