Updated unique fields

[chrissm79]

I must say that I'm a huge fan of Ardent. Just started playing around with it today and it's been a breeze!

The only issue I've run into is updating models when there is a unique field (for example, email address). When I attempt to save the model without updating the unique field, I get an error stating that there is a duplicate in the database. I'm likely doing something wrong, but is there a way to update models that have unique fields w/out doing a force save?

[travisolbrich]

I'm actually working on this same issue right now. This issue is that you need to add the id of the record you are editing to the rule. See: [http://four.laravel.com/docs/validation#rule-unique] The solution I have is a little helper function I made that will go through all of the rules you made for a model, find 'unique' rules, and automatically add the ID of what you are editing to the rule. Then you can take what the helper spit out and feed it to save().

I'm going to finish testing up this function and I'll put it up tomorrow if you would like to use it.

[travisolbrich]

Here's what I've got, feel free to make any suggestions you like!

Code: https://gist.github.com/travisolbrich/5795122 Test: https://gist.github.com/travisolbrich/5795134

I had considered forking Ardent and working this in as a object method. Thoughts?

[kernel-io]

@travisolbrich The update() method that was added as a result of your changes, does not share the same footprint as the laravel update method, It would be great if you would let the first variable be an array of attributes, and if it is not present, then continue on as you currently do.

This would allow us to do something along the lines of

$object->update(Input::only('example_a','example_b'));

instead of the way we have to do it now which is

$object->fill(Input::only('example_a','example_b'))->update();

[travisolbrich]

Oops, my apologies. I didn't realize this was already in Eloquent. I'll be sure to check next time.

Wouldn't it be better to change the name of my method to something different altogether since Eloquent's update() and my update() do different things?

[kernel-io]

Depends, personally I like the fact that your method gets rid of the unique validations, I think that is something that eloquent should have inbuilt (imho, Eloquent is kinda lacking in certain 'common sense' features).

maybe you could just integrate the laravel method into the start of yours, if you want I can submit a PR with the changes (that shouldn't break the current API you have)..

[travisolbrich]

Great, please go ahead and submit one. I'm in the midst of a move and can't really write code this week.

Thanks!

[kernel-io]

Ok the PR I will submit will break the API, I figured there isnt really a way to tell either type of array from each other (rules versus updated attributes), is that fine with you?

[travisolbrich]

Works with me, but I still think it would be a better idea to change the name of it altogether. Less confusion that way, I think.

I really wish the $rules was not static, this would make it a bit easier.

[kernel-io]

suggestion for a new name then? update() is what it does, updateAndSave()?, updateRecord()?

[travisolbrich]

Should it say something about the exclusions or uniqueness?

[kernel-io]

updateAndIgnoreUniqueRulesOnTheModel() ? :p

I don't think it really needs to, the update name itself to me just implies that it will do a SQL update as opposed to a SQL INSERT (which is how I stumbled upon the Eloquent method)..

[travisolbrich]

What about building it in to beforeSave()?

[igorsantos07]

I've noticed the code you've been talking about in this issue is already in Eloquent, in the method buildUniqueExclusionRules() and updateUniques(). Also I noticed the original method was only update(), but I got somewhat lost with the updates. @travisolbrich have written an update() method that is incompatible with the original's Eloquent method and thus it was renamed into a specific method to be used only when there's rules for unique attributes? To me it seems pretty much useless, since you have to use a specific different method in some cases.

Did I got things right? If yes, there may be a way to merge both signatures into one?

[igorsantos07]

I was wondering... maybe we could hook up buildUniqueExclusionRules() into the save() method? Since it's called by update(), whenever save() was triggered by this method it would clean the unique rules and fix stuff; if save() was called by a new object it would simply go ahead.

What do you think @travisolbrich ?

[igorsantos07]

Additionally, this could also be achieved including an internal beforeValidate hook that would change the rules as needed. This could be even cleaner as long as we can tell if the data is being saved or updated.

[travisolbrich]

buildUniqueExclusionRules was actually a function I put in to Ardent, not Eloquent. I like the idea of building it in to Save, especially if you can still override it by putting in your own $rules.

[igorsantos07]

Ops, I meant Ardent! Haha

I'll take a look in this soon. Would you mind reviewing the changes later?

[nightowl77]

Just ran into this exact issue myself. Since we are gluing Ardent and Eloquent together, shouldn't we add this functionality out the box. For example, on line 514 https://github.com/laravelbook/ardent/blob/master/src/LaravelBook/Ardent/Ardent.php#L514 of Ardent.php

if ($this->exists)  // ie, we are doing validation on an update
  $rules = $this->buildUniqueExclusionRules($rules);

We then need to change function buildUniqueExclusionRules to accept a $rules parameter. I see that it wants to call the static rules. Not sure why it is like that but I'm sure @igorsantos07 had a good reason for that. If you could bring me up to speed I'd appreciate that.

With that change uniques would work without the programmer having to spend extra time building "beforeValidate" code for every unique he has. It would also future proof code, for example, in Laravel 5 they decide that the unique/update situation must be handled differently. Ardent code might have a easy upgrade path as Ardent automatically hides these complexities.

Would this change be something you guys are interested in? Unless I'm missing something important, I can fork and PR right now.

[igorsantos07]

@nightowl, I'll be unable to work on Ardent code until later this week, sorry. On top of that, I'm not the original coder of that part of Ardent so I still need to take a careful look at it to see what it does and how it could change - that's why this issue is still open. Maybe you may feel more comfortable with it, so if you're interested in, fork and PR (: On Jul 25, 2013 9:43 AM, "nightowl77" notifications@github.com wrote:

Just ran into this exact issue myself. Since we are gluing Ardent and Eloquent together, shouldn't we add this functionality out the box. For example, on line 514 https://github.com/laravelbook/ardent/blob/master/src/LaravelBook/Ardent/Ardent.php#L514of Ardent.php

if ($this->exists) // ie, we are doing validation on an update $rules = $this->buildUniqueExclusionRules($rules);

We then need to change function buildUniqueExclusionRules to accept a $rules parameter. I see that it wants to call the static rules. Not sure why it is like that but I'm sure @igorsantos07https://github.com/igorsantos07had a good reason for that. If you could bring me up to speed I'd appreciate that.

With that change uniques would work without the programmer having to spend extra time building "beforeValidate" code for every unique he has. It would also future proof code, for example, in Laravel 5 they decide that the unique/update situation must be handled differently. Ardent code might have a easy upgrade path as Ardent automatically hides these complexities.

Would this change be something you guys are interested in? Unless I'm missing something important, I can fork and PR right now.

— Reply to this email directly or view it on GitHubhttps://github.com/laravelbook/ardent/issues/52#issuecomment-21550980 .

[nightowl77]

No stress. I'm more than happy to fork and code it myself, maybe even throw in a few tests and a travis build file (I've never used travis before and always wanted to play with that).

I'd just like to know if this is a worthwhile issue to pursue (ie, make Ardent handle uniques-on-update automatically behind the scenes). I'm trying to rack my brain for a use-case where a unique should ignore it's own ID when doing an update.

[igorsantos07]

Please, go ahead with the tests (:

And I think it makes a lot of sense for ardent to fix the unique rules on update cases. Simple case: user with email. The email must be unique, but on an update the email will be there for that very entry, but shouldn't be present in other entries. On Jul 25, 2013 11:18 AM, "nightowl77" notifications@github.com wrote:

No stress. I'm more than happy to fork and code it myself, maybe even throw in a few tests and a travis build file (I've never used travis before and always wanted to play with that).

I'd just like to know if this is a worthwhile issue to pursue (ie, make Ardent handle uniques-on-update automatically behind the scenes). I'm trying to rack my brain for a use-case where a unique should ignore it's own ID when doing an update.

— Reply to this email directly or view it on GitHubhttps://github.com/laravelbook/ardent/issues/52#issuecomment-21556680 .

[nightowl77]

Hi @igorsantos07

I have done the changes, ran a few manual tests, and it seems everything's working great. Pull request above.

I have done the following:

1. To make it backwards compatible (incase devs already pass a 3rd parameter) Adrent will not overwrite that last 3rd parameter if it exists.
2. There was a small bug in buildUniqueExclusionRules. You referenced param[3] instead of param[2] - the code worked because you imploded it, but it had to be fixed in order for change (1) mention above to work
3. I tried to keep compatibility with the ReflectionClass stuff you are doing in line 767 by wrapping it in an IF and making the function parameter optional. I'm not sure where that is used but I suspect it has something to do with the non-laravel stuff you are building
4. To future proof the code even more I added a autoUniqueUpdates property which by default is true. If any developer wants to manage his unique himself, he can simply in his extended class set this property to false.

As for tests.... ummm... yea. I thought it might a quick thing, but writing tests seems to be almost a project by itself. I'm used to tests for the frontend which is pretty straight forward and fast to code, but package testing is a complete different ballgame. I understand how Mockery works and it's role in package testing, it's just writing all those different mock calls that will be made that will be very time consuming. Two or more devs should have a code sprint weekend, because this seems to be quite a lot for a single developer to take on.

Please let me know if you are happy with the PR.

[igorsantos07]

The discussions got a little mixed up, sorry for not placing everything here.

I created the RFC #84 where we can discuss better ideas to clean up code and consequently improve handling of those issues.

@travisolbrich thinking a little more about this issue, I think the fix of validation of uniques should happen inside the validate method, identifying if it's an update and then fixing accordingly... Not sure yet though, would see when working in the code.

[ronlobo]

Does anyone know how to use the unique rule for a specific foreign key match only?

Tried something like:

 public function update($id) {
$collection = Dashboard::find($id);
    DashboardCollection::$rules['name'] = DashboardCollection::$rules['name'] . ',' . $collection->id .',id, user_id,' . Auth::user()->id_user;

....
}

I'm updating the rule in the update function, because of the stuff mentioned above. So it only uses the unique rule, if the user id matches the foreign key user_id in the Collection table?

Thanks in advance!

Greets Ron

[igorsantos07]

I guess this is/will be fixed by the PR #86, so I'm closing this issue to focus discussion there... If I'm doing shit please let me know haha