auraphp / Aura.Filter

Validate and sanitize arrays and objects.
MIT License
159 stars 33 forks source link

custom rules not applying when field is blank #134

Closed php- closed 6 years ago

php- commented 7 years ago

Hello @pmjones I was trying to make 'required-if' rule, where the field would be required only in some conditions, but because of the fact that rules are not called to blank fields there is no workaround about this issue.

Thanks for help George

harikt commented 7 years ago

Hi,

Could you show some code what you have tried to implement or can you send a unit test to prove that there is a failure ?

Thank you

php- commented 7 years ago

Ill explain in short, add new validation rule and just return true, next create subject filter class, and do test the result on blank valued input. the result will be false. that means that validation is not called and returns false by default because of having blank value

php- commented 6 years ago

@harikt any updates here?

harikt commented 6 years ago

i ll look into this tomorrow. Currently on mobile.

On 14-Dec-2017 22:46, "George Garchagudashvili" notifications@github.com wrote:

@harikt https://github.com/harikt any updates here?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/auraphp/Aura.Filter/issues/134#issuecomment-351777337, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHWhgs_ITNvpzFmBAPZUcZIeag5bHTNks5tAVgBgaJpZM4NZvAf .

harikt commented 6 years ago

Finally I invested sometime to understand what you were saying. ( An example would have helped though, but as you failed to provide :( ) .

First I thought this was simple when you don't apply the blank rule and rule will be applied on it. But further testing with code I understood it is not as what I expected it to be.

Tested code below .

<?php
require __DIR__ . '/vendor/autoload.php';

class ValidateHex
{
    public function __invoke($subject, $field)
    {
        echo "\n Subject : " . $subject->$field;
        // done!
        return true;
    }
}

use Aura\Filter\FilterFactory;

$validate_factories = array(
    'test' => function () { return new ValidateHex(); },
);

$filter_factory = new FilterFactory(
    $validate_factories
);

$filter = $filter_factory->newSubjectFilter();

// the 'color' field must be a hex value of no more than 6 digits
$filter->validate('color')->is('test')->asSoftRule();

$subject = [
    'color' => ''
];

$success = $filter->apply($subject);

$failures = $filter->getFailures();
var_dump($failures->getMessages());

var_dump($success);

The underlying problem lies at https://github.com/auraphp/Aura.Filter/blob/ac33b700379d1cb978a077328425153060a91ab5/src/Spec/ValidateSpec.php#L40-L42 .

I don't have a solution for this right now. But that is a nice bug 👍 .

Thank you for the report.

UPDATE : I will try to invest some time tomorrow to write a test and see if I can fix this. I have some ideas / work around for this. Just wanted to make sure other tests run 👍 .

harikt commented 6 years ago

Oh oh and this seems related to https://github.com/auraphp/Aura.Filter/issues/119 the one @jakejohns created.

php- commented 6 years ago

@harikt ya u got it, thanks, and it will not work when that field is not present at all. This is important because what I'm trying to achieve is for example: there are required fields on insert, but those fields can be omitted from user when updating data. Hope we get solution soon. Best Regards

harikt commented 6 years ago

I am sorry that I can't make a solution for your problem.

I have created a failed test https://github.com/auraphp/Aura.Filter/pull/137 so in case someone find it interesting can play around the same.

harikt commented 6 years ago

I actually modified the PR #137 , so if you pass something as

->skipBlankRule();

It will skip checking whether the field is blank or not.

I am not sure if that is a right way or not. I wished if we could move the checking blank as a rule itself. But that will break some API I believe.

I am open to discussion about the same. Not very much convinced with the names. ie also open to discussion.

Will wait for others before we merge.

In the mean time you can use the branch via composer.

Hope that helps! .

harikt commented 6 years ago

Fixed via #137