WireInputData should allow multidimensional arrays

[ghost]

@abdusco commented on Sep 28, 2017, 6:27 AM UTC:

Short description of the issue

I'm building an inputfield for a custom fieldtype. The inputfield will display a (possibly) long list of items with a number of fields in each. Instead of setting input name attributes beforehand and posting all field data back and forth and filtering saveable data on the backend, I'm trying to reduce the amount of processing by setting input names using JS only if it has been changed.

// using vue.js
<input @change="changed($event, item, 'author')" type="text" v-model="item.author">

changed(event, item, key) {
    input = event.target;
    if (!input.name) input.name = `changed[${item.id}][${key}]`; // name as multidimensional array
}

When the page is submitted, this was supposed to give me an array like this

But after some head scratching I found out that WireInputData class actually removes multidimensional arrays from $_POST (and $_GET) in cleanArray() method

/**
 * Clean an array of data
 * Removes multi-dimensional arrays and slashes (if applicable) 
 * @param array $a
 * @return array
 */
protected function cleanArray(array $a) {
    $clean = array();
    foreach($a as $key => $value) {
        if(is_array($value)) continue; // we only allow one dimensional arrays
        if(is_string($value) && $this->stripSlashes) $value = stripslashes($value);
        $clean[$key] = $value;
    }
    return $clean;
}

Expected behavior

$input->post should allow multidimensional arrays

Actual behavior

It removes them

Optional: Suggestion for a possible fix

Removing the following line solves the issue

if(is_array($value)) continue; // we only allow one dimensional arrays

Steps to reproduce the issue

Create a set of inputs like this

<input name="changed[1][text]"/> // defaults to type=text
<input name="changed[1][status]"/>
<input name="changed[2][status]"/>

Post the form, $input->post->changed will be an empty array.

Setup/Environment

PW 3.0.76

This issue was moved by netcarver from processwire/processwire-issues#387.

[ghost]

@LostKobrakai commented on Sep 28, 2017, 7:19 AM UTC:

Just so you know: This is an intentional limitation by ryancramerdesign, which was often discussed and requested to be changed. I can't remember the exact reasoning right now.

[ghost]

@abdusco commented on Sep 28, 2017, 7:23 AM UTC:

I'm guess this is why: https://security.stackexchange.com/questions/127808/is-array-injection-a-vulnerability-and-what-is-the-proper-term-for-it

2-3 levels should be enough. No need to allow arbitrarily deep arrays.

[ghost]

@matjazpotocnik commented on Sep 28, 2017, 7:25 AM UTC:

https://processwire.com/talk/topic/691-wireinput-only-allows-one-dimensional-arrays-why/?tab=comments#comment-5719

[ghost]

@abdusco commented on Sep 28, 2017, 7:27 AM UTC:

Probably what I will do is just limit the recursion to 2 or 3 levels to keep it safe. I don't see any potential issues with adding it.

Hopefully :]

[ghost]

@teppokoivula commented on Sep 29, 2017, 6:15 AM UTC:

One argument against enabling an arbitrary number of levels (2, 3, or whatever) would be that this will, again, cause unexpected issues that can be somewhat difficult to debug. Though it may solve most use cases, it could cause even more head scratching when deeper structures are needed.

That being said, I'm not strongly against enabling a number of levels. Another solution would be a configuration setting, or some other method of declaring that "right now I need this many levels".

[ghost]

@abdusco commented on Sep 29, 2017, 6:17 AM UTC:

Another solution would be a configuration setting, or some other method of declaring that "right now I need this many levels".

That sounds like the best approach, just like maxUrlSegments setting, a $config setting for input array depth would be great to have, instead of removing the value unexpectedly.

[ghost]

@netcarver commented on Feb 6, 2019, 10:08 PM UTC:

ryancramerdesign Do you want to take any further action on this issue? How about making the cleanArray() method hookable so people can override default behaviour if needed?

[ghost]

@ryancramerdesign commented on Feb 7, 2019, 3:56 PM UTC:

netcarver Thanks for your help in checking through all these issue reports, I really appreciate your efforts here.

Since neither the core or any of the PW Inputfields use multi-dimensional arrays, and aren't likely to, I don't want to add support for them at this time. After all, the PHP superglobals are already available when/if someone needs multi-dimensional arrays in input. If it weren't already easily accessible from the PHP superglobals then of course we'd support it. But since one can already get to this input by other means, I think it's better for PW to limit the possible inputs to it when possible. PW has always focused on really limiting those input vectors and and multi-dimensional arrays are just one of those doors I don't want to open.

[ghost]

@netcarver commented on Feb 7, 2019, 5:48 PM UTC:

ryancramerdesign abdusco LostKobrakai matjazpotocnik teppokoivula Ok, unless anyone else wants to make another argument for this, I intend to add the "won't fix" label and close this in a week or so.

[ghost]

@teppokoivula commented on Feb 7, 2019, 6:11 PM UTC:

My vote would still go for a two-fold solution, where this would a) be configurable behaviour, i.e. a new config setting for the max depth, and b) where one could override that max depth value on a case by case basis when requesting data.

I understand the point that ryancramerdesign makes – and obviously it's his choice in the end – but personally I prefer $input over PHP superglobals in all the ProcessWire related code I write, and see it as an important part of the toolset ProcessWire provides to developers.

As such, I do think that it's worth considering this, even if the core itself doesn't currently require it.

I'll admit that it's pretty rare that I personally really require multidimensional arrays of input – but when I do, having to use a mixture of $input and superglobals feels less than optimal.

[ghost]

@netcarver commented on Mar 14, 2019, 8:44 PM UTC:

At the moment this seems to clearly be a "won't fix" for Ryan. I am therefore moving it to the requests repo where this conversation can be continued if needed.

[gRegorLove]

I'd still really like this to be considered as a configuration option. I've built up a re-usable set of libraries around ProcessWire that validates and sanitizes form submissions. It is rare, but it throws a wrench in things when I need to have a multi-dimensional array.

Switching to $_POST in those scenarios means I need to wrote more custom code to handle validation and sanitization for those edge cases.

[Toutouwai]

@gRegorLove, see... Merged PR: https://github.com/processwire/processwire/pull/161 Blog announcement: https://processwire.com/blog/posts/pw-3.0.178/

I guess this request should be closed now.