WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 10 days ago

Last modified 2 days ago

#47746 closed defect (bug) (fixed)

PHP 7.4 compatibility fix / implode argument order

Reported by: jrf Owned by: desrosj
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch php74 has-dev-note
Focuses: coding-standards Cc:
PR Number:

Description

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:

Related:

Note: join() is an alias of implode(). It is generally considered best practice to use the canonical function rather than an alias, however, that is outside the scope of this ticket.

Attachments (5)

47746-fix-implode-argument-order.patch (3.4 KB) - added by jrf 3 months ago.
47746.diff (970 bytes) - added by jorbin 2 months ago.
47746-patch-up-SimplePie.patch (987 bytes) - added by jrf 3 weeks ago.
47746-simplepie-1-5-3.diff (157.6 KB) - added by desrosj 3 weeks ago.
requests-1-7-3470169.diff (1.3 KB) - added by desrosj 3 weeks ago.

Download all attachments as: .zip

Change History (31)

#1 @jrf
3 months ago

FYI: I have reviewed ALL function calls to implode() and join() to get at this patch. Based on the current trunk, there are no remaining issues.

#2 follow-up: @johnbillion
3 months ago

  • Version trunk deleted

The changes to Requests and Simplepie need to be made upstream to their respective repos.

The other two changes look good.

#3 in reply to: ↑ 2 @jrf
3 months ago

Replying to johnbillion:

The changes to Requests and Simplepie need to be made upstream to their respective repos.

The other two changes look good.

Pull requests to both upstream repos have already been opened (see links above), but neither is very actively maintained.

The Requests dependency in Core is at the latest version - 1.7 -, but that version was released in October 2016 (!!!).
Ref: https://github.com/rmccue/Requests/releases

The Requests repo has had no commits since December 2017 and only a few, mostly DevOps related ones since the last release.

The SimplePie dependency in Core is at version 1.3.1 - which was released in October 2012 😰, while the current version is 1.5.2 released in August 2018.
Ref: https://github.com/simplepie/simplepie/releases

And while the SimplePie repo has actually had some commits this year (February), the version used in WP Core is pretty far behind the latest release.

So, yes, while I agree that the fixes should primarily be made upstream, I don't think WP should be throwing deprecation notices when run on PHP 7.4 while we are waiting for
1) new releases from upstream and
2) the existing WP Core versions of those libraries to be updated.

@jorbin
2 months ago

#4 @jrf
2 months ago

  • Keywords php74 added

#5 @jrf
4 weeks ago

FYI:

The patch for Core looks good and should IMO be committed ASAP.

#6 @desrosj
4 weeks ago

  • Keywords commit added
  • Owner set to desrosj
  • Status changed from new to assigned

#7 @desrosj
4 weeks ago

In 46155:

General: Ensure the arguments passed to implode() are in the correct order.

The implode() function accepts two. parameters, $glue and $pieces. For historical reasons, these parameters have been accepted in any order, though it was recommended that the documented order of $glue, $pieces be used.

Starting in PHP 7.4, specifying the parameters in the reverse order will trigger a deprecation notice with the plan to remove this tolerance in PHP 8.0.

This change fixes the occurrences of reversed arguments in Core with the exception of those contained in included external libraries. These will be handled separately.

Props jrf, jorbin.
See #47746.

#8 @desrosj
4 weeks ago

  • Keywords close added; commit removed

I'm leaving this open for now, but it may be best to tackle the remaining issues in separate tickets to update the two libraries.

I know @jrf has been working with @rmccue to get a new version of Requests released, but it does not seem SimplePie has seen any traction.

#9 @jrf
4 weeks ago

New issue introduced in: [46175]

New patch added to #36824 as that's the ticket which introduced it.

#10 @SergeyBiryukov
4 weeks ago

In 46182:

Tests: Replace join() with implode() in do_enclose() tests introduced in [46175], and ensure the arguments passed are in the correct order.

The implode() function accepts two parameters, $glue and $pieces. For historical reasons, these parameters have been accepted in any order, though it was recommended that the documented order of $glue, $pieces be used. It is also generally considered best practice to use the canonical function rather than an alias.

Starting in PHP 7.4, specifying the parameters in the reverse order will trigger a deprecation notice with the plan to remove this tolerance in PHP 8.0.

Props jrf.
Fixes #36824. See #47746.

#11 @jrf
3 weeks ago

Status summary:

  • Issues in WP Core native code have been fixed.
  • Issues in the external Requests and SimplePie libraries remain unpatched at this moment.

Regarding SimplePie:

  • Ticket #36669 is open to address this, but has just been punted to Future Release.
  • The issue has been patched upstream, but is not contained in a tagged release yet.
  • If SimplePie does not get updated to a more recent version in WP 5.3, the existing code should be patched for the time being. I will attach a patch for this.

Regarding Requests:

  • The issue has been patched upstream (above mentioned PR has been merged in the mean time), but is not contained in a tagged release yet.
  • Still hoping for a new tagged release over the next few days to go into WP 5.3.

#12 @jrf
3 weeks ago

SimplePie has released a new version including the changes needed for PHP 7.4 support: https://github.com/simplepie/simplepie/releases/tag/1.5.3

#13 @desrosj
3 weeks ago

  • Keywords needs-testing added; close removed

47746-simplepie-1-5-3.diff updates SimplePie to version 1.5.3.

Here is a test build on my fork: https://travis-ci.org/desrosj/wordpress-develop/builds/588479168

#14 @desrosj
3 weeks ago

Since there has been no tagged release for Requests, requests-1-7-3470169.diff updates Requests to version 1.7.3470169, which corresponds to the most recent commit.

#15 @jrf
3 weeks ago

@desrosj 👍🏻

FYI: Just checked with @rmccue regarding Requests and the intention is still to try and get a new release tagged over the next week.

#16 @stevenkword
3 weeks ago

Safety first with regards to SimplePie. It's so late in the 5.3 cycle that I don't feel comfortable making a decision at this point. Please follow the related ticket, #36669, where we will introduce the library update in the next development cycle to ensure proper time to validate compatibility among plugin authors.

This ticket was mentioned in Slack in #core by ocean90. View the logs.


3 weeks ago

#18 @desrosj
3 weeks ago

Let's move forward with 47746-patch-up-SimplePie.patch and requests-1-7-3470169.diff with the intention to fully update SimplePie through #36669 in 5.4, and when Requests is officially tagged the version string can be changed during beta.

#19 @iseulde
3 weeks ago

In 46257:

r46253: Use implode instead of join, see #47746

#20 @desrosj
3 weeks ago

In 46258:

External Libraries: Fix PHP 7.4 compatibility issues in the Requests library.

See: https://github.com/rmccue/Requests/compare/v1.7.0...3470169

Props jrf, jorbin, desrosj.
See #47746.

#21 @desrosj
3 weeks ago

In 46260:

External Libraries: Fix PHP 7.4 compatibility issues in the SimplePie library.

See: https://github.com/simplepie/simplepie/commit/38b504969ed08903cb12718e8270263a8c93080e

Props jrf, stevenkword, jorbin, desrosj.
See #47746.

#22 @ayeshrajans
3 weeks ago

It looks like SimplePie library has also cut a new release with this fix 👍🏽.

#23 @jrf
3 weeks ago

  • Keywords close added; needs-testing removed

@iseulde Thanks for catching that newly introduced issue straight away ;-)

@desrosj Thanks for committing these last two patches.

I think we can close this ticket now as the current code base should be clean of the issue now.
Fingers crossed 🤞🏻 no further new issues of this type will be introduced.

A new issue can/should probably be created to update Requests to the newly tagged version once released.

#24 @desrosj
10 days ago

  • Keywords close removed
  • Resolution set to fixed
  • Status changed from assigned to closed

Thanks, everyone for the work on this!

#25 @jorbin
7 days ago

In 46416:

External Libraries: Fix PHP 7.4 compatibility issue in the Requests library.

Moves https://github.com/rmccue/Requests/pull/370 into WordPress.

Previous [46258].

See #47746.

Note: See TracTickets for help on using tickets.