somnambulist-tech / validation

A re-write of rakit/validation, a standalone validation library inspired by Laravel Validation
MIT License
44 stars 13 forks source link

validating empty arrays #24

Closed laylatichy closed 5 months ago

laylatichy commented 9 months ago

hi,

ran into an issue today

reproduction: https://phpsandbox.io/n/9ld9z#index.php

'required|array' for [] should pass unless I'm missing something

'string' for [] should fail, same as above

empty arrays passed in data are treated as missing instead of an array

dave-redfern commented 9 months ago

@laylatichy Apologies for the delay in responding to you.

The rule is working as intended because the array is empty but you have explicitly said it should be there via required. Required strictly doesn't check for the key but the value, therefore for validation purposes, unless explicitly allowed, "empty" values (false, '' (empty string), [], etc) are treated as being invalid. This happens in the Validation class when it validates the attribute and performs an isEmptyValue check via the Required rule.

Currently there is no empty array rule and I am not sure that this would be a good addition. This, to me, sounds like it is an optional field and instead of allowing an empty array, flag it as sometimes and then otherwise require a value in the array or don't pass the array at all. Alternatively: you could always add a custom rule that allows empty arrays and add it in to your validations for your specific use case.

Does this make sense?

laylatichy commented 9 months ago

hi @dave-redfern thanks for the reply, no worries

that array required was just an example

I was facing an issue with optional parameter in request, let's call it 'city' that should be string if present, but user sent an empty array

https://phpsandbox.io/e/x/x9lcp

it doesnt trigger an error, getValidatedData is returning empty array so later on in the chain array 'city' is passed as an argument into some function that expects string

had to write additional validator around this package so just thought that was unintended behaviour, if not feel free to close this issue

dave-redfern commented 9 months ago

Ah! Now that is an issue; that should have failed validation. I'll take a look at it.

Thanks for highlighting that bug.

dave-redfern commented 9 months ago

Looking into the internals, this is the result of what I consider to be a flaw in how the rules are utilised. Currently (this is how it was inherited), defining a rule e.g. city => string does not implicitly make it required. So when validateAttribute is called, even though the string rule did fail, because the field is considered "optional" and it is an empty value, it is treated as being ignorable and is skipped. However: at this point it is in the validation loop that treats it as being valid, so the now clearly bad value is added to validated attributes.

The immediate "fix" is to add required into the ruleset: city => sometimes|required|string. That will actually trigger the validation correctly and prevent that optional code path from running since it will fail as the value is "empty" even though it could be passed an array. sometimes will still allow the field to be skipped entirely.

The real fix that I have not had the time to dedicate as yet, is to re-implement the rules so that required is not needed (required_if etc would still be there but need re-working), because any rule unless it is sometimes or nullable, would be treated implicitly as required. This change would break compatibility with prior versions and the original rakit/validation lib so it would be a major version increase.

laylatichy commented 9 months ago

thanks @dave-redfern

yeah that's what I'm currently doing in my wrapper + swap 'city is required' message to something custom as in fact it's not required

thank you for your time, will keep an eye on future releases

dave-redfern commented 5 months ago

I'm closing this issue as the fix is already planned for the 2.0 rebuild.