Skip to content
This repository has been archived by the owner on Jul 16, 2023. It is now read-only.

Uniqueness #37

Closed
ghost opened this issue Apr 1, 2013 · 29 comments
Closed

Uniqueness #37

ghost opened this issue Apr 1, 2013 · 29 comments

Comments

@ghost
Copy link

ghost commented Apr 1, 2013

Since validation rules are declared as static, it is not possible to use instance variables to validate models for uniqueness?

When updating a user for example, you want to ignore uniqueness for the user being updated. You can do this by specifying the ID of the model to ignore. Otherwise the validation rules will fail as the user you are updating has the same email address as the user you are updating.

'email' => 'unique:users,email_address,10'

By using a Validator service you can pass an instance of the model into a method and return a validator object for that model which you can check on save.

namespace Models\Services\Validators;

use Models\User;
use Validator;

class UserValidator {

    public static function getValidator(User $user) {
        $rules = [
            'first_name'  => 'required|max:50',
            'last_name'   => 'required|max:50',
            'email'       => "required|max:100|email|unique:users,email{$user->id}"
        ];

        return Validator::make($user->toArray(), $rules);
    }

}

You can't do this with a static variable on a model.

// This will fail if you are updating an existing user.
public static $rules = array(
    'email' => 'required|email|unique,users,email',
);
@laravelbook
Copy link
Collaborator

@grbsn - You've proposed a nice design pattern! 👍
As for your original problem: Ardent provides a mechanism to override static rules. validate() and save() (and also forceSave() ) methods have the $rules parameter in which you may pass custom validation rules.

I've just posted a small snippet here

@shkm
Copy link
Contributor

shkm commented Apr 2, 2013

I look forward to such an elegant solution! For now, in order to keep validation in the model, I went ahead and overrode validate() with my own rules.

public function validate($rules = array(), $customMessages = array())
{
    if (empty($rules)) {
        $rules = array(
            'slug' => 'required|alpha_dash|between:1,128|unique:pages,slug,' . $this->id,
        );
    }

    return parent::validate($rules, $customMessages);
}

@ghost
Copy link
Author

ghost commented Apr 3, 2013

Would it be a good idea to remodel validation to follow this pattern?

@laravelbook
Copy link
Collaborator

@grbsn Sure! Please do go ahead and share your ideas...

I'm thinking Factory pattern here...

Here's some pseudo-code off the top of my head...

Interface definition:

public interface ICustomValidatorFactory {
    public function makeValidator(/* Eloquent */ $entity) { }
}

Implementation:

public class UserValidatorFactory extends ICustomValidatorFactory {

    public function makeValidator(\User $user) {
         $rules = [... custom validation rules ...];
         $customMessages = [... custom error messages ...];

         return \Validator::make($user->toArray(), $rules, $customMessages);
    }

}

Modifications to Ardent base class:

class Ardent
{
    public ICustomValidatorFactory $validatorFactory;

    public function validate( $rules = array(), $customMessages = array() ) {

        // auto-hydrate entity attributes from Input
        if ( $this->autoHydrateEntityFromInput ) {
            $attributes = array_intersect_key( Input::all(), $rules );
            foreach ( $attributes as $key => $value ) {
                $this->setAttribute( $key, $value );
            }
        }

        $validator = null;

        if ( !is_null( $this->validatorFactory ) ) {
            $validator = $this->validatorFactory->makeValidator($this);
        }

        if ( is_null( $validator ) ) {
            // fallback - instantiate the validator manually... (extra code removed for succinctness)
            $validator = Validator::make( $this->attributes, $rules, $customMessages );
        }

        $success = $validator->passes();
        // ... rest of the code
    }

}

Route/Controller code:
New user creation:

Route::post('create/new/user', function () {
    $user = new User;
    $user->save();
});

Modify existing user (uses custom validator):

Route::post('update/existing/user/(:num)', function ($id) {
    $user = User::find($id);
    $user->validatorFactory = new UserValidatorFactory();
    $user->update();
});

What do you think?

Update: It's not a proper factory method... but you get the idea...

@ghost
Copy link
Author

ghost commented Apr 4, 2013

