Pkp-lib: ValidationFactory reports required fields with value "0" as invalid

[ViRuSTriNiTy]

Hi there,

i'm currently trying to use ValidationFactory to validate my custom form. In this custom form i have a drop down with required flag and <option value="" ..., <option value="0" ...) , <option value="1" ...) as well as <option value="2" .... When i select the option with value "0" and submit the form the ValidationFactory reports the the field as required although i selected something. I tracked this issue down to the use of the empty() function. Relying on this function to detect "empty" values means that a field will not validate when the user inputs the string "0" and the docs confirm this by stating

A variable is considered empty if it does not exist or if its value equals FALSE

Just try it out yourself, open a submission that is in review process and try to change the title to "0". Won't work because validation fails.

I'm not really sure why you use empty(). Can we change it to a simple check for "empty" string like $value == '' instead? There is already a long SO question about how to check for "empty" strings whereas some users also suggest to not use empty(), see https://stackoverflow.com/a/719002/3936440.

:Edit: Also the validator assumes that a required form field will post a value on form submit. If no value is posted then the empty check isn't even executed as a key check is executed first and this key check returns false. See the following line taken from ValidationFactory::required()

if ($action === VALIDATE_ACTION_EDIT &&
    array_key_exists($requiredProp, $props) && // <--  when this check returns false then empty() is not executed
    empty$props[$requiredProp]))) {`
  $validator->errors()->add($requiredProp, __('form.missingRequired'));

Maybe i should create a seperate issue for this.

So lonG
Daniel

[asmecher]

@ViRuSTriNiTy, agreed, we should not be using empty for this. Do you have a specific change to propose?

Just to confirm that I understand the key check issue -- is a 0 for e.g. a title also setting off that problem? Or are there different conditions to reproduce it?

[ViRuSTriNiTy]

@asmecher Fields are validated universally in ValidationFactory, so no matter what field type, a 0 will result in a "field is required" message.

My proposal would be a simple static private method named isEmpty()

static private function isEmpty($value) {
    return $value == '';
}

and a replace of empty($props[$requiredProp]) calls with self::isEmpty($props[$requiredProp]).

The key check issue is actually a different issue (form field does not post a value on submit, no value dependency). That's why i suggested to create an additonal issue for this.

[NateWr]

Thanks, this is an area where unit tests would be really useful. :+1:

[asmecher]

Merged https://github.com/pkp/pkp-lib/pull/5788 -- thanks, @ViRuSTriNiTy!