Bug #35136

Problem with validating simple types

Added by Rens Admiraal over 9 years ago. Updated about 9 years ago.

Status:
Resolved
Priority:
Should have
Category:
-
Target version:
-
Start date:
2012-03-22
Due date:
% Done:

100%

Estimated time:
PHP Version:
Has patch:
No
Complexity:

Description

When an argument of a controller action is annotated with @param integer $var, the validation never fails if a string is passed to the argument.

This is because the TypeConverter returns (integer) $value, which will always match on the Validator. This could for example be solved with something like this in the TypeCoverter:

        if (!is_numeric($source)) {
            throw new \TYPO3\FLOW3\Exception('Can\'t convert source value to integer', 1332411849);
        }

Problem with this approach is the output: an unfriendly error.

Maybe it would be possible to do a quick validation on simple types before the actual TypeConverter starts doing his job?

#1

Updated by Christian Müller over 9 years ago

I see what you mean but I think the Converter should just do it's job and don't validate anything, so throwing an exception is out of question for me. Also this problem about first mapping then validating or the other way around would probably need to be decided on a case by case basis (objects clearly first need mapping) and then custom validators and mappers are really complex to configure as you would need to define the mapping/validation order too.
I think the current approach is fine for most cases, I would say the validation rule on this argument needs to be tightened to example a number range then it could still throw some validation error.

#2

Updated by Rens Admiraal over 9 years ago

I'm totally fine with the conversion, that's not something to discuss I think. But validation always has to be done in a meaningful way. If we are converting types based on the @param tag AFTER the TypeConverter converts the value, then the validation is useless and will cause confusion and errors.

So either we should fix the validation, or we should clearly state the situations in which validation on simple types will not work (or maybe even totally skip validation on the @param annotation and require a @FLOW3\Validation rule...)

#3

Updated by Sebastian Kurfuerst over 9 years ago

you are not allowed to throw an exception inside the type converter in case of a user error.

Instead, you should return an \TYPO3\FLOW3\Error\Error object which is then shown to the user.

However, you at least need to make sure that the empty value is always correctly passed through, else no empty values are allowed.

This change should only go in with a proper functional test.

Greets, Sebastian

#4

Updated by Bastian Waidelich over 9 years ago

  • Project changed from TYPO3 Flow Base Distribution to TYPO3.Flow
  • Status changed from New to Accepted
#5

Updated by Bastian Waidelich over 9 years ago

  • Has patch set to No

Rens Admiraal wrote:

Hi Rens,

This is because the TypeConverter returns (integer) $value [...]

That's not what I observe. A string of "foo" is simply passed on to the action instead of being casted to integer (which would result in 0).

#6

Updated by Bastian Waidelich over 9 years ago

Bastian Waidelich wrote:

That's not what I observe.

Not true, forget my comment. Caching issue doh ;)

#7

Updated by Gerrit Code Review over 9 years ago

  • Status changed from Accepted to Under Review

Patch set 1 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/10071

#8

Updated by Gerrit Code Review over 9 years ago

Patch set 2 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/10071

#9

Updated by Gerrit Code Review over 9 years ago

Patch set 3 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/10071

#10

Updated by Gerrit Code Review about 9 years ago

Patch set 4 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/10071

#11

Updated by Gerrit Code Review about 9 years ago

Patch set 5 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/10071

#12

Updated by Bastian Waidelich about 9 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100
#13

Updated by Gerrit Code Review about 9 years ago

  • Status changed from Resolved to Under Review

Patch set 1 for branch FLOW3-1.1 has been pushed to the review server.
It is available at http://review.typo3.org/11719

#14

Updated by Bastian Waidelich about 9 years ago

  • Status changed from Under Review to Resolved

Also available in: Atom PDF