Fixed Error trying to reset style property

[Arty]

I tried resetting style property @nodeStats and got this error:

Server Error
Array to string conversion

    XenForo_Application::handlePhpError()
    strval() in XenForo/Model/StyleProperty.php at line 1228
    XenForo_Model_StyleProperty->updateStylePropertyValue() in XenForo/Model/StyleProperty.php at line 1311
    XenForo_Model_StyleProperty->saveStylePropertiesInStyleFromInput() in XenForo/ControllerAdmin/Style.php at line 362
    XenForo_ControllerAdmin_Style->actionStylePropertiesSave() in XenForo/FrontController.php at line 347
    XenForo_FrontController->dispatch() in XenForo/FrontController.php at line 134
    XenForo_FrontController->run() in /Users/kasutaja/Documents/htdocs/xf/www/dev/admin.php at line 13

Tested in 1.4.0 RC 2

Edit: actually its not @nodeStats that is causing that issue. Simply saving style properties in nodes list group in admin control panel is triggering that error.

 

[Chris D]

What is the error? You have included the stack trace without the error message.

 

[Arty]

Sorry, forgot to add. Fixed.

 

[Arty]

I've added check to dump $definition and $newValue before that line when $newValue is not a string, here is property that is causing problems:

$definition dump:

Array
(
    [property_definition_id] => 23381
    [definition_style_id] => 0
    [group_name] => responsive
    [title] => Non-Responsive Minimum Width
    [description] => If the responsive design is disabled globally or for a specific page, this will define the minimum width of the page.
    [property_name] => nonResponsiveMinWidth
    [property_type] => scalar
    [css_components] => a:0:{}
    [scalar_type] => number
    [scalar_parameters] => 
    [addon_id] => XenForo
    [display_order] => 100
    [sub_group] => nonresponsive
    [property_id] => 39809
    [style_id] => 0
    [property_value] => 976px
    [effectiveState] => default
)

$newValue dump:

Array
(
    [border-top-width] => 
    [border-top-left-radius] => 
    [border-right-color] => 
    [border-right-style] => 
    [border-right-width] => 
    [border-top-right-radius] => 
    [border-bottom-color] => 
    [border-bottom-style] => 
    [border-bottom-width] => 
    [border-bottom-right-radius] => 
    [border-left-color] => 
    [border-left-style] => 
    [border-left-width] => 
    [border-bottom-left-radius] => 
    [margin-all] => 
    [margin-top] => 1px
    [margin-right] => 4px
    [margin-bottom] => 1px
    [margin-left] => 4px
    [padding-all] => 
    [padding-top] => 
    [padding-right] => 
    [padding-bottom] => 
    [padding-left] => 
    [width] => 14px
    [height] => 14px
    [extra] => display: block;
white-space: nowrap;
text-indent: 9999px;
overflow: hidden;
opacity: 0.25;
)

nonResponsiveMinWidth isn't even customized in that style and its not in nodes list group, so that doesn't make any sense.

 

[Arty]

Found property with that value: @nodeTinyIcon.

Checked it to make sure there is no mix up in definitions, it has its own correct definition with id 23377. It doesn't make any sense why would XenForo use definition from different style property.

 

[Arty]

Found where error is happening: XenForo_Application::ParseQueryString on this line (line 1245):

        $output = array_merge_recursive($output, $values);

I've attached dumps (via var_export to help debugging) of variables $chunk, $values and $output inside that loop as well as string value of $string

There is no mention of number 23381 anywhere in string, anywhere in $chunk and anywhere in $values, yet it magically appears as array key in $output

 

[Mike]

Ugh, the whole max_input_vars thing is really really annoying -- why it affects parse_str, I'll never know...

 

[Arty]

I figured that out. Problem is array keys are numbers, not strings. From array_merge_recursive description:

If, however, the arrays have the same numeric key, the later value will not overwrite the original value, but will be appended.

So because both old $output and $values have numeric key 23377, instead of merging arrays it creates new key that just happens to match definition id of another property.

 

[Arty]

Quick fix: replaced that line with

        $output = self::mergeArrays($output, $values);

and added new function:

  public static function mergeArrays()
   {
     $arrays = func_get_args();
     $result = array_shift($arrays);
     foreach ($arrays as $array)
     {
       if (!is_array($array))
       {
         continue;
       }
       foreach ($array as $key => $value)
       {
         if (!isset($result[$key]) || !is_array($result[$key]) || !is_array($value))
         {
           $result[$key] = $value;
         }
         else
         {
           $result[$key] = self::mergeArrays($result[$key], $value);
         }
       }
     }
     return $result;
   }

Edit: updated with better code that is shorter and checks if arguments are arrays to avoid errors.

 

[Mike]

You actually just wrote XenForo_Application::mapMerge.

There's a situation where that doesn't give the correct output. The problem with chunking this comes down to input in the form of:

a[]=0&a[]=1&a[]=2&a[]=3

The expected output is:

Array
(
    [a] => Array
        (
            [0] => 0
            [1] => 1
            [2] => 2
            [3] => 3
        )
)

If you force chunks of 1, the array_merge_recursive option gives the correct output. With mapMerge, this output is generated:

Array
(
    [a] => Array
        (
            [0] => 3
        )
)

So mapMerge fails if ...[] spans multiple chunks; array_merge_recursive fails if ...[123]... spans multiple chunks. The former is probably less common, but both could happen.

When I wrote this code, I did look at writing it by hand, but there are so many edge cases. I don't know if I need to look at it again.

 

[Arty]

Maybe simpler solution would be to change array keys to string for style properties? Like properties[prop12345] instead of properties[12345]

 

[Mike]

That would still fall over on other forms -- this is a technique that would be used on any potentially big form.

I have come up with a workaround that isn't 100% perfect, but it eliminates one of the common failure points. The problem with the mapMerge approach had to do with x[]=0&x[]=1... getting split over chunks. I'm now detecting these variables and automatically replacing the [] with a continuous set of numbers. The only time I believe this should fail is if you have a mix of "push" values and actual values, such as x[0]=1&x[]=2 . I don't think this should be that common though.