search

dwightwatson / validating

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

Make model identifier injection more generic #154

Closed Propaganistas closed 8 years ago

Propaganistas commented 8 years ago

As a follow-up on https://github.com/dwightwatson/validating/pull/153.

This PR allows developers to specify the rules for model identifier injection in $uniqueRulesForInjection. This primarily prevents code duplication that would occur as you suggested in https://github.com/dwightwatson/validating/pull/153.

dwightwatson commented 8 years ago

I think the case for the $rule === 'unique' was because the library knows how to infer the correct parameters if they're not provided (i.e. the table name first, the column second, the primary key third). The prepareUniqueRule will do this for all the rules provided to it, and I'm not sure that will be compatible with unique_with.

Propaganistas commented 8 years ago

How about this? -> Detect if the unique rule is unique or something else -> If it is the core unique, then infer intermediary parameters (but not the identifier) -> always add the model identifier as last parameter

This way the core validator is fully supported and third party validators are limited supported (only adding the model identifier).

dwightwatson commented 8 years ago

That change still maintains an assumption that other unique_* validators will use the model identifier as the last parameter, and the library will have no way of knowing (if, for whatever reason) the ID has already been set.

The only way I feel would be appropriate, but I'm not convinced is worth the trouble, is to take any rules with the term unique in them, check the model for a camelCased method name, something like injectUniqueWithIdentifiers or something, which could handle the implementation for the given rule. The library could then ship with just injectUniqueIdentifiers and then other people can support other unique validators downstream if they wanted to.

Propaganistas commented 8 years ago

Ok no problem. Closing this for now.