Added setting of collectionRawValues property on CollectionInputFilter during setData

rkeet commented 6 years ago

Provide a narrative description of what you are trying to accomplish:

[x] Are you fixing a bug?

Fix for bug described here: https://github.com/zendframework/zend-inputfilter/issues/168

rkeet commented 6 years ago

CS-Check passed

It moaned about line endings (I'm on Windows) but didn't apply to commited file (commiting Unix style)

froschdesign commented 6 years ago

@rkeet Please provide also an unit test. Thanks!

rkeet commented 6 years ago

@froschdesign I gave it a shot. Does/would it suffice?

edit: test fails without fix, passes with fix

rkeet commented 6 years ago

@froschdesign Could you have a look and let me know what you think? @weierophinney pinging by suggestion of @svycka to see if you could possibly also have a look and give an opinion?

edit: related bug report

froschdesign commented 6 years ago

@rkeet I think, we should look at the behaviour of the BaseInputFilter class and compare:

public function testRawValuesMethod()
{
    $data = [
        'field' => 'foo bar',
        'raw'   => 'baz',
    ];

    // Input-filter
    $inputFilter = new BaseInputFilter();
    $input = new Input('field');
    $input->getFilterChain()->attachByName(SeparatorToDash::class);
    $inputFilter->add($input);
    $inputFilter->setData($data);

    // Tests
    $this->assertEquals(['field' => 'foo bar'], $inputFilter->getRawValues());

    $this->assertTrue($inputFilter->isValid());

    $this->assertArrayHasKey('field', $inputFilter->getValues());
    $this->assertArrayNotHasKey('raw', $inputFilter->getValues());
    $this->assertArrayHasKey('field', $inputFilter->getRawValues());
    $this->assertArrayNotHasKey('raw', $inputFilter->getRawValues());

    $this->assertEquals(['field' => 'foo-bar'], $inputFilter->getValues());
    $this->assertEquals(['field' => 'foo bar'], $inputFilter->getRawValues());
}

The raw value(s) means the unfiltered value(s) of defined inputs – not the entire input of setData.

References

Code

https://github.com/zendframework/zend-inputfilter/blob/4901fc67554efbb7c292ae499129792977df1220/src/Input.php#L286-L289

https://github.com/zendframework/zend-inputfilter/blob/4901fc67554efbb7c292ae499129792977df1220/src/Input.php#L313-L317

https://github.com/zendframework/zend-inputfilter/blob/4901fc67554efbb7c292ae499129792977df1220/src/BaseInputFilter.php#L433-L441

Documentation

The getValue() method returns the filtered value of the 'foo' input, while getRawValue() returns the original value of the input.

https://docs.zendframework.com/zend-inputfilter/intro/#introduction

So this is wrong: 'raw_only' => 'not in filters - should be in raw data',

(I hope I understood your goal correctly.)

svycka commented 6 years ago

@froschdesign I guess you did not understand he changed how raw values will be retrieved. Before this change raw values were retrieved from each inputFilter with their getRawValues() logic. and now raw values are set in setData() method and it does not do anything with it just sets what it gets and later returns.

example what could be different(pseudo code)

// inputfilter config:
CollectionInputFilter
    inputFilter
       - name

// example input data
[
  'collection' => [
    ['name' => 'your name', 'invalid_field' => 'omg'],
  ]
]
// getRawValues() result before this PR
[
    ['name' => 'your name'],
]
// getRawValues() result after this PR
[
    ['name' => 'your name', 'invalid_field' => 'omg'],
]

but after this PR getRawValues should work a lot faster :D as it only does one set and one get comparing to collection_elements_count*collection_inputfiler_fields_count calls and it't strange why it was implemented like that and because of this I think there was a reason for this.

froschdesign commented 6 years ago

@svycka I think, you missed the key question: Why should the invalid field be included?

// getRawValues() result after this PR
[
    ['name' => 'your name', 'invalid_field' => 'omg'],
]
'raw_only' => 'not in filters - should be in raw data',

svycka commented 6 years ago

@froschdesign yes maybe you know the answer? also, I am not sure there are no more differences because raw values now get from a lot of inputs and foreach... where can have different getRawValues implementation

froschdesign commented 6 years ago

@svycka

yes maybe you know the answer?

See my comment above:

The raw value(s) means the unfiltered value(s) of defined input(s) – not the entire input of setData.

I also see the problem of a BC break and the changes in the current behaviour, because the return values are different and the raw values are set before validation. If we include the entire input of setData then we hijack the getRawValues methods.

I think, we should wait for an answer from @rkeet.

rkeet commented 6 years ago

A CollectionInputFilter contains a target InputFilter. Based on data received via a post request zero or more instances of the InputFilter are dynamically created of the target InputFilter and are part of the CollectionInputFilter.

When looping a CollectionInputFilter, the isValid method is applied to each InputFilter instance. For $this->collectionRawValues[$key] = $inputFilter->getRawValues(); to be executed a series of if() statements, one including a return statement, are done.

Setting the rawValues on a CollectionInputFilter should be completely separate from what happens in any of the instances created due to a property. As such, using the setData function for a straight copy/paste of all data, regardless of whether or not input names are valid, should be done for the CollectionInputFilter.

Also, the (raw) data of a CollectionInputFilter itself is never validated. By which I mean that the complete collection of all InputFilter instances, part of the collection, are never validated as a whole. As such, this is again an excellent place for keeping complete raw data, as the data eventually used is evaluated on a per InputFilter bases and not a whole CollectionInputFilter.

Simply put: when receiving data for a collection of input filters, it should be treated at the level of the collection, not a level deeper (input filter level).

@froschdesign your comment above appeared while I was trying to word this reply to earlier discussion so reply here:

Concern about BC-break Based on my own usage of this fix so far, I have not encountered problems with regards to a BC-break (repo with this fix and specific class with fix). If anything, this fix caused multiple issues to be resolved, such as re-populating forms for continued editing, validation of deep nesting (BaseFieldset > Fieldset > Collection > Fieldset > Collection > Fieldset), and a host of other small issues which cause known headaches.

Definition of getRawValues I hope my reply above, about the differences in levels between CollectionInputFilter vs (target) InputFilter suffices, else I can try again to explain.

froschdesign commented 6 years ago

@rkeet Thanks for your answer! 👍

But that still does not make it right:

// getRawValues() result after this PR
[
    ['name' => 'your name', 'invalid_field' => 'omg'],
]

If anything, this fix caused multiple issues to be resolved, such as re-populating forms for continued editing, validation of deep nesting (BaseFieldset > Fieldset > Collection > Fieldset > Collection > Fieldset), and a host of other small issues which cause known headaches.

Please don't get me wrong, it is very good and important to fix these problems, but changing the return value of getRawValues method is wrong here.

rkeet commented 6 years ago

[...] changing the return value of getRawValues method is wrong here.

After further considering this, yes, I agree. Also, I'm having trouble thinking of a proper fix, because setting this data here, does take care of the issues I've mentioned, at least when using such deep nesting as exampled.

I'm open to suggestions here. Apart from a version bump with this fix, I'm flush out of ideas on for this issue at the moment.

rkeet commented 5 years ago

Had a wee idea/brainfart for this. Not sure if it would solve the issue, but throwing it in here so it gets thought about.

Instead of modifying getRawValues (as discussed at length above), how about an additional getUnfilteredValues() function. This might be something to add onto InputFilter instead of CollectionInputFilter to make it uniformly available, maybe in the same way as with getRawValue/getRawValues when it comes to a single InputFilter vs a CollectionInputFilter, but still having the whole data set on the collection as well (like I was trying with this PR).

Suggested:

InputFilter

property unfilteredData -> gets set at setData time
function getUnfilteredDataValue returns unfiltered data (literally nothing has been done with this data - though returning might need to be blocked until filtered/validated for security, don't want it to be a risk)
property data gets used as before - no change
function getRawValue -> no change

CollectionInputFilter

property unfilteredData also used - Holds data for complete collection of InputFilter objects ->inherited
function getUnfilteredDataValue returns unfiltered data of collection -> inherited
property data -> no change
function getRawValues -> no change (still uses child objects' getRawValue in loop)

Idea would give additional property & function on InputFilter but would allow pure user data to be returned. Idea would also partially undo this PR.

Like I said, throwing it out there, just an idea. If you guys think it has merit and/or is an improvement of PR, I'm willing to go give it a shot.

Small addition (edit): Not sure how this would impact the fixes I mentioned and have in use due to the PR in current state, but in my own (linked) repo's.

If anything, this fix caused multiple issues to be resolved, such as re-populating forms for continued editing, validation of deep nesting (BaseFieldset > Fieldset > Collection > Fieldset > Collection > Fieldset), and a host of other small issues which cause known headaches.

weierophinney commented 5 years ago

I've had a chance to read through this and the original issue (#168), and have the following observations:

As @froschdesign notes, getRawValues() and getRawValue() are expected to return values only for known inputs. For collections, this means descending into each group in the collection and fetching those. I'm not clear after the various discussions if this is happening or not; if it isn't, we need to fix it.
We already have a method for retrieving unknown inputs: getUnknown(). I'm not sure if this is useful for collections, however.
Following on the above two points, I can see a clear case for being able to access the full set of data passed to a collection. As @rkeet noted in the original issue, this could be used to vary validation based on what fields are present.

My take is that we should add:

interface UnfilteredDataInterface
{
    /**
     * @return array
     */
    public function getUnfilteredData();

We would implement this interface in input filters and collections, and they would essentially just return the value of the $data property.

If this seems reasonable, let's go ahead and start working on that aspect. In the meantime, @rkeet , can you indicate whether or not getRawValues() is broken for collections (given the guidelines I outlined above), and, if so, write up a unit test demonstrating that? I can potentially work on solutions to that and/or the UnfilteredDataInterface additions early next week.

svycka commented 5 years ago

@weierophinney is this getUnfilteredData(); will be used as $context in validators?

froschdesign commented 5 years ago

@svycka I think this would change the current behaviour and ends in a BC break.

https://github.com/zendframework/zend-inputfilter/blob/799ad48ed1666d3c62126fec73dd20453b3a9e4d/src/BaseInputFilter.php#L232

svycka commented 5 years ago

then why would we need getUnfilteredData() as it will be impossible to use in validation

froschdesign commented 5 years ago

@svycka We will wait for the answer / solution from @rkeet. I think he already has an idea for it.

rkeet commented 5 years ago

@froschdesign @svycka @weierophinney

I haven't forgotten this, just didn't have time this weekend. I hope to get to it this week either during work, an evening or maybe during the coming weekend. However, very busy at the moment.

What I'll try to do the coming days:

Setup up a skeleton application demonstrating issue with plain modules, comments, etc.
Summarize this thread & #168 into a coherent whole

Quick re-read though:

getRawValue(s)() should not be altered from existing functionality to prevent bc-break
Agreed with that getRawValue(s)() returns only known input values, UnfilteredDataInterface should provide solution next to these functions
UnfilteredDataInterface adds functionality instead of modifying existing ones

I'll get into that more by providing the code I mentioned with unit testing - I hope this can wait a few more days (possibly weeks). Like I said, currently very busy, also next to my job. Though holidays soon, so might get something done then...

weierophinney commented 5 years ago

@rkeet Take your time - we can roll new releases pretty quickly. I'm going to go ahead and tag 2.9.0 today, as it gives us PSR-7 support, finalizing that story for zend-inputfilter and zend-form. We can roll 2.10.0 when you've got this done; ping me when you have something to review!

rkeet commented 5 years ago

Create new PR specific for previous comment about what to do, here.

Created new one as this one was specific for working with the getRawValue(s) functions, where as the new one adds new functionality UnfilteredDataInterface (and implementation)

zendframework / zend-inputfilter