WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 8 years ago

#2584 closed defect (bug) (wontfix)

do_action merges array args, causing data loss prior to callback

Reported by: skeltoac Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.0.2
Component: Administration Keywords: dev-feedback
Focuses: Cc:

Description (last modified by skeltoac)

Try passing two associative arrays into an action callback.

Change History (9)

comment:1 @random9 years ago

What's the expected behaviour and what's happening instead?

function myfunction() {
	print_r(func_get_args());
	die();
}
// -1 == pass all arguments
add_action('myaction', 'myfunction', 1, -1);
do_action('myaction', array('1', '2', '3'), array('4', '5'));

returns

Array
(
    [0] => 1
    [1] => 2
    [2] => 3
    [3] => Array
        (
            [0] => 4
            [1] => 5
        )

)

which it seems to have been carefully designed to do. I think the apply_filters behaviour is better (passing arrays directly), but don't see the data loss. (Sorry if I've missed something or misread you!)

comment:2 @skeltoac9 years ago

Try it with a pair of associative arrays.

comment:3 @random9 years ago

Ah, of course! /me slaps self. (The keys are discarded when the array is passed to call_user_func_array(), if anyone is playing along at home.) Is it worth breaking backwards compatibility over, though, for an edge case with an easy workaround? A note in the docs or the code seems sufficient to me.

comment:4 @skeltoac9 years ago

  • Keywords dev-feedback added

A note would be one solution. Another would be to fix the function. Breakage is counter-indicated by the sheer lack of prior reports on this issue.

What thinkst thou, devs?

comment:5 @davidhouse9 years ago

I'm not sure I understand this. From skeltoac's title to this bug, it would appear that two associative arrays with clashing keys get their contents merged destructively, which sounds like reasonable behaviour (so I'd call invalid). But then random's comment on call_user_func_array() doesn't make sense, the merging's got nothing to do with that.

comment:6 @skeltoac9 years ago

  • Description modified (diff)

davidhouse, do you see how the first array is "demoted" by one level? You might have to test this yourself to see what it's doing.

Here's the use case: Using the update_option_{$option_name} action, log the old and new values of an option that stores an associative array. (apply #2553 first)

comment:7 @skeltoac9 years ago

Also re #2553, try this variation:

do_action("update_option_{$option_name}", $oldvalue, $_newvalue));

comment:8 @random9 years ago

It's not broken, just counterintuitive. Virtually everywhere else in the code where arrays are sent, the behaviour is intended. e.g.,

do_action('wp_authenticate', array(&$user_login, &$user_pass));

Should have a callback like

function my_auth($login, $pass) // ...

The array behaviour can't be changed without breaking plugins using those hooks. If the behaviour is undesirable, by all means discuss changing it. I just don't think it's bad enough to warrant breaking backwards compatibility over.

David:

do_action('myaction', 
	array('a' => 1, 'b' => 2, 'c' => 3), 
	array('d' => 4, 'e' => 5)
	);

get_func_args() in the callback returns this:

Array
(
    [0] => 1
    [1] => 2
    [2] => 3
    [3] => Array
        (
            [d] => 4
            [e] => 5
        )

)

But what gets passed to call_user_func_array() is this:

Array
(
    [a] => 1
    [b] => 2
    [c] => 3
    [0] => Array
        (
            [d] => 4
            [e] => 5
        )

)

No data is lost by the merge, it's just inaccessible to the function since call_user_func_array() discards the keys.

The workaround is just to pass an array of arrays as the first argument, like the #2553 patch does.

do_action('myaction', array( 
	array('a' => 1, 'b' => 2, 'c' => 3), 
	array('d' => 4, 'e' => 5)
	) );

comment:9 @Nazgul8 years ago

  • Resolution set to wontfix
  • Status changed from new to closed

No real traction for almost a year, so closing it as wontfix.

If somebody has suggestions/patches/... to add, feel free to reopen.

Note: See TracTickets for help on using tickets.