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

Support PHP 8.0 #1794

Merged
merged 4 commits into from
Oct 20, 2020
Merged

Support PHP 8.0 #1794

merged 4 commits into from
Oct 20, 2020

Conversation

deguif
Copy link
Collaborator

@deguif deguif commented Sep 12, 2020

Done in this PR:

  • Update composer constraint to allow PHP 8.0.
  • Update travis configuration in order to run tests on PHP 8.0 (nightly).
  • Update changelog

Done in previous PRs:

Done in external PRs:

  • Wait for aws/aws-sdk-php to be compatible with PHP 8. [https://github.com/Fixed crash on PHP 8.0 aws/aws-sdk-php#2079]
  • Wait for elasticsearch/elasticsearch to be compatible with PHP 8.0 and remove --ignore-platform-reqs composer option.

@deguif deguif force-pushed the php-8.0 branch 2 times, most recently from acae5c8 to a3b2289 Compare September 12, 2020 11:57
src/Util.php Outdated Show resolved Hide resolved
@deguif
Copy link
Collaborator Author

deguif commented Sep 12, 2020

One failure is remaining on PHP 8.0 but it's related to aws SDK.
See aws/aws-sdk-php#2079 for details and the fix that should be merged soon (I hope).

src/Query/Match.php Outdated Show resolved Hide resolved
@ruflin
Copy link
Owner

ruflin commented Sep 15, 2020

@deguif Thanks for getting this started. We had in the past a similar issue with a naming conflict with the Bool query. What we did back then is rename it to BoolQuery instead: https://github.com/ruflin/Elastica/blob/master/src/Query/BoolQuery.php I wonder if we should follow the same pattern here instead of using the _ for consistency? MatchQuery?

.travis.yml Outdated Show resolved Hide resolved
src/Query/Match.php Outdated Show resolved Hide resolved
tests/PipelineTest.php Outdated Show resolved Hide resolved
@ruflin
Copy link
Owner

ruflin commented Sep 15, 2020

@deguif This will also need an update of the CHANGELOG and I wonder if we should also update the readme that it now running at least tests with PHP 8.0?

@GrahamCampbell
Copy link

Bump aws/aws-sdk-php package version to fix a bug with PHP 8.0 (aws/aws-sdk-php#2079).

There are definitely more similar bugs like the one that I fixed. Amazon's job is to fix that, though. I have just fixed the one that affected me.

@GrahamCampbell
Copy link

Also, Guzzle, a dependency of the AWS SDK is not ready for PHP 8 yet.

@ruflin
Copy link
Owner

ruflin commented Sep 21, 2020

Is there option to exclude the AWS part for 8.0 for now?

@GrahamCampbell
Copy link

Guzzle is now ready, btw. 7.2.0 supports PHP 8.0.

ruflin pushed a commit that referenced this pull request Oct 20, 2020
This PR will allow to support PHP 8.0 in #1794 

It allows to run PHPUnit **8.5** for PHP **7.2** support, but also to run PHPUnit **9.4** for versions supporting it (7.3, 7.4, 8.0) as PHP **8.0** is not supported in PHPUnit **8.5**.

ClientConfiguration::fromDsn('test:0');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since PHP 8.0 RC1 a 0 port number is considered valid: See php/php-src#6152
For details see how it behaves on other PHP versions: https://3v4l.org/ZYhJc

@deguif
Copy link
Collaborator Author

deguif commented Oct 20, 2020

@ruflin I think this PR is now ready.
Testing on PHP 8.0 is now green with all the changes done in previous PRs.

I will later update the PHP version in travis configuration to 8.0 and also remove the composer flag --ignore-platform-reqs when elasticsearch/elasticsearch will officially support PHP 8.0.

@ruflin ruflin merged commit 8de1dbf into ruflin:master Oct 20, 2020
@ruflin
Copy link
Owner

ruflin commented Oct 20, 2020

@deguif Nice! Thank you!

@deguif deguif deleted the php-8.0 branch October 20, 2020 13:25
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

Successfully merging this pull request may close these issues.

5 participants