I don't actually know how Ardent validates, do you call the validator on save() method? Something like this could be a possible implementation.

<?php namespace Ardent;

use Illuminate\Database\Eloquent\Model as Eloquent;
use Illuminate\Support\Facades\Validator;

abstract class ValidatorServiceProvider {

    public $model;
    public $rules;
    public $messages;

    public function __construct(Eloquent $model, $rules = [], $messages = [])

        $this->model = $model;
        $this->rules = $rules;
        $this->messages = $messages;
    }

    public function getMessages()
    {
        return $this->messages;
    }

    public function setMessages($messages)
    {
        $this->messages = $messages;
    }

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

    public function setRules($rules)
    {
        $this->rules = $rules;
    }

    public function getValidator()
    {
        return Validator::make($this->model->toArray(), $this->rules, $this->messages);
    }

}
<?php namespace Ardent;

use \User;

class UserValidator extends ValidatorServiceProvider {

    public function __construct(User $user)
    {
        parent::__construct($user);

        $this->rules = [
            'id' => 'required|integer'
        ];
    }

}

@laravelbook
Copy link
Collaborator

@grbsn Ardent->save() method invokes Ardent->validate() internally:

    public function save( array $rules = array(), $customMessages = array(), ... ) {
        $validated = $this->validate( $rules, $customMessages );
        // ... rest of the code
    }

Laravel comes pre-equipped with a powerful IoC container. We could leverage that class to inject custom validator providers into our model objects as needed (which may/may not render the factory pattern somewhat redundant...)

Here's a possible usage scenario...

// bootstrap code:
App::bind('existingUser', function($id) {
    $user = User::find($id);
    $user->validatorFactory = new UserValidatorFactory;
    return $user;
});

// usage:
Route::post('update/existing/user/(:num)', function ($id) {
    $user = App::make('existingUser', $id);
    $user->update();
});

... alternatively, dependency injection via constructor:

App::bind('ICustomValidatorFactory', 'UserValidatorFactory');

abstract class Ardent extends Model {

    public function __construct( array $attributes = array(), ICustomValidatorFactory $validatorFactory ) {
        parent::__construct( $attributes );
        $this->validationFactory = $validationFactory;
        $this->validationErrors = new MessageBag;
    }  
}

However, I'm not too fond of messing with the constructor... not sure if Laravels IoC provides DI via property (or "setter" method)..

@jrahmy
Copy link

jrahmy commented Apr 5, 2013

For dealing with the original issue, I do this in my edit routes:

$rules = Entity::$rules;
foreach ($rules as &$rule) {
    $rule = array_filter($rule, function($rule) {
        if (String::startsWith($rule, 'unique:')) {
            return false;
        }
        return true;
    });
}

