Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrote injectUniqueIdentifierToRules() to be more generic #118

Closed
wants to merge 1 commit into from
Closed

Conversation

Shingedo
Copy link

A simple implementation of the problem mentioned in #117.

@dwightwatson
Copy link
Owner

This removes all the additional functionality provided for the unique: field, including the ability to infer table name, field name and primary key. I'm not particularly worried about that but it will be a breaking change and will probably need to be tagged 0.11.x.

Would you be able to make this PR against master so that it goes into the Laravel 5 version at least, then I suppose I'll have to make a new branch 0.11 and backport the change into there.

@Shingedo
Copy link
Author

Yeah it unfortunately does, but I was actually quite puzzled why it was there in the first place. Especially since I think that it doesn't work if you want to specify additonal where clauses for a unique rule like this:

'email' => 'unique:users,email_address,NULL,id,account_id,<row-id>'

prepareUniqueRule wouldn't inject the id at all since the check in line 650 would fail because $parameter[3] would be id and not unset like it expects.

So I am not really sure if it is really breaking BC or rather removing a "feature" that didn't work completely in the first place. Furthermore I don't think much of anything would break because most likely nobody expected that functionality to be there in the first place and just wrote out the complete unique rules.

But if you still want to make a new major release I can do a PR against master

EDIT:
Nevermind I took another look at it and realized that the ignore_id would actually be at $parameter[2] which means that my solution doesn't work with added where clauses... Not good that no TestCase caught that.

Hmm this is actually quite a big issue, because the custom rule i mentioned in #117 always expects the id to be ignored to be the last parameter. The only idea i can come up with to resolve this would be adding a placeholder. Something like {ignore_id}.

@dalabarge
Copy link
Contributor

Just tossing in some commentary on this issue. I think the fundamental problem is that the Validator service itself does not support this custom rule. The key is to add a custom Validator and then use logic within that class to handle the problem. Unique ID injection is a common enough use case that making it part of the model is a short cut. Uniqueness across multiple columns, while useful in some cases, is not really a great thing to shove into the model unless provided as a bolt on trait (which also just feels wrong). Maybe you should look into providing a placeholder replacement functionality within a custom Validator macro.

http://laravel.com/docs/4.2/validation#custom-validation-rules

<?php 

use Illuminate\Validation\Factory;
use Illuminate\Validation\ValidationServiceProvider as ServiceProvider;

class ValidationServiceProvider extends ServiceProvider {

    public function register()
    {
        $this->registerPresenceVerifier();
        $this->app->bindShared('validator', function($app)
        {
            $validator = new Factory($app['translator'], $app);

            // The validation presence verifier is responsible for determining the existence
            // of values in a given data collection, typically a relational database or
            // other persistent data stores. And it is used to check for uniqueness.
            if (isset($app['validation.presence']))
            {
                $validator->setPresenceVerifier($app['validation.presence']);
            }

            // Add custom validation rule macros
            $macros = [
                'unique_with' => 'App\Validators\UniqueWithValidationMacro',
            ];
            foreach( $macros as $macro => $class)
            {
                $validator->extend($macro, $class . '@validate' . ucfirst(studly_case($macro)));
                $validator->replacer($macro, $class . '@replace' . ucfirst(studly_case($macro)));
            }
            return $validator;
        });
    }
}

On the ValidationMacro classes you'll have to get clever and figure out how to inject the model ID dynamically. Fortunately validateUniqueWith() will get an instance of the Validator which should have a reference to the model. If it doesn't then that would be the worth while PR for watson/validating.

Hope this helps you figure this use case out.

@dalabarge
Copy link
Contributor

This may be a non-issue now as a check in the prepareUniqueRule method injects the ID dynamically if the third parameter is null: if (! isset($parameters[2]) || strtolower($parameters[2]) === 'null') { so in the case of trying to pass on unique extra parameters you'd just do something like unique:connection.table,field,null,key,extras... and then prepareUniqueRule would inject the appropriate identifier.

@dwightwatson this PR could probably be closed.

@dwightwatson
Copy link
Owner

The latest release on the master branch now supports adding additional unique injectors and ships with support for the default Laravel unique and the popular third-party unique_with rules. You can add and remove support for additional rules as you please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants