PHP4 notation for passing object by reference broken

[david.binda]

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.

[SergeyBiryukov]

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?

[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.

[SergeyBiryukov]

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.

[azaozz]

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.

[jrf]

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

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

[SergeyBiryukov]

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.

[SergeyBiryukov]

Marking for a second committer's review.

[azaozz]

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

[SergeyBiryukov]

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.

[whyisjake]

In 46707:

Tests: Fix a typo in an inline comment.

Fix a regression from [46568].

Props david.binda.
See #48312.

[SergeyBiryukov]

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.