if ($entity->save($rules)) { ...

Will probably move that to a static method in my base model at some point, and it requires array syntax for the rules.

@JonathanHindi
Copy link

Any updates on this issue... Facing the same problem.

@andrew13
Copy link
Contributor

Yeah this will alleviate an issue I'm having as well. For now I'm passing in a different rules array when updating.

@jrahmy
Copy link

jrahmy commented Apr 13, 2013

If you extend Ardent with your own model class, you can do..

    public function validate($rules = array(), $customMessages = array())
    {
        if (empty($rules) and $this->exists)
        {
            $rules = static::$rules;

            foreach ($rules as &$rule) {
                $rule = array_filter($rule, function($rule) {
                    if (String::startsWith($rule, 'unique:')) {
                        return false;
                    }
                    return true;
                });
            }
        }

        return parent::validate($rules, $customMessages);
    }

It will strip all unique rules if the model already exists in the database. All your models have to extend your custom model class instead of Ardent.

@ockam
Copy link

ockam commented May 18, 2013

CakePHP 2 use a nice pattern for that. You can specify if a rule apply:

  • only if field is present in model (required: false)
  • all the time (required: true)
  • on create or update ( required: create, required: update)

http://book.cakephp.org/2.0/en/models/data-validation.html#required

Don't know how this could be implemented in Ardent, but It could use a less confusing term than required.

@andrew13
Copy link
Contributor

I really like the cakephp 2 way

required | required:create | required:update

I would love to see that added

@cviebrock
Copy link

I'm just switching from L3 to L4 and came across Ardent. It sounds very much like a custom BaseModel class I use for many of my L3 projects. I too had this same uniqueness issue when validating, and here's how I solved it. Maybe not the most elegant, but feel free to steal what you think is useful.

The BaseModel class has static $rules, $messages, and $errors properties, just like Ardent. You are free to use unique validators in the rules. The method that does the validation takes care of the self-uniqueness issue:

    /**
     * Validate the Model
     *    runs the validator and binds any errors to the model
     *
     * @param $rules:array
     * @param $messages:array
     * @return bool
     */
    public function is_valid($rules=null, $messages=null)
    {

        // innocent until proven guilty
        $valid = true;

        // load defaults if none given
        if ($rules===null) $rules = static::$rules;
        if ($messages===null) $messages = static::$messages;

        if( !empty($rules) ) {

            // if model already exists, filter out any "unique" rules
            if ($this->exists) {

                foreach($rules as $attribute=>$rule_parts) {

                    // rules could be strings or arrays, so let's settle on one
                    if (!is_array($rule_parts)) {
                        $rule_parts = explode('|', $rule_parts);
                    }

                    foreach($rule_parts as $key=>$rule_part) {
                        if (starts_with($rule_part, 'unique')) {
                            $rule_parts[$key] = 'unique:'.$this->table().','.$attribute.','.$this->get_key();
                        }
                    }

                    $rules[$attribute] = $rule_parts;

                }

            }

            $validator = Validator::make( $this->attributes, $rules, $messages );

            $valid = $validator->valid();

            if ( $valid ) {
                $this->errors->messages = array();
            } else {
                $this->errors = $validator->errors;
            }

        }

        return $valid;
    }

@ghost
Copy link
Author

ghost commented May 24, 2013

Why not just use a validator service for each model. It keeps your model clean and you can use different validators if and when you need to. It also solves the problem with accessing model attributes from a static member and creates testable validation classes as you are using dependency injection to pass your model into the validator. For example, look at my UserValidator.php validator service.

@ghost ghost closed this as completed May 24, 2013
@russback
Copy link

russback commented Jun 7, 2013

The service looks like the way to go but I'm not entirely clear on how you'd use your example. For example you have a getValidator() method in your UserValidator example that demonstrates updating a user. How would it look for say creating a user - would there be another method or logic in the getValidator() method that determines which ruleset to apply?

I have a use case where a user profile is split into multiple forms so I need to validate only the data in each form, not the whole user model, so I'm thinking I'd need methods for create, updateStep1, updateStep2, etc.

The core validator ignores any attributes that aren't passed to the passes() method unless they are implicit rules, such as required. In this case, the validator will fail on account of the attribute not being present.

@ghost
Copy link
Author

ghost commented Jun 8, 2013

When I am using validators, I don't validate input, but the model after it has been filled by the input. For example, let's create a new user. You use the same rules when creating as when updating a user.

public function create()
{
    if (Request::method() == 'POST')
    {
        // Create a new user and fill the model.
        $user = new User(Input::get('user'));

        $validator = new Models\Services\Validators\UserValidator::getValidator($user);

        if ($validator->passes())
        {
            $user->save();
            return Redirect::route('users/show', [$user->id]);
        }

        return Redirect::back()->withInput()->withErrors($validator);
    }

    return View::make('users.create');
}

If you need to be able to validate a user at different steps, you may decide that you want to validate the input array after each form submission, then validate the user model once all of the stages have been complete.

If you wanted to have multiple validators for a model, there are many different approaches you can do. You could have two validator services, each in a different script or have many methods in your service, each with a different set of rules.

Hope this helps!

@russback
Copy link

When I am using validators, I don't validate input, but the model after it has been filled by the input. For example, let's create a new user.

I like that approach - not one that I'd thought of.

If you wanted to have multiple validators for a model, there are many different approaches you can do. You could have two validator services, each in a different script or have many methods in your service, each with a different set of rules.

Ah OK, so you're using a getValidator() method not because it's the only method that can be used but because it's the only one you need. So if I want to take the route of a single validation service for a model but with multiple rules, I can just have getCreateValidator(), getProfileUpdateValidator(), etc etc.

@ghost
Copy link
Author

ghost commented Jun 10, 2013

Exactly!

@benjaminkohl
Copy link

"Otherwise the validation rules will fail as the user you are updating as the same email address as the user you are updating."

You said it best. The unique rule in Laravel needs to be tweaked to automatically allow the updated record to have the same unique value as the updated record. Hehe.

@ghost
Copy link
Author

ghost commented Jul 14, 2013

The problem with that is, how would the validator decide whether the user already exists?

@travisolbrich
Copy link
Contributor

All you would need to do is see if the model has an 'id' field set.

Travis Olbrich

Date: Sun, 14 Jul 2013 13:33:43 -0700
From: notifications@github.com
To: ardent@noreply.github.com
Subject: Re: [ardent] Uniqueness (#37)

The problem with that is, how would the validator decide whether the user already exists?


Reply to this email directly or view it on GitHub.

@cviebrock
Copy link

$user->exists

On 2013-07-14, at 3:33 PM, George Robinson notifications@github.com wrote:

The problem with that is, how would the validator decide whether the user already exists?


Reply to this email directly or view it on GitHub.

@mspivak
Copy link

mspivak commented Oct 2, 2013

Why is this issue marked as "closed"? Is there a solution provided to this?

@igorsantos07
Copy link
Member

@mspivak I'm kinda maintaining the project now but had little time to work on it in the last month.
Looks like there was a lot of discussion in this issue already. Could you summarize the problem if it's important for you, be it here or into a new issue? So we can properly maintain the track on the project issues (:
I'll probably come back here next week for real work!

@mspivak
Copy link

mspivak commented Oct 3, 2013

@igorsantos07 Since in Ardent we set the rules in the model class (not the instance in a controller), when using the "unique" rule we need a way to skip the record being updated so the update doesn't fail. Traditionally we used a rule string like unique:users,email,9 where 9 was the id of the record to be excluded. Now with Ardent, we need a workaround to skip the record being updated.

Here's an awful one that I wrote to fix the issue on my end, but I'm sure you'll do it nicer: https://gist.github.com/mspivak/6816621 You just need to use a rule using a placeholder like unique:users,email,{id}. This will only work for one field primary keys named "id".

@andrew13
Copy link
Contributor

andrew13 commented Oct 5, 2013

You don't need to use a place holder. Ardent can handle that for you using updateUniques(), which will hopefully get changed back to update() #110 👍

@yosefflomin
Copy link

Might be missing something, but this seems like you can get an error back from the DB if the id was actually changed/updated, for some reason, and not validated to be unique.

Took a stab at it myself modifying the Ardent class. The below approach seems to cover the above scenario, would be happy to get some feedback on it.

public function validate(array $rules = array(), array $customMessages = array()) {
    if ($this->fireModelEvent('validating') === false) {
        if ($this->throwOnValidation) {
            throw new InvalidModelException($this);
        } else {
            return false;
        }
    }

    // check for overrides, then remove any empty rules
    $rules = (empty($rules))? static::$rules : $rules;

    // make sure primary key is validated
    if (!isset($rules[$this->primaryKey])) {
        $rules[$this->primaryKey] = 'unique:'.$this->table;
    }

    foreach ($rules as $field => $rls) {

        // check if unique fields have been modified or not, if not, exclude them from validation
        $uPos = strpos($rls, 'unique');

        if ($uPos !== false) {
            // didnt change
            if ($this->$field == $this->getOriginal($field)) {
                // remove the unique rule as field didnt change
                $uEPos = strpos($rls, '|', $uPos);
                $uEPos = $uEPos ? $uEPos : strlen($rls);
                $rules[$field] = substr($rls, 0, $uPos) . substr($rls, $uEPos);
            }
        }

        if ($rls == '') {
            unset($rules[$field]);
        }
    }
...

@simplenotezy
Copy link

Thanks @yosefflomin for above solution; it works like a flaw.

This should be merged into the main repo.

@igorsantos07
Copy link
Member

It seems this should be fixed by buildUniqueExclusionRules()

This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests