WordPress.org

Make WordPress Core

Opened 4 weeks ago

Closed 3 weeks ago

Last modified 3 days ago

#48312 closed defect (bug) (fixed)

PHP4 notation for passing object by reference broken

Reported by: david.binda Owned by: SergeyBiryukov
Milestone: 5.3 Priority: normal
Severity: normal Version: 5.3
Component: Plugins Keywords: has-patch commit dev-reviewed
Focuses: Cc:
PR Number:

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 4 weeks ago.
48312-fix-and-test.patch (1.8 KB) - added by jrf 4 weeks 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
4 weeks ago

#1 @SergeyBiryukov
4 weeks ago

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

#2 in reply to: ↑ description @SergeyBiryukov
4 weeks 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
4 weeks 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.


4 weeks ago

#5 in reply to: ↑ 3 ; follow-up: @SergeyBiryukov
4 weeks 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
4 weeks 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
4 weeks 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
4 weeks 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
4 weeks 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.


4 weeks ago

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


4 weeks ago

#11 @SergeyBiryukov
4 weeks 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.


3 weeks ago

#13 @azaozz
3 weeks 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 3 weeks ago by azaozz (previous) (diff)

#14 @SergeyBiryukov
3 weeks ago

  • Keywords dev-reviewed added; dev-feedback removed

#15 @SergeyBiryukov
3 weeks ago

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

#16 @SergeyBiryukov
3 weeks 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
3 days ago

In 46707:

Tests: Fix a typo in an inline comment.

Fix a regression from [46568].

Props david.binda.
See #48312.

#18 @SergeyBiryukov
3 days 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.