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

Replace trait with abstract class to avoid Deprecated Functionality issue in PHP 8.1 #1175

Merged

Conversation

karyna-t
Copy link
Contributor

@karyna-t karyna-t commented Nov 5, 2021

In PHP 8.1, Calling a static method directly on a trait is deprecated.
(see https://www.php.net/manual/en/migration81.deprecated.php#migration81.deprecated.core.static-trait)

Because of this, we receive such a deprecation notice when running our code with PHP8.1:
Deprecated Functionality: Calling static trait method Elasticsearch\Namespaces\BooleanRequestWrapper::performRequest is deprecated, it should only be called on a class using the trait

The simplest solution is to replace trait with abstract class.
We also can make performRequest function non-static, but there will be naming conflicts (classes that use it already have the function with the same name). Thus there will more BIC changes.

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@karyna-t
Copy link
Contributor Author

Hi @ezimuel @philkra @jrodewig , may I ask someone to check this issue? This problem is quite critical for us.

@ezimuel
Copy link
Contributor

ezimuel commented Nov 16, 2021

@karyna-tsymbal-atwix thanks for the PR. I'll review it asap. In the meantime, I merged your PR at ezimuel/ringphp#4 and released ezimuel/ringphp 1.2.0, thanks also for this other contribution.

@ezimuel ezimuel self-assigned this Nov 16, 2021
@ezimuel ezimuel added this to the 7.16.0 milestone Nov 16, 2021
@andrewbess
Copy link

Hello @ezimuel
Should we fix src/Elasticsearch/Endpoints/Ml/PostData.php that breaks builds in this PR?
Thank you in advance for your answer.

@karyna-t
Copy link
Contributor Author

Hi @ezimuel , PHP 8.1 is released, so we'd really appreciate it if you continue reviewing this PR 🙏

regarding this file where there was an issue in builds: src/Elasticsearch/Endpoints/Ml/PostData.php
It's not related to my changes but I fixed it in the scope of this PR (as there is a really minor change there).
cc @andrewbess

@andrewbess
Copy link

Hello @ezimuel
The PHP 8.1 has been released.
Making a compatibility of our project with PHP 8.1 relates from your extension.
Could you please share your plans with releasing new version of your extension with us?
Thank you in advance.

@ezimuel
Copy link
Contributor

ezimuel commented Dec 3, 2021

@andrewbess, @karyna-tsymbal-atwix sorry for my late reply. I'm going to review and merge this PR asap, hopefully next week. The goal is to release it with 7.16.0.

@xmav
Copy link

xmav commented Dec 6, 2021

@ezimuel Could you please advice scheduled release date for 7.16.0 ?

@ezimuel ezimuel merged commit 26a902d into elastic:master Dec 7, 2021
@ezimuel
Copy link
Contributor

ezimuel commented Dec 7, 2021

@karyna-tsymbal-atwix thanks for this PR, I just merged.

@ezimuel
Copy link
Contributor

ezimuel commented Dec 7, 2021

@xmav I'm going to release 7.16.0 this week.

@andrewbess
Copy link

@karyna-tsymbal-atwix thanks for this PR, I just merged.

Hello @ezimuel
Thank you for the fast reaction

ezimuel pushed a commit that referenced this pull request Dec 7, 2021
…ssue in PHP 8.1 (#1175)

* Replace trait with abstract class to avoid deprecation functionality issue

* fix phpcs fail
@ezimuel
Copy link
Contributor

ezimuel commented Dec 9, 2021

Just released elasticsearch-php 7.16.0 including the fix for PHP 8.1. Thanks again to @karyna-tsymbal-atwix for the PR!

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