Opened 9 years ago
Closed 8 years ago
#36590 closed defect (bug) (fixed)
POST['nav-menu-data'] breaks other POST values
Reported by: | Unyson | Owned by: | 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:
- Install Unyson plugin and activate it
- Create
{theme}/framework-customizations/theme/manifest.php
with the following contents:
$manifest['supported_extensions'] = array( 'megamenu' => array(), );
- Go to Unyson page, install and activate the MegaMenu extension
- Go to Appearance > Menus
- Create 2 menu hierarchy like this
- Follow these steps
Attachments (11)
Change History (64)
#3
@
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:
↓ 7
@
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
@
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:
↓ 33
@
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
@
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.
#8
@
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?
#9
follow-up:
↓ 10
@
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!
#10
in reply to:
↑ 9
@
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 toexplode()
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]
).
#11
@
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
@
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:
↓ 16
@
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
@
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.
#16
in reply to:
↑ 13
;
follow-ups:
↓ 17
↓ 25
↓ 26
@
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
@
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.
#18
@
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.
#20
@
9 years ago
@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/
#21
@
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
@
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
@
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.
#26
in reply to:
↑ 16
@
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
@
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
@
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:
↓ 30
@
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
@
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
@
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.
#32
@
9 years ago
This bug also breaks https://wordpress.org/plugins/location-specific-menu-items-by-country/ New values (array) are not saved.
#33
in reply to:
↑ 6
@
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:
↓ 35
↓ 36
@
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
#36
in reply to:
↑ 34
@
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
#38
@
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
@
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
@
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
@
9 years ago
D'oh!
Yeah, the second if ( function_exists( 'array_replace_recursive' ) ) {
check makes no sense.
#44
@
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
#49
@
8 years ago
36590-trunk.4.diff updates the patch for trunk to apply cleanly again.
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.