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

Replaced array_walk with array_map in Connection::getURI #1075

Conversation

pascaldevink
Copy link
Contributor

In Connection::getURI, array_walk was used together with passing the values as references, to change the original array.
Passing values as references is error-prone and discouraged for quite some time.
Also, when using in conjunction with PHP 8.0, it will fail.

array_map can do the same thing as the original array_walk implementation, but without the downsides of having side effects and having to pass values as references.

@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?

@mfn
Copy link

mfn commented Nov 16, 2020

This came up in #1063 (comment)

I took the changes in the PR and applied them to our local source tree in CI and can confirm that they work as intended!

@giovannialbero1992
Copy link
Contributor

Hi @mfn, can we cherry-pick this into #1063 ?

@mfn
Copy link

mfn commented Nov 17, 2020

@giovannialbero1992 🤷‍♀️

I see what you mean, didn't realize that #1063 targets master and this PR targets 7.10. Technically I don't see a reason not to.

However I'm not familiar with the process here, just a reported at this point :) I'm used to the multiple branch model that fixes will be applied to the lowest version (7.10 here) and then merged upstream into master anyway? But I think I'm not qualified to answer this, sorry.

@pascaldevink
Copy link
Contributor Author

@deguif @giovannialbero1992 PHP 8 was released today, is there any update on the progress of this PR?

@deguif
Copy link

deguif commented Dec 10, 2020

@ezimuel this one should probably be merged to fully support PHP 8 , as it fails otherwise on PHP 8

@deguif
Copy link

deguif commented Dec 10, 2020

@pascaldevink it should probably target 7.x, and would you able to rebase so that the test failure disappear as it seems it was fixed in this branch

@pascaldevink pascaldevink changed the base branch from 7.10 to 7.x December 15, 2020 14:02
@pascaldevink pascaldevink force-pushed the refactor_array_walk_to_array_map_in_connection branch from b5b6140 to cb80cdc Compare December 15, 2020 14:02
In Connection::getURI, array_walk was used together with passing the
values as references, to change the original array.
Passing values as references is error-prone and discouraged for quite
some time.
Also, when using in conjunction with PHP 8.0, it will fail.

array_map can do the same thing as the original array_walk
implementation, but without the downsides of having side effects and
having to pass values as references.
@pascaldevink pascaldevink force-pushed the refactor_array_walk_to_array_map_in_connection branch from cb80cdc to 80f0cf7 Compare December 15, 2020 14:03
@pascaldevink
Copy link
Contributor Author

@deguif @ezimuel rebased and updated the target against 7.x. The tests are succeeding again as well

* Added the x-elastic-client-meta header

* Removed @ExpectedException usage in PHPUnit

* Removed prestissimo plugin for composer in github action

* Added .phpunit.result.cache in .gitignore

* Add the t transport parameter in telemetry client header

* Fixed semver format for PHP version in client telemetry header
@ezimuel ezimuel merged commit b55e189 into elastic:7.x Dec 16, 2020
@ezimuel
Copy link
Contributor

ezimuel commented Dec 16, 2020

@pascaldevink actually, it was enough to remove the &$key reference in the previous array_walk in order to be compatible with PHP 8 but your code was better, thanks!

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.

7 participants