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

PHP 7.4 compatibility fix / implode argument order #346

Merged
merged 2 commits into from Sep 16, 2019
Merged

PHP 7.4 compatibility fix / implode argument order #346

merged 2 commits into from Sep 16, 2019

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Jul 21, 2019

implode() takes two parameters, $glue and $pieces.
For historical reasons, implode() accepted these parameters in either order, though it was recommended to use the documented argument order of implode( $glue, $pieces ).

PHP 7.4 is slated to deprecate the tolerance for passing the parameters for implode() in reverse order.
PHP 8.0 is expected to remove the tolerance for this completely.

Refs:

`implode()` takes two parameters, `$glue` and `$pieces`.
For historical reasons, `implode()` accepted these parameters in either order, though it was recommended to use the documented argument order of `implode( $glue, $pieces )`.

PHP 7.4 is slated to deprecate the tolerance for passing the parameters for `implode()` in reverse order.
PHP 8.0 is expected to remove the tolerance for this completely.

Refs:
* https://wiki.php.net/rfc/deprecations_php_7_4#implode_parameter_order_mix
* https://php.net/manual/en/function.implode.php
@jrfnl
Copy link
Member Author

jrfnl commented Jul 21, 2019

The Travis failure seems unrelated to this PR. Looks like the DevOps site of this repo needs a refresh.

@jrfnl
Copy link
Member Author

jrfnl commented Sep 13, 2019

Anything I can do to move this patch forward ?

WordPress 5.3 should include this patch, so it would be great if it could be merged and a new version of Requests tagged before WP 5.3 goes into RC....

@ozh
Copy link
Collaborator

ozh commented Sep 15, 2019

@rmccue can we consider removing the mandatory tests? Some patches are fairly trivial but tests always fail anyway ...

@jrfnl
Copy link
Member Author

jrfnl commented Sep 15, 2019

can we consider removing the mandatory tests? Some patches are fairly trivial but tests always fail anyway ...

Wouldn't fixing them be more effective ?

@rmccue
Copy link
Collaborator

rmccue commented Sep 15, 2019

See #351 for the tests.

@jrfnl
Copy link
Member Author

jrfnl commented Sep 15, 2019

I'll happily rebase this PR once #351 is merged to show the tests passing. Just ping me.

@rmccue
Copy link
Collaborator

rmccue commented Sep 15, 2019

#351 is merged now, so should be good to merge in here.

(If you could, please merge rather than rebase, thanks!)

@jrfnl
Copy link
Member Author

jrfnl commented Sep 16, 2019

(If you could, please merge rather than rebase, thanks!)

I will if you insist, but I honestly don't understand why you'd want that ?
https://blog.developer.atlassian.com/stop-foxtrots-now/

@rmccue
Copy link
Collaborator

rmccue commented Sep 16, 2019

I prefer preserving the history. :)

@jrfnl
Copy link
Member Author

jrfnl commented Sep 16, 2019

@rmccue But there is no history to preserve... all a normal rebase does is re-attach my branch at the head of the current master, the commit stays the same (including commit date etc).

@codecov-io
Copy link

Codecov Report

Merging #346 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #346   +/-   ##
=========================================
  Coverage     92.11%   92.11%           
  Complexity      760      760           
=========================================
  Files            21       21           
  Lines          1762     1762           
=========================================
  Hits           1623     1623           
  Misses          139      139
Impacted Files Coverage Δ Complexity Δ
library/Requests/Transport/fsockopen.php 94.18% <100%> (ø) 68 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e4fe0eb...2aced35. Read the comment docs.

Copy link
Collaborator

@rmccue rmccue left a comment

Choose a reason for hiding this comment

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

Patch looks good; I've restarted the failing tests as they appear to be transient errors (again; not sure why these are more frequent these days...)

@jrfnl
Copy link
Member Author

jrfnl commented Sep 16, 2019

@rmccue Could you (selectively) do that again ? Builds failing again on unrelated issues.

@jrfnl
Copy link
Member Author

jrfnl commented Sep 16, 2019

@rmccue Two more to restart (again)....

@rmccue
Copy link
Collaborator

rmccue commented Sep 16, 2019

Yep, restarting them in smaller blocks, as I think the issue is with the tests server not supporting enough concurrency. All looking good so far...

@rmccue rmccue merged commit bb04061 into WordPress:master Sep 16, 2019
@rmccue
Copy link
Collaborator

rmccue commented Sep 16, 2019

Phew, finally, thanks so much for this! We should add 7.3 and 7.4 to the testing matrix as well as non-failing.

@jrfnl
Copy link
Member Author

jrfnl commented Sep 16, 2019

We should add 7.3 and 7.4 to the testing matrix as well as non-failing.

@rmccue I've just prepared a PR for that and am waiting for a passing build to pull it ;-)

@jrfnl jrfnl deleted the feature/php-7.4-compatibility-implode branch September 16, 2019 14:27
@jrfnl jrfnl added this to the 1.7.1 milestone Sep 17, 2019
@jrfnl jrfnl mentioned this pull request Apr 18, 2021
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.

None yet

4 participants