Logic for updateUniques fixed

[nightowl77]

Hi Igor

I'd really appreciate if you could look at this for me. The logic in updateUniques seems a bit flawed. On the one hand the function allows the programmer to pass in $rules, but the moment you do that, updateUniques becomes nothing more than a save function and buildUniqueExclusionRules never gets called.

This PR makes updateUniques work in all cases.

I also removed some of that Reflection stuff you used and shortened it a bit.

I would really appreciate it if you could push this through. I did test it manually and it worked fine. I'm helping with the cleanup of Confide at Zizaco/confide#124 and I need buildUniqueExclusionRules to be a bit more flexible.

Thank you

[Sylph]

It'll be nice to have it take into account custom primary keys as well. I had to do a quickfix to make that work.
Also, the implode creates 'unique,' rather than 'unique:', which is broken.

Example of a rough quickfix: http://pastebin.com/cd3K6Nem

[nightowl77]

Thank you @Sylph

I didn't even know that the Validator has additional support for custom primary keys (it's not documented).

If have implemented your code and I have updated the README.md file. You can read the new docs here https://github.com/nightowl77/ardent#uniquerules

Tests I have done

Test 1: Most often used use-case

When you define this rule in your model

public static $rules = array(
     'email' => 'required|email|unique:users,email',
);

the array generated by buildUniqueExclusionRules is

Array
(
    [email] => Array
        (
            [0] => required
            [1] => email
            [2] => unique:users,email,7,id
        )
)

Test 2: No table name in rules

public static $rules = array(
     'email' => 'required|email|unique',
  );

Resulting array from buildUniqueExclusionRules

Array
(
    [email] => Array
        (
            [0] => required
            [1] => email
            [2] => unique:users,email,7,id
        )

)

Test 3: Custom Primary Key

When adding a custom primary key

protected $primaryKey = "userID";

buildUniqueExclusionRules returns

Array
(
    [email] => Array
        (
            [0] => required
            [1] => email
            [2] => unique:users,email,7,userID
        )

)

[markalanevans]

@igorsantos07

It seems the issue of uniques is in multiple "issues"
#52
#94
and of course this one: #86

So it looks like the fix has been applied:

https://github.com/nightowl77/ardent/blob/9430959f0d84f49cf1aef2aeb4e16b1237ed4d63/src/LaravelBook/Ardent/Ardent.php#L811

Am i correct? So if we ran composer update should this problem go away?

[tortuetorche]

Am i correct? So if we ran composer update should this problem go away?

@markalanevans Yes, you are.
But you need to specify in composer.json file the nightowl77 fork URL:

{
  ...
  "require": {
    "laravel/framework": "4.0.*",
    ...
    "laravelbook/ardent": "dev-master",
  },
  ...
  "repositories": [{
    "type": "vcs",
    "url": "https://github.com/nightowl77/ardent"
  }],
  ...
}

[nightowl77]

@tortuetorche That is correct. Thank you

To the developers of ardent. Are you guys still out there? This issue has now been open for more than a month. We had multiple developers working on it, we had multiple issues about this, we have other projects like Confide waiting for this patch to be accepted. I wrote documentation and did everything I could. Did I waste my time for nothing?

I kindly request an update on why there is a hold-up. Surely pressing the "accept pull request" button would not take up that much time. Maybe this is not the direction you want to go in. I respectfully accept that, but If that is the case, please give me the specs and I will gladly code it for you exactly according to your specs.

To everyone waiting for this patch. Please change your composer.json file like @tortuetorche explained above. Please test my changes and report back on this thread if everything worked fine. The more positive feedback we get on this, the more comfortable the developers would be to accept this patch.

[markalanevans]

ok. Thanks @nightowl77 .

It does seem like the laravelbook team is busy ....

[nightowl77]

I think we can in all fairness say this project has been completely abandoned. I understand that people get busy and there is absolutely nothing wrong with that. We all have lives and other projects. We all understand that this is for free. We (myself included) are all happy to contribute our own free time for the greater good.

At the very least they can post something along the lines of "Sorry guys we are busy, would someone like to take this over". I do not expect the developers of ardent to give any more time than what they have already given. I do however expect them to give us the avenues to fix the problem. This is after all what the whole open source movement is about. Right now we are "CodeBlocked" and they refuse to give us any feedback whatsoever.

I repeat my call for all developers having a problem with updateUniques. Pull my version, test it, report back here.

[Sylph]

Thanks for adding that, nightowl7. I might use ardent for my next project thanks to that and maybe even contribute, though this 'CodeBlocked' issue is making me question the future of the project.

[markalanevans]

Perhaps we should create a separate github account like laraveltools and then fork this into it and call it Ardent2 or something.

Then lets clean this ish up.

There are alot of basic things it doesn't do, and a few of things it should NOT do. Hydration being one of them.

-Mark

[tortuetorche]

@nightowl77 I have a bug when my model has a custom primary key:

class Tweet extends Ardent
{
    public static $rules = [
        'type'    => 'required',
        'title'   => 'required|unique:tweets',
    ];

    protected $primaryKey = 'title';
}

$t1 = new Tweet( array('type'=> 'text', 'title' => 'first tweet') );
$t1->save(); //-> true

$t2 = new Tweet( array('type'=> 'text', 'title' => 'second tweet') );
$t2->save(); //-> true
$t2->title = "first tweet";
$t2->updateUniques();//-> true; but it should return false !

Otherwise your version is OK for me. Thank you !

[tortuetorche]

And one more thing, Ardent lacks of validateUniques() method, do you plan to add one ?

[noguespi]

Good job @nightowl77 and @Sylph it was an anoying bug and an important security issue. Hop this will be pulled in the main repo ASAP.

[Sylph]

@tortuetorche, sorry I barely wrote/tested that code, before deciding not to use ardent for my project. If I get back to using ardent and no one have fixed it, I'll fix it.

[andrew13]

@igorsantos07 Can we get some info on this one?

[igorsantos07]

@andrew13 soon! I'm back in activity and I'll start to clean up all those issues this week! Sorry for such a delay, moving to another country is no easy task :(

[igorsantos07]

@nightowl77 Hey man, sorry for the wait :(
I did not read all the discussion and comments yet but I did read the issue. Tomorrow I'll push your changes into a new version, trusting your manual tests and the fact this code looks like is being used in Confide already, right?

I'm URGING for some automated testing so I won't be afraid of new code anymore! I hope I can get to work on them this week. Thanks for the patience if you're still there!

[thomaswelton]

@igorsantos07 @nightowl77 Sorry guys, just trying to catch up on all the threads about validation for update on unique fields.
Was this supposed to be fixed? I see there have been recent changes to the repo about 6 hours ago and a few PRs merged. But I'm still having the same issue. Any guidance?

[thomaswelton]

Sorry the post just above mine confused me as it said closed, but I see this PR is still open.
Using the branch by @nightowl77 fixes my issues, so thanks for that.
Looking forward to seeing this merged. 👍

[igorsantos07]

Weirdly merged in the command-line. I'm going to release a new version soon! As this is just fixes, it should be 2.0.X, right?

[andrew13]

IMO it should be 2.1.x. Usually the third is a hot fix version, whereas, the second is a minor release which this would fall in to.

X.x.x being breaking changes and/or major functionality changes and/or major code changes
x.X.x being bug fixes, minor improvements
x.x.X Hotfix, Security release, release went out with a major issue, etc

[andrew13]

and AWESOME! 👍

[igorsantos07]

Oh, I forgot. There's new arguments in a method, right? I was considering this as a bug-fix only release. Thanks for the tip (:

[igorsantos07]

If anyone could give me a better description for 2.1.0, please write it here (:

[andrew13]

I would say "Improvements to validation rule 'unique' handling"

[nightowl77]

Hi @igorsantos07

Thank you for getting back to us.

I looked at the latest version of ardent and all the code written by @Sylph is still not in master. I'm not sure where it disappeared to (could have been my mistake, I'm not quite a Git Ninja yet), but from the history it does seem like you have the commit message "Added changes as per Sylph's post. Now adds support for custom primary keys, auto population of table names"

Code from my repo has support for primaryKeys:

https://github.com/nightowl77/ardent/blob/master/src/LaravelBook/Ardent/Ardent.php#L750

The current master of Ardent has

https://github.com/laravelbook/ardent/blob/master/src/LaravelBook/Ardent/Ardent.php#L754

If I did something "git commit" related to make this code disappear, please let me know as I'd like to avoid it in the future.

Thank you

[andrew13]

@nightowl77 I'm willing to bet it was just missed in the merge. "Weirdly merged in the command-line." @igorsantos07 I'd suggest a hotfix. #114

[igorsantos07]

Thanks Andrew for the followup and for understanding exactly the "weirdly
merged" part :P
I'll release this soon!
On Oct 21, 2013 5:01 PM, "Andrew" [email protected] wrote:

I'm willing to bet it was missed since he phased the merge as "Weirdly
merged in the command-line." @igorsantos07https://github.com/igorsantos07I'd suggest a hotfix.
#114 #114

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/86#issuecomment-26745920
.

[andrew13]

👍