Make WordPress Core

Opened 9 years ago

Closed 8 years ago

#36590 closed defect (bug) (fixed)

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

Reported by: unyson's profile Unyson Owned by: swissspidy's profile swissspidy
Milestone: 4.5.3 Priority: normal
Severity: normal Version: 4.5
Component: Menus Keywords: has-unit-tests has-patch commit fixed-major
Focuses: javascript, administration Cc:

Description

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(),
);
  1. Go to Unyson page, install and activate the MegaMenu extension
  2. Go to Appearance > Menus
  3. Create 2 menu hierarchy like this
  4. Follow these steps

Attachments (11)

36590.diff (5.1 KB) - added by ericlewis 9 years ago.
36590.2.diff (5.4 KB) - added by ericlewis 9 years ago.
36590.3.diff (5.5 KB) - added by swissspidy 9 years ago.
36590.4.diff (5.5 KB) - added by swissspidy 9 years ago.
36590.5.diff (7.2 KB) - added by swissspidy 9 years ago.
36590-trunk.diff (7.6 KB) - added by swissspidy 9 years ago.
36590-4.5.2.diff (7.0 KB) - added by swissspidy 9 years ago.
36590-trunk.2.diff (8.2 KB) - added by neverything 9 years ago.
36590-4.5.3.diff (4.3 KB) - added by neverything 9 years ago.
36590-trunk.3.diff (7.5 KB) - added by neverything 9 years ago.
36590-trunk.4.diff (7.4 KB) - added by swissspidy 8 years ago.

Download all attachments as: .zip

Change History (64)

#1 @Unyson
9 years ago

  • Keywords needs-patch added

#2 @ericlewis
9 years ago

  • Milestone changed from Awaiting Review to 4.5.1

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.

#3 @keraweb
9 years ago

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!

#4 follow-up: @swissspidy
9 years ago

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.

#5 @keraweb
9 years ago

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

#6 follow-up: @keraweb
9 years ago

  • Keywords needs-testing added

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 );
                        }
                }
        }
}

#7 in reply to: ↑ 4 @ericlewis
9 years ago

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
9 years ago

#8 @ericlewis
9 years ago

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?

Last edited 9 years ago by ericlewis (previous) (diff)

#9 follow-up: @keraweb
9 years ago

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

Last edited 9 years ago by keraweb (previous) (diff)

#10 in reply to: ↑ 9 @ericlewis
9 years ago

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
9 years ago

#11 @ericlewis
9 years ago

  • Keywords has-patch has-unit-tests added; needs-patch removed

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?

#12 @keraweb
9 years ago

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

#13 follow-up: @swissspidy
9 years ago

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.

#14 @keraweb
9 years ago

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

#15 @swissspidy
9 years ago

  • Keywords dev-feedback added

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

#16 in reply to: ↑ 13 ; follow-ups: @ericlewis
9 years ago

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.

#17 in reply to: ↑ 16 @swissspidy
9 years ago

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
9 years ago

#18 @swissspidy
9 years ago

  • Keywords needs-testing dev-feedback removed

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

@swissspidy
9 years ago

#19 @ocean90
9 years ago

array_replace_recursive() is only available for PHP 5.3+.

@swissspidy
9 years ago

#21 @swissspidy
9 years ago

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.

#22 @ocean90
9 years ago

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

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


9 years ago

#24 @adamsilverstein
9 years ago

  • Milestone changed from 4.5.1 to 4.5.2

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

#25 in reply to: ↑ 16 @SergeyBiryukov
9 years ago

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

#26 in reply to: ↑ 16 @dd32
9 years ago

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

#27 @keraweb
9 years ago

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

#28 @Unyson
9 years ago

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

#29 follow-up: @swissspidy
9 years ago

  • Keywords needs-refresh added

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 … } ?

#30 in reply to: ↑ 29 @SergeyBiryukov
9 years ago

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.

#31 @swissspidy
9 years ago

  • Keywords needs-testing added; needs-refresh removed

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

#33 in reply to: ↑ 6 @Ryan Stutzman
9 years ago

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 );
                        }
                }
        }
}

#34 follow-ups: @keraweb
9 years ago

Hi Ryan,

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

Last edited 9 years ago by keraweb (previous) (diff)

#35 in reply to: ↑ 34 @Ryan Stutzman
9 years ago

Will do now and report back.

#36 in reply to: ↑ 34 @Ryan Stutzman
9 years ago

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

#37 @Ryan Stutzman
9 years ago

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

#38 @swissspidy
9 years ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

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

#39 @swissspidy
9 years ago

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.

#40 @ericlewis
9 years ago

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 ); 
}

#41 @swissspidy
9 years ago

D'oh!

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

#42 @swissspidy
9 years ago

  • Keywords needs-refresh added

#43 @neverything
9 years ago

  • Keywords needs-testing needs-refresh removed

Refreshed the patches for 4.5.3.

#44 @neverything
9 years ago

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.

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


9 years ago

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


9 years ago

#47 @swissspidy
9 years ago

  • Keywords commit added

#48 @swissspidy
8 years ago

  • Owner set to swissspidy
  • Status changed from reopened to assigned

#49 @swissspidy
8 years ago

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

#50 @swissspidy
8 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#51 @swissspidy
8 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 4.5.3

#52 @swissspidy
8 years ago

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.

#53 @swissspidy
8 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

Note: See TracTickets for help on using tickets.