WordPress.org

Make WordPress Core

Opened 11 months ago

Last modified 6 months ago

#41502 new defect (bug)

_wp_expand_nav_menu_post_data() corrupts multiple select (array value)

Reported by: elliotcondon Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.9
Component: Menus Keywords: has-patch has-unit-tests
Focuses: administration Cc:

Description

Hi guys.

Elliot here - ACF developer. I've recently added custom fields to the WP menu and WP menu item forms.

In doing so, i have discovered that the _wp_expand_nav_menu_post_data() function (called in wp-admin/nav-menus.php on line 56) is corrupting some array $_POST values.

The issue is caused with inputs using a name like so: my_field[] my_field[]

Please note that there is no issue when a key is defined like so: my_field[0] my_field[1]

There are many cases where a developer would not define the 'keys' of an 'array type' input name. Think of a multiple select element. The element accepts only one name attribute and it must be defined like so my_field[].

  • Also, checkbox / radio fields are commonly rendered using the same naming style.

Is it possible to request some extra logic within the _wp_expand_nav_menu_post_data() function that allows for this?

I suspect the issue is caused on line 1125 of the 'wp-admin/includes/nav-menu.php' file where the $_POST value is modified: $new_post_data[ $array_bits[ $i ] ] = wp_slash( $post_input_data->value );

Thanks Elliot

Attachments (1)

41502.diff (3.9 KB) - added by dd32 6 months ago.

Download all attachments as: .zip

Change History (6)

#1 @dd32
6 months ago

  • Component changed from General to Menus
  • Focuses administration added
  • Keywords needs-testing added

Hey @elliotcondon - Sorry this never got any attention.

Would you be able to put together a short example plugin which shows the behaviour you're seeing? or steps on how to recreate it easily using ACF?

Looking at the code, and testing it with browser-inserted fields, it at first looks like it's working as expected for me, but I might be mis-understanding how you're expecting it to work, or I might be missing a crucial step.

JSON Posted:
[
...
 {
    "name": "testing[]",
    "value": "A"
  },
  {
    "name": "testing[]",
    "value": "B"
  },
...
]

Resulting var_dump( $_POST ):
...
  'testing' => 
    array (size=2)
      0 => string 'A' (length=1)
      1 => string 'B' (length=1)
...

edit: in the above situation, I was testing with <input type="text" name="testing[]" value="A"> fields

Last edited 6 months ago by dd32 (previous) (diff)

#2 @elliotcondon
6 months ago

Hi @dd32

Thanks for the reply.

I've just tried to replicate this issue using 2 text inputs as you have: <input type="text" name="testing[]" value="A"> <input type="text" name="testing[]" value="B">

I can confirm that the JSON posted correctly contains both inputs: {\"name\":\"testing[]\",\"value\":\"A\"},{\"name\":\"testing[]\",\"value\":\"B\"}

However, when printing out the $_POST array, the only "testing" is: [testing] => B

The $_POST data does not contain an array (as expected). Instead, only a single value. This is very different to the results you have found.

I have included a fix within ACF that changes the JSON string so that array values can correctly be posted. Can you please test on a site without ACF, or any other plugin that may be fixing this?

#3 @dd32
6 months ago

  • Keywords needs-patch added; needs-testing removed

Thanks for confirming that @elliotcondon

I have absolutely no idea how it worked for me previously, I've just followed my own steps and it's working exactly as you describe it. I even copy pasted the example!

@dd32
6 months ago

#4 @dd32
6 months ago

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

41502.diff contains a fix & a unit test for this.

I'm not 100% convinced that this hasn't changed the behaviour of the function in a some way, so more unit tests are probably needed / useful here.

#5 @elliotcondon
6 months ago

Hi @dd32

Thanks for the reply, I've glad you were able to replicate the issue. This patch looks to be working for the basic example, but not for more complicated scenarios.

Testing the basic example, everything looks to be working well:

<input type="text" name="testing[]" value="A">
<input type="text" name="testing[]" value="B">

Result:

[testing] => Array
        (
            [0] => A
            [1] => B
        )

However, there is another scenario where an issue occurs (slightly more complicated, but just as common). When working on a plugin, it is best practice to 'namespace' your $_POST data by writing input names like my[field_1];

I have found the following 2 issues:

Issue 1. Extra $_POST data. When using a name-spaced input name, the $_POST data contains an extra value. Note the 3rd bit of data is a duplicate:

<input type="checkbox" name="my[field_1][]" value="A"> A
<input type="checkbox" name="my[field_1][]" value="B"> B

Result:

[my] => Array
        (
            [field_1] => Array
                (
                    [0] => A
                    [1] => B
                    [] => B
                )

        )

Issue 2. Hidden input breaks $_POST data. When adding a list of checkbox or radio inputs, it is common to use a hidden input above the list to allow an "empty" value to be saved. Doing so prevents the array data from being interpreted:

<input type="hidden" name="my[field_1]" value="">
<input type="checkbox" name="my[field_1][]" value="A"> A
<input type="checkbox" name="my[field_1][]" value="B"> B

Result:

    [my] => Array
        (
            [field_1] => Array
                (
                    [] => B
                )

        )

Let me know how you go testing these 2 scenarios. Happy to help provide any other info.

Question. Why do we need such a complicated _wp_expand_nav_menu_post_data() function? If the form data is encoded into a JSON string and sent via $_POST['nav-menu-data'], can't we just decode this string and replace $_POST['nav-menu-data'] with the array value?

Thanks Elliot

Note: See TracTickets for help on using tickets.