Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests broken since release of elastic/elasticsearch-php 7.4.0 #1710

Closed
deguif opened this issue Nov 19, 2019 · 16 comments
Closed

Tests broken since release of elastic/elasticsearch-php 7.4.0 #1710

deguif opened this issue Nov 19, 2019 · 16 comments

Comments

@deguif
Copy link
Collaborator

deguif commented Nov 19, 2019

I investigated the issue and it seems some classes are now auto-generated, and their name was changed.

For example, Elasticsearch\Endpoints\Ingest\Pipeline\Delete was renamed to Elasticsearch\Endpoints\Ingest\DeletePipeline

@deguif
Copy link
Collaborator Author

deguif commented Nov 19, 2019

@ruflin @thePanz a quick solution would be to update composer.json to avoid installing this version.

But I'm not sure what will be the best way to support 7.4 version and all 7.x versions.
Maybe we should create an issue on their repository.

@deguif
Copy link
Collaborator Author

deguif commented Nov 19, 2019

#1711 should fix the tests, by marking the elasticsearch package as conflicting since version 7.4.0 in the composer.json file.

@thePanz
Copy link
Collaborator

thePanz commented Nov 19, 2019

A better solution would be to set the version range to something like ^7.0 || < 7.4.0
So that we could work on the issue with more time. wdyt?

@deguif
Copy link
Collaborator Author

deguif commented Nov 19, 2019

@thePanz I'm not sure the || will do what is expected here.
As 7.4.0 should match ^7.0, but we should probably use ^7.0 < 7.4.0 as it would be an and in this case.
I was thinking the conflict was more appropriate, but it's up to you ;)

@deguif
Copy link
Collaborator Author

deguif commented Nov 19, 2019

I confirm ^7.0 || <7.4.0 is not working while ^7.0 <7.4.0 is working as expected.

@deguif
Copy link
Collaborator Author

deguif commented Nov 19, 2019

I also created an issue on the official elastichsearch-php repository, let's see what will come out.

@ezimuel
Copy link
Collaborator

ezimuel commented Nov 20, 2019

I just commented about this issue here.

@ruflin
Copy link
Owner

ruflin commented Nov 20, 2019

For having consitent testing, +1 on fixing the dependency version (already done). I would hope somewhere in the future, releases of the two libraries are even in more sync.

At the same time I'm kind of glad we found this now os testing with "latest" is not only bad. I suggest we make the change on our end before we ship 7.0 GA. As far as I understand it mainly changes some internal implementation but nothing external?

@ezimuel
Copy link
Collaborator

ezimuel commented Nov 20, 2019

@ruflin exactly, I'm doing my best to avoid BC breaks but sometime it's quite hard to keep all previous inconsistency in place. I'm working on elasticsearch-php 7.4.1 to reintroduce (some) old namespaces as class aliases.
Now that we have a code generation from the REST API specification we'll be more aligned with Elasticsearch version.

@ruflin
Copy link
Owner

ruflin commented Nov 25, 2019

@ezimuel Don't worry, fully understand that sometimes breaking changes in minors are needed ;-) Do you think we should directly rely on 7.4.1 and it will make things less breaking on our end?

@ezimuel
Copy link
Collaborator

ezimuel commented Nov 25, 2019

@ruflin Yes, I'm releasing 7.4.1 soon with all the unexpected BC breaks. In the mean time, can you test elastic/elasticsearch-php#968 and let me know if there are BC breaks in Elastica?

I also tried to explain here the motivation behind the two potential BC breaks added intentionally in 7.4.0.

@deguif
Copy link
Collaborator Author

deguif commented Nov 25, 2019

I just tested the test suite with elastic/elasticsearch-php#968 and there are 2 remaining errors related to class renaming. See issue on elastic/elasticsearch-php#967 for more details.

@deguif
Copy link
Collaborator Author

deguif commented Nov 25, 2019

Everything should be fine with the release of elastic/elasticsearch-php 7.4.1
I created PR #1715 to exclude version 7.4.0 but let greater version be installed.

@ezimuel
Copy link
Collaborator

ezimuel commented Nov 26, 2019

I just released 7.4.1 that solves the BC break related to the endpoint class naming and it solves also the trigger_error() issue https://github.com/babenkoivan/scout-elasticsearch-driver/issues/297 using the suppress error @ operator.

@deguif
Copy link
Collaborator Author

deguif commented Nov 26, 2019

Thanks @ezimuel

@deguif
Copy link
Collaborator Author

deguif commented Nov 30, 2019

I'm closing as all tests are now green.

@deguif deguif closed this as completed Nov 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants