#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: |
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)
Change History (20)
#1
@
5 years ago
- Component changed from General to Plugins
- Milestone changed from Awaiting Review to 5.3
#2
in reply to:
↑ description
@
5 years ago
#3
follow-up:
↓ 5
@
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:
↓ 6
@
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
@
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:
↓ 8
@
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:
- The point of the removed code was to support objects passed by reference and the unit test was not passing the object by reference.
- 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.
- 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 usedassertEquals()
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
@
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
@
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.
Replying to david.binda:
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?