POST['nav-menu-data'] breaks other POST values

[Unyson]

This breaks POST values.

Steps to reproduce:

1. Install ​Unyson plugin and activate it
2. Create {theme}/framework-customizations/theme/manifest.php with the following contents:

$manifest['supported_extensions'] = array(
    'megamenu' => array(),
);

3. Go to Unyson page, install and activate the MegaMenu extension
4. Go to Appearance > Menus
5. Create 2 menu hierarchy ​like this
6. Follow ​these steps

[ericlewis]

Thank you for the bug report @unyson. I can confirm this behavior.

Our extraction of PHP $_POST variables does not currently support double-nested array variables. It should, and perhaps we can find an intelligent manner to support any level of nested arrays here, in case other plugins are going deeper.

[keraweb]

Currently the Json is decoded like this.

<?php
json_decode( stripslashes( $_POST['nav-menu-data'] ) );

This creates an array of objects if I'm correct. I think changing this to the code below could solve this since it will create an array of arrays.

<?php
json_decode( stripslashes( $_POST['nav-menu-data'] ), true );

It's 2AM here so I'll do some tests tomorrow!

[swissspidy]

How can I reproduce it without using this plugin? Some tests would be great, so thanks in advance @keraweb!

We were planning to release 4.5.1 tomorrow, so we'd need a solution (and a ticket owner) sooner rather than later.

[keraweb]

Busy testing different things at the moment. You can ignore my previous comment, this made no sense at all. (need to remind myself not to think about code when I'm tired...)

[keraweb]

Okay, Did some testing with various plugins and this looks promising. Perhaps someone could make the code more "neat".

This is a complete redo of the current function that handles the $_POST['nav-menu-data'] data.

Instead of a preg_match i've used explode to separate all the array keys (removing the "]" afterwards).
I loop through all the depth levels to create a temporary array that has the correct depth and keys.
When this is complete I merge this with the $_POST values (replacing if needed).

If anyone can help me test this for different plugins and settings that would be great.

@Unyson can you check if the problem is fixed for your plugin? I somehow can't get the plugin to work properly in the dev release of WP.

<?php
/*
 * If a JSON blob of navigation menu data is found, expand it and inject it
 * into `$_POST` to avoid PHP `max_input_vars` limitations. See #14134.
 */
if ( isset( $_POST['nav-menu-data'] ) ) {
        $data = json_decode( stripslashes( $_POST['nav-menu-data'] ) );
        if ( ! is_null( $data ) && $data ) {
                foreach ( $data as $post_input_data ) {
                        // For input names that are arrays (e.g. `menu-item-db-id[3]`), derive the array path keys via explode.
                        $levels = explode('[', $post_input_data->name );
                        if ( count( $levels ) > 1 ) {
                                if ( empty( $_POST[ $levels[0] ] ) ) {
                                        $_POST[ $levels[0] ] = array();
                                }
                                foreach ( $levels as $level_key => $array_key ) {
                                        if ( $level_key == 0 ) {
                                                continue;
                                        }
                                        // Remove the trailing "]"
                                        $levels[ $level_key ] = substr( $array_key, 0, -1 );
                                        // Cast keys with a numeric array index to integers.
                                        if ( is_numeric( $array_key ) ) {
                                                $levels[ $level_key ] = (int) $array_key;
                                        }
                                }

                                $temp_array = array(); // Temp array to create the correct depth with the level keys
                                $start = 1; // 1 because the level 0 allready exists in $_POST
                                $depth = count( $levels ) - 1; // minus 1 because counting arrays doesn't match their key indexes

                                // Add the actual value to the last found level
                                $temp_array[ $levels[ $depth ] ] = wp_slash( $post_input_data->value );
                                // Create the correct depth if needed
                                if ( $depth > $start ) {
                                        for ( $i = ( $depth - 1 ), $min = $start; $i >= $min; $i-- ) {
                                                // Create the new array depth
                                                $temp_array[ $levels[ $i ] ] = $temp_array;
                                        }
                                }
                                // Add (or replace if it exists) the value to the $_POST object at the correct depth
                                $_POST[ $levels[0] ] = array_replace_recursive( $_POST[ $levels[0] ], $temp_array );
                        } else {
                                $_POST[ $post_input_data->name ] = wp_slash( $post_input_data->value );
                        }
                }
        }
}

[ericlewis]

Replying to swissspidy:

How can I reproduce it without using this plugin?

Open an Edit Nav Menu screen with at least one item.

Run this JavaScript

var inputCheckbox = jQuery('<input type="checkbox" name="somevariable[1][title-off]">');
jQuery('.menu-item .menu-item-settings').first().prepend(inputCheckbox);

which injects an input with a multidimensional name into the form.

Submit the form, and inspect $_POST after the nav-menu-data expansion. Notice

["somevariable"]=> array(1) { [1]=> string(2) "on" }

where we would expect

["somevariable"]=> array(1) { [1]=> array(1) { [title-off] => string(2) "on" } }

This problem isn't expressed in core because ​the default Edit Nav Menu walker class uses single-dimension arrays for input element names. However, plugins can supply their own Nav Menu Walker class, which can create any input element names.

[ericlewis]

In attachment:36590.diff​, I've moved the variable expansion code into a separate function wp_expand_nav_menu_post_data(), and modified it to support multidimensional nesting. Feedback welcome on this implementation.

I've also added some tests. One still fails, as it seems array_merge_recursive() doesn't merge values under the same numeric index, and instead moves the value of one numeric index into the next numeric index (see ​this example). Is there a good way to merge arrays recursively while preserving numeric key indexes?

[keraweb]

@ericlewis

array_merge_recursive() won't work because it appends new values.
You'll need to use array_replace_recursive() to properly add the values. (see ​this example)
EDIT NOTE: We don't have to worry about the replace part in this case. All key values should be unique (otherwise the normal post would'nt work either)

Also, how does the preg_match() compare to explode() performance wise? I've understood that a preg_match is a relatively heavy function to use. Not sure though :-)

Also, the current function does the array merge on the whole $_POST value. I can imagine with larger menu's this can get slower than to do this merge/replace on the first array key that is found. The code has a lot less to loop through for every new key that is found.

Let me know your thoughts!

[ericlewis]

Replying to keraweb:

You'll need to use array_replace_recursive() to properly add the values.

Ah thank you!

Also, how does the preg_match() compare to explode() performance wise?

Good question! On a menu of 500 items (presumably with ~5000 POST variables, and 5000 preg_match() invocations) wp_expand_nav_menu_post_data() execution takes about 25ms on my computer. The entire request takes about ~5 seconds, presumably due to the cost of updating 500 menu item objects in the DB. I think here the preg_match() gives us a bit of readability without notable decline in perf.

Also, the current function does the array merge on the whole $_POST value. I can imagine with larger menu's this can get slower than to do this merge/replace on the first array key that is found. The code has a lot less to loop through for every new key that is found.

We'll have to merge onto the $_POST global because we're dealing with one POST variable at a time, which could represent a nested key (e.g. a[1][2][3]) and the next could be a deep-sibling of the previous (e.g. a[1][2][4]).

[ericlewis]

attachment:36590.2.diff​ cleans up the implementation, passes unit tests, and fixes the bug in the Unyson plugin. @Unyson, can you also apply this patch and confirm it works for you?

[keraweb]

@ericlewis ,

Looks good!

I think here the preg_match() gives us a bit of readability without notable decline in perf.

Did some testing with your preg_match vs an explode. You're right, performance wise the difference is almost non-existend.

We'll have to merge onto the $_POST global because we're dealing with one POST variable at a time, which could represent a nested key (e.g. a[1][2][3]) and the next could be a deep-sibling of the previous (e.g. a[1][2][4]).

Why not only merge/replace from the key we're in and leave the other keys be?
$_POST[ $key ] = array_replace_recursive( $_POST[ $key ], $new_post_data[ $key ] ))

Anyhow, code works for me!

[swissspidy]

Great to hear the patch fixes the behaviour, thanks for testing!

I like that there are now unit tests for this part, but it also introduces a new function, which we rarely do in a minor release.

I'd love to hear another opinion / see more testing before anything gets committed.

[keraweb]

@swissspidy , @ericlewis ,

Maybe just create a patch without the creation of a new function (like before)?
Since it's a relatively small block of code I don't suppose it realy matters for usability.

[swissspidy]

@keraweb A function is easier to test though...

[ericlewis]

Replying to swissspidy:

I like that there are now unit tests for this part, but it also introduces a new function, which we rarely do in a minor release.

What are our considerations in not introducing functions in a minor release? If it provides unit testing for a problematic piece of code then that seems beneficial.

[swissspidy]

Replying to ericlewis:

Replying to swissspidy:

I like that there are now unit tests for this part, but it also introduces a new function, which we rarely do in a minor release.

What are our considerations in not introducing functions in a minor release? If it provides unit testing for a problematic piece of code then that seems beneficial.

Don't get me wrong, I'm not opposed to doing it :) It just doesn't happen that often and I think is worth noting. Perhaps others have objections though, no idea.

[swissspidy]

36590.3.diff​ marks the function as private as it's not intended for public use.

[ocean90]

array_replace_recursive() is only available for PHP 5.3+.

[keraweb]

@ocean90 ,

Maybe create our own function to do this?

​http://stackoverflow.com/questions/2874035/php-array-replace-recursive-alternative

​http://en.michaeluno.jp/array_replace_recursive-alternative/

[swissspidy]

One possibility would be to add a compat function for array_replace_recursive() for PHP < 5.3. BuddyPress did this a while back, see ​https://buddypress.trac.wordpress.org/changeset/9263.

36590.5.diff​ is a proof-of-concept for that.

[ocean90]

When you want to go with a compat function then the function should be placed into wp-includes/compat.php.

[adamsilverstein]

Unfortunately this fix isn't ready in time for 4.5.1, punting for consideration in 4.5.2.

[SergeyBiryukov]

Replying to ericlewis:

What are our considerations in not introducing functions in a minor release? If it provides unit testing for a problematic piece of code then that seems beneficial.

There were some recent precedents in [32299], [32364], [36120], [36429].

[dd32]

Replying to ericlewis:

What are our considerations in not introducing functions in a minor release? If it provides unit testing for a problematic piece of code then that seems beneficial.

If the function exists in the same file, not a problem at all.
If the function exists in a different file, we avoid it.
The main reason is that failed updates can leave a site only partially updated and brings the potential for a fatal error due to the cross-dependency.

In this case, the fatal would only be present on the nav menu's screen so a fatal isn't fatal to the site, the user can still continue with another update, however it could also be added to trunk in the different file, and included inline in 4.5 without significant issue (We'd just not be able to include the unit test in the 4.5 branch).

[keraweb]

(Just an idea) What about the following:

Put the inline patch in the next minor release. So just create the whole fix in one file.

Directly after this patch is merged, create a new patch for the next major release that will overwrite the inline patch in the minor release and puts the compat function in compat.php where it belongs.

Patchception :-)

This way it's possible to at least fix it in next minor release.

[Unyson]

@ericlewis https://core.trac.wordpress.org/ticket/36590#comment:11

it works

(sorry for slow reply, I had set a wrong email in my profile and got no notifications)

[swissspidy]

Thanks for your input, @dd32!

In that case, we should put these 2 functions in wp-admin/nav-menus.php for 4.5.2, and wp-admin/includes/nav-menu.php for trunk. Of course a solution without the need for _array_replace_recursive() would be great, too.

Otherwise, should we perhaps just use if ( ! function_exists( 'array_replace_recursive' ) ) { function array_replace_recursive … } ?

[SergeyBiryukov]

Replying to swissspidy:

Otherwise, should we perhaps just use if ( ! function_exists( 'array_replace_recursive' ) ) { function array_replace_recursive … } ?

Yup, and I think it should go in wp-admin/nav-menus.php for 4.5.2, and wp-includes/compat.php for trunk.

[swissspidy]

Just uploaded two new patches, 36590-4.5.2.diff​ for 4.5.2 and 36590-trunk.diff​ for (surprise) trunk.

[Ryan Stutzman]

This bug also breaks ​https://wordpress.org/plugins/location-specific-menu-items-by-country/ New values (array) are not saved.

[Ryan Stutzman]

Hi keraweb, I can verify this fixes it.

Replying to keraweb:

Okay, Did some testing with various plugins and this looks promising. Perhaps someone could make the code more "neat".

This is a complete redo of the current function that handles the $_POST['nav-menu-data'] data.

Instead of a preg_match i've used explode to separate all the array keys (removing the "]" afterwards).
I loop through all the depth levels to create a temporary array that has the correct depth and keys.
When this is complete I merge this with the $_POST values (replacing if needed).

If anyone can help me test this for different plugins and settings that would be great.

@Unyson can you check if the problem is fixed for your plugin? I somehow can't get the plugin to work properly in the dev release of WP.

<?php
/*
 * If a JSON blob of navigation menu data is found, expand it and inject it
 * into `$_POST` to avoid PHP `max_input_vars` limitations. See #14134.
 */
if ( isset( $_POST['nav-menu-data'] ) ) {
        $data = json_decode( stripslashes( $_POST['nav-menu-data'] ) );
        if ( ! is_null( $data ) && $data ) {
                foreach ( $data as $post_input_data ) {
                        // For input names that are arrays (e.g. `menu-item-db-id[3]`), derive the array path keys via explode.
                        $levels = explode('[', $post_input_data->name );
                        if ( count( $levels ) > 1 ) {
                                if ( empty( $_POST[ $levels[0] ] ) ) {
                                        $_POST[ $levels[0] ] = array();
                                }
                                foreach ( $levels as $level_key => $array_key ) {
                                        if ( $level_key == 0 ) {
                                                continue;
                                        }
                                        // Remove the trailing "]"
                                        $levels[ $level_key ] = substr( $array_key, 0, -1 );
                                        // Cast keys with a numeric array index to integers.
                                        if ( is_numeric( $array_key ) ) {
                                                $levels[ $level_key ] = (int) $array_key;
                                        }
                                }

                                $temp_array = array(); // Temp array to create the correct depth with the level keys
                                $start = 1; // 1 because the level 0 allready exists in $_POST
                                $depth = count( $levels ) - 1; // minus 1 because counting arrays doesn't match their key indexes

                                // Add the actual value to the last found level
                                $temp_array[ $levels[ $depth ] ] = wp_slash( $post_input_data->value );
                                // Create the correct depth if needed
                                if ( $depth > $start ) {
                                        for ( $i = ( $depth - 1 ), $min = $start; $i >= $min; $i-- ) {
                                                // Create the new array depth
                                                $temp_array[ $levels[ $i ] ] = $temp_array;
                                        }
                                }
                                // Add (or replace if it exists) the value to the $_POST object at the correct depth
                                $_POST[ $levels[0] ] = array_replace_recursive( $_POST[ $levels[0] ], $temp_array );
                        } else {
                                $_POST[ $post_input_data->name ] = wp_slash( $post_input_data->value );
                        }
                }
        }
}

[keraweb]

Hi Ryan,

Did you also check the latest patch?
#31 -> https://core.trac.wordpress.org/attachment/ticket/36590/36590-4.5.2.diff

[Ryan Stutzman]

Will do now and report back.

[Ryan Stutzman]

The patch works as well. Nice work!

Replying to keraweb:

Hi Ryan,

Did you also check the latest patch?
#31 -> https://core.trac.wordpress.org/attachment/ticket/36590/36590-4.5.2.diff

[swissspidy]

There wasn't any commit here to fix this bug. Why close it?

[swissspidy]

The version numbers in the patches need to be changed to 4.5.3, otherwise I'm still confident about the two different patches ( 4.5.x and trunk).

@ericlewis It'd be great if you could test the latest patch, since you've been working on this issue as well.

[ericlewis]

Tested attachment:36590-trunk.diff​, works well.

I renamed the function array_merge_recursive() to _array_merge_recursive() to test attachment:36590-4.5.2.diff​ as I don't have an easy way to test with a real 5.2 environment. I get a 502, which I believe is due to the logic at the beginning of array_merge_recursive() causing an infinite loop:

if ( function_exists( 'array_replace_recursive' ) ) { 
    return array_replace_recursive( $base, $replacements ); 
}

[swissspidy]

D'oh!

Yeah, the second if ( function_exists( 'array_replace_recursive' ) ) { check makes no sense.

[neverything]

Refreshed the patches for 4.5.3.

[neverything]

Refreshed the patch https://core.trac.wordpress.org/attachment/ticket/36590/36590-trunk.3.diff again, accidentally had some code for another ticket in it.

[swissspidy]

36590-trunk.4.diff​ updates the patch for trunk to apply cleanly again.

[swissspidy]

In 37748:

Menus: Support nested array variables in POST data when saving menus.

[36510] allowed larger menus to be created in the Edit Menu screen by JSON-encoding the entire form into a single input field. However, it did not correctly handle nested arrays.

This introduces a new _wp_expand_nav_menu_post_data() helper function to handle this POST data which uses array_replace_recursive() internally. Since the latter is only available on PHP 5.3+, we add a compatibility function to ensure PHP 5.2 support.

Props ericlewis, neverything, swissspidy.
Fixes #36590 for trunk. See #14134.

[swissspidy]

Reopening for 4.5.3

[swissspidy]

In 37750:

Menus: Fix _wp_expand_nav_menu_post_data() for PHP 5.2.

[37748] introduced _wp_expand_nav_menu_post_data() together with an array_replace_recursive() compatibility function for PHP 5.2.
Even though that compat function is tried and tested in other projects like BuddyPress, we need to add additional isset() checks in order to avoid 'Undefined index' notices in our case.

See #36590.

[swissspidy]

In 37754:

Menus: Support nested array variables in POST data when saving menus.

[36510] allowed larger menus to be created in the Edit Menu screen by JSON-encoding the entire form into a single input field. However, it did not correctly handle nested arrays.

This introduces a new _wp_expand_nav_menu_post_data() helper function to handle this POST data which uses array_replace_recursive() internally. Since the latter is only available on PHP 5.3+, we add a compatibility function to ensure PHP 5.2 support.

Merge of [37748] and [37750] to the 4.5 branch.

Props ericlewis, neverything, swissspidy.
Fixes #36590. See #14134.