dwightwatson / validating

Automatically validating Eloquent models for Laravel
MIT License
968 stars 76 forks source link

Little issue with "unique" rule #197

Closed ghost closed 6 years ago

ghost commented 6 years ago

Recently, i refactored one of my features to a Service class. Inside this service class, i'm creating a new Validator object merging rules of different kinds. (I'm doing this because i wanna all validations to be thrown in the same request). Here follow an example:

$validator = Validator::make(
    array_merge($data, ['categories' => $categories]),
    array_merge($product->getRules(), ['categories' => 'required'])
);

Obs: $product is an instance of the model Product that implements the ValidatingTrait from this package.

After this, i started to receive errors from the unique rule of this model when trying to update the record. I did some digging and found that the problem is $product->getRules() only return the array of rules i've declared in my model. So i need to run $product->updateRulesUniques();before calling it. I was expecting this to be ready already just after the model instantiation.

Well, i just wanna know if this is the expected behavior, or if there is anything i can do, or anything we can change on this package, so this call to updateRulesUniques() can be no needed anymore.

Thank you.

dwightwatson commented 6 years ago

I would say that it's expected behaviour that if you call getRules() you get back the rules set on the model untouched. I've not encountered a use-case like yours before but perhaps you could define another method on your base model like getProcessedRules() would return the rules after the unique rules have been handled?

ghost commented 6 years ago

I'm thinking different... as i still using getRules() to retrieve the prepared rules:

// return the original rules array
$product->getRules();

// now return the prepared rules array
$product->updateRulesUniques();
$product->getRules();

this means that after run the method updateRulesUniques(), i can't get the original rules again within this instance of my model.

This isn't really a problem, but it's kind of tricky. Maybe is better to aways maintain the array of rules as it is, retrievied by the getRules() method, and implement this new getPreparedRules() method.

PS: I noticed that the method updateRulesUniques() is not being used anywhere.

dwightwatson commented 6 years ago

Yeah, I get where you're coming from. Perhaps you'd be better off with injectUniqueIdentifierToRules($rules) which at the moment is a protected method on the model. That takes an array of rules and returns to you the updated ruleset.

For your use-case maybe you can implement the getPreparedRules() method on your base model like this.

public function getPreparedRules()
{
    return $this-> injectUniqueIdentifierToRules($this->getRules());
}

I suspect undateRulesUniques() was added to the public API because someone needed to access the processed rules externally - which is why it isn't used by the library itself. Does this help?

ghost commented 6 years ago

Anyway this can be implemented at the trait instead?

dwightwatson commented 6 years ago

I'm not opposed to it, happy to look at a PR if you feel like putting one together with tests.

ghost commented 6 years ago

Well, a can make a without any problem. I'm just trying to figure it out how to test this feature...

dwightwatson commented 6 years ago

If you have a look at some of the other tests around the uniques feature and it's methods that might give you a hint. Unfortunately it's not something I'm going to have time to take a look at any time soon. Otherwise just keep the code in your app/s.