Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#48312 closed defect (bug) (fixed)

PHP4 notation for passing object by reference broken

Reported by: davidbinda's profile david.binda Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.3 Priority: normal
Severity: normal Version: 5.3
Component: Plugins Keywords: has-patch commit dev-reviewed
Focuses: Cc:

Description

This is quite weird situation, since WordPress has already dropped support for PHP < 5.6.20, the r46149 is removing support for PHP 4 way of passing in object as references:

if ( is_array( $arg ) && 1 == count( $arg ) && isset( $arg[0] ) && is_object( $arg[0] ) ) { // array(&$this) {
    $args[] =& $arg[0];
}

which may be breaking code, which works in PHP4 and due to the compat logic in do_action ( introduced in r4177 ), still works nowadays. Eg.:

do_action( 'some_action', array( $this ) );

It's true that in PHP 5 all objects are being passed as references, so the array wrapping is no longer needed, but in case there is a code using the old notation (see above), the args are not really being passed to the callback in an expected way, eventually producing issues in the callbacks.

I'm attaching a phpunit test simulating the faulty behaviour, which was/is working before r46149.

Attachments (2)

48312.diff (828 bytes) - added by david.binda 5 years ago.
48312-fix-and-test.patch (1.8 KB) - added by jrf 5 years ago.
do_action(): Fix PHP-4 style passing of objects not longer broken out of array, includes updated unit test.

Download all attachments as: .zip

Change History (20)

@david.binda
5 years ago

#1 @SergeyBiryukov
5 years ago

  • Component changed from General to Plugins
  • Milestone changed from Awaiting Review to 5.3

#2 in reply to: ↑ description @SergeyBiryukov
5 years ago

Replying to david.binda:

It's true that in PHP 5 all objects are being passed as references, so the array wrapping is no longer needed, but in case there is a code using the old notation (see above), the args are not really being passed to the callback in an expected way, eventually producing issues in the callbacks.

Thanks for bringing this up! Has this come up in a project with some legacy code?

I haven't seen any other reports in the 4 weeks since [46149] was committed, however maybe it would be safer to revert [46149] for now and re-commit early in 5.4 to allow for more testing and feedback?

@jrf, what do you think?

#3 follow-up: @SergeyBiryukov
5 years ago

Another option would be to leave it as is for now and see if any other reports show up during RC or after the release.

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


5 years ago

#5 in reply to: ↑ 3 ; follow-up: @SergeyBiryukov
5 years ago

  • Milestone changed from 5.3 to Future Release

Replying to SergeyBiryukov:

Another option would be to leave it as is for now and see if any other reports show up during RC or after the release.

Let's do that, this seems like an edge case that could be fixed in a minor release if necessary.

#6 in reply to: ↑ 5 @azaozz
5 years ago

Replying to SergeyBiryukov:

Let's do that, this seems like an edge case that could be fixed in a minor release if necessary.

Right. That's the best decision under the circumstances. As a regression, it can also be fixed during RC if necessary.

#7 follow-up: @jrf
5 years ago

@SergeyBiryukov @davidbinda Thanks for pinging me.

the array wrapping is no longer needed, but in case there is a code using the old notation (see above), the args are not really being passed to the callback in an expected way, eventually producing issues in the callbacks.

I do see the problem and you're correct that it's the array wrapping not being removed which is causing it.

I really would not recommend reverting the patch as it removed a lot of redundancy.

I'm attaching a patch which fixes this specific use-case in a simple way by just bringing that one tiny bit back.

The patch also updates the unit test kindly provided by @davidbinda:

  1. The point of the removed code was to support objects passed by reference and the unit test was not passing the object by reference.
  2. The unit test was using short arrays which are a) not allowed CS-wise and b) PHP 5.4+ code which would in a way invalid the test.
  3. The point of the passing by reference is that the object passing to the functions attached to the action is the same object and not a copy (PHP4), so using assertSame() actually tests that, while the previously used assertEquals() did not.

Let's do that, this seems like an edge case that could be fixed in a minor release if necessary.

@SergeyBiryukov I'll leave it up to your judgement whether to commit this patch now or later.

I do think this patch is safe and that @davidbinda has a valid point about the changed behaviour.

I've got a build running now to make sure that all tests pass: https://travis-ci.org/jrfnl/wordpress-develop/builds/598323636

@jrf
5 years ago

do_action(): Fix PHP-4 style passing of objects not longer broken out of array, includes updated unit test.

#8 in reply to: ↑ 7 @SergeyBiryukov
5 years ago

  • Keywords has-patch commit added
  • Milestone changed from Future Release to 5.3

Replying to jrf:

I do think this patch is safe and that @davidbinda has a valid point about the changed behaviour.

Thanks for the patch! Moving back to handle this regression in RC2.

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


5 years ago

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


5 years ago

#11 @SergeyBiryukov
5 years ago

  • Keywords dev-feedback added

Marking for a second committer's review.

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


5 years ago

#13 @azaozz
5 years ago

48312-fix-and-test.patch looks good, +1 to commit. Just a small WPCS "violation", should be 1 === count() not 1 == count() :)

Last edited 5 years ago by azaozz (previous) (diff)

#14 @SergeyBiryukov
5 years ago

  • Keywords dev-reviewed added; dev-feedback removed

#15 @SergeyBiryukov
5 years ago

  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

#16 @SergeyBiryukov
5 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 46568:

Plugins: Restore backward compatibility for PHP4-style passing of array( &$this ) as action argument to do_action().

This is a follow-up to [46149] to avoid unnecessary breakage in case of using the old notation.

Props david.binda, jrf.
Reviewed by azaozz.
Fixes #48312.

#17 @whyisjake
5 years ago

In 46707:

Tests: Fix a typo in an inline comment.

Fix a regression from [46568].

Props david.binda.
See #48312.

#18 @SergeyBiryukov
5 years ago

In 46708:

Tests: Fix a typo in an inline comment introduced in [46568].

Reviewed by whyisjake, SergeyBiryukov.
Props david.binda.
Merges [46707] to the 5.3 branch.
Fixes #48564. See #48312.

Note: See TracTickets for help on using tickets.