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.

Sign up for GitHub

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

Jump to bottom

[PHP 8.0] Deprecate Match query class and introduce MatchQuery #1799

Merged
merged 3 commits into from
Sep 28, 2020
Merged

[PHP 8.0] Deprecate Match query class and introduce MatchQuery #1799

merged 3 commits into from
Sep 28, 2020

Conversation

deguif
Copy link
Collaborator

@deguif deguif commented Sep 25, 2020 •
edited
Loading

See #1793

ruflin
ruflin approved these changes Sep 28, 2020
View reviewed changes
src/Query/Match.php
*/
class Match extends AbstractQuery
class Match extends MatchQuery
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@ruflin ruflin merged commit 66dc2ff into ruflin:master Sep 28, 2020
@ruflin
Copy link
Owner

ruflin commented Sep 28, 2020

@deguif This is great. Thanks!

@deguif deguif mentioned this pull request Sep 28, 2020
Closed
@deguif deguif deleted the php-8-match-query branch September 28, 2020 15:44
@deguif deguif mentioned this pull request Oct 19, 2020
Merged
8 tasks
ruflin pushed a commit that referenced this pull request Oct 20, 2020
@deguif
Support PHP 8.0 (#1794)
8de1dbf 
#### Done in this PR:

- [x] Update composer constraint to allow PHP 8.0.
- [x] Update travis configuration in order to run tests on PHP 8.0 (nightly).
- [x] Update changelog

#### Done in previous PRs:
- [x] Deprecate `Elastica\Query\Match` class and add a new `Elastica\Query\MatchQuery` class as match is now a reserved keyword. [#1799]
- [x] Update phpunit version in order to be compatible with PHP 8.0. [#1811]
- [x] Bump `aws/aws-sdk-php` package version to fix a bug with PHP 8.0. [#1798]

#### Done in external PRs:
- [x] Wait for `aws/aws-sdk-php` to be compatible with PHP 8. [aws/aws-sdk-php#2079]
- [ ] Wait for `elasticsearch/elasticsearch` to be compatible with PHP 8.0 and remove `--ignore-platform-reqs` composer option.
@mfn
Copy link
Contributor

mfn commented Nov 16, 2020

Is there any time table for a release containing this fix?

I'm starting to run our tests again PHP8 and hit the "parse error" as outlined.

Although I should be able to use master, it actually doesn't work for me though I think it should => any ideas?

composer require ruflin/elastica:7.0.x-dev --prefer-dist
./composer.json has been updated
Loading composer repositories with package information
Updating dependencies (including require-dev)
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - This package requires php ^8.0 but your PHP version (7.4.12) does not satisfy that requirement.


Installation failed, reverting ./composer.json to its original content.

I do run this with PHP 7.4.12 but according to this in master, it should work 🤔

Elastica/composer.json

Line 15 in 41ab9bf

"php": "^7.2 || ^8.0",

Thank you

@mfn
Copy link
Contributor

mfn commented Nov 16, 2020

PS: got it working by adding --ignore-platform-reqs to the composer command, still puzzles why it wouldn't work without it

@deguif
Copy link
Collaborator Author

deguif commented Nov 16, 2020

The behaviour you described is weird, as our CI jobs use several PHP 7.x versions and there are no issues like the one you encountered.
We also ignore platform requirements when testing on PHP 8 as the official elasticsearch library is not yet ready. (I see you commented on the PR on this repository too).

I think we should be able to publish a new 7.1.0 version in the coming days, but it would be better to wait for the elasticsearch official library to be officially compatible with PHP 8.0 before publishing the 7.1.0.

@reedy
Copy link
Contributor

reedy commented Nov 27, 2020

Not sure the best place to ask this (let me know if you want me to file an issue instead).. Is there any chance of getting this backported to the 6.x branch? Or some version also adding the MatchQuery class/alias also into 6.x so it can be used instead?

While presumably Elastica 6.x might not support PHP 8.0 (but 6.x is currently maintained based on the Readme), where a project can/will support multiple versions of elastica/elasticsearch php, being able to have the same class available in all versions would be really useful

Thanks!

@olix21
Copy link

olix21 commented Nov 30, 2020

@reedy It could be interesting to open a dedicated issue/MR for that

@reedy
Copy link
Contributor

reedy commented Nov 30, 2020

Will do!

This was referenced Nov 30, 2020
Closed
Closed
@reedy reedy mentioned this pull request Feb 11, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruflin ruflin ruflin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@deguif @ruflin @mfn @reedy @olix21