Remove exceptions from #4734 #4761

Merged
merged 4 commits into from Jul 19, 2013

Conversation

Projects
None yet
3 participants
Owner

weierophinney commented Jul 1, 2013

#4734 introduced some type checking into a number of filters in order to remove notices that were being raised about "array to string" conversions. However, it did so by raising exceptions, which will be a non-starter for anybody who was previously passing arrays and/or objects to these filters.

Likely the more appropriate method would be to simply return the value presented, and potentially raise an E_USER_WARNING indicating the filter's inability to process the value.

@ghost ghost assigned weierophinney Jul 1, 2013

weierophinney added some commits Jul 1, 2013

[#4761] Convert exceptions to warnings
- To retain BC, filters should never raise exceptions
- Instead, raising warning and returning original value
[#4761] Added tests for null values
- null values should be filtered without warnings
[#4761] null values should not raise warnings
- null values should not be filtered, and should not raise warnings
Contributor

gag237 commented Jul 2, 2013

If we accept an agreement that any Filter instance of Zend\Filter\FilterInterface should generate an error if it can not filter out, it is best to leave the exception.

I want to draw attention to such cases:

Lets we have $input (instance of InputFilrer\Input) based on config below

$inputSpec = array(
    'title' => array(
           'required' => false,
            'validators' => array(
                array(
                    'name' => 'stringLength',
                    'options' => array(
                        'max' => 2000
                    )
                )
            ),
            'filters' => array(
                array(
                    'name' => 'stripTags',
                ),
                array(
                    'name' => 'null',
                ),
            ),
     )
);

and we have any $data (of course not valid).

$data = array(
  'title' => array()
);

then i call $input->isValid() will be triggered notice.

If this data is from form it's logical trigger notice to developer to show that form does not correspond to inputFilter. But this data may be from any part, for example from GET.
And i think it is not good idea trigger notice, better leave Exception, because possible to make small changes in InputFilter\Input something like that:

public function getValue()
{
    $filter = $this->getFilterChain();
    try {
        return $filter->filter($this->value);
    } catch (\Zend\Filter\Exception\InputFilterException $e) {
       return $this->getRawValue();
    }
}
Owner

weierophinney commented Jul 2, 2013

@gag237 It's a bad idea to use try/catch for logical branching -- if nothing else, because it adds a ton of processing overhead. Additionally, you're confusing filtering and validation. In the InputFilter, filters happen prior to validation -- so even if the values cannot be filtered, they will likely still fail validation.

Throwing the notice is still useful in these situations, however, as you can then see in the logs that some location on your site is submitting data incorrectly -- or somebody is attempting to trigger failures to try and find vulnerabilities.

Contributor

gag237 commented Jul 2, 2013

Perhaps you are right.

Is it possible to change the description of Filter\FilterInterfatse::filter() when the filter should throw an exception RuntimeException, when trigger error.

Owner

weierophinney commented Jul 2, 2013

@gag237 Go for it.

Member

Maks3w commented Jul 14, 2013

That methods have a contract for return string but with this approach are returning null or mixed

If this try to resolve a BC Break I suggest revert the original PR (#4734) and apply it for the next "major" version

Owner

weierophinney commented Jul 18, 2013

@Maks3w It's not a BC break; it's correcting already released behavior to be more consistent and less brittle.

Member

Maks3w commented Jul 19, 2013

Can you update the docblocks for reflect the changes? (return types and some note about the E_USER_WARNING)

Add info in docblock about warnings
- Added a paragraph indicating inability to filter non-scalar values,
  and that a warning will be raised in such circumstances.
- Updated return annotations to indicate a "mixed" possibility.
Owner

weierophinney commented Jul 19, 2013

@Maks3w Done.

@ghost ghost assigned Maks3w Jul 19, 2013

Maks3w added a commit that referenced this pull request Jul 19, 2013

Maks3w added a commit that referenced this pull request Jul 19, 2013

Maks3w added a commit that referenced this pull request Jul 19, 2013

@Maks3w Maks3w merged commit f5c6ba2 into zendframework:master Jul 19, 2013

1 check failed

default The Travis CI build failed
Details

weierophinney added a commit to zendframework/zend-filter that referenced this pull request May 15, 2015

[zendframework/zendframework#4761] Convert exceptions to warnings
- To retain BC, filters should never raise exceptions
- Instead, raising warning and returning original value

weierophinney added a commit to zendframework/zend-filter that referenced this pull request May 15, 2015

[zendframework/zendframework#4761] Added tests for null values
- null values should be filtered without warnings

weierophinney added a commit to zendframework/zend-filter that referenced this pull request May 15, 2015

[zendframework/zendframework#4761] null values should not raise warnings
- null values should not be filtered, and should not raise warnings

gianarb pushed a commit to zendframework/zend-filter that referenced this pull request May 15, 2015

gianarb pushed a commit to zendframework/zend-filter that referenced this pull request May 15, 2015

gianarb pushed a commit to zendframework/zend-filter that referenced this pull request May 15, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment