andrewelkins / Laravel-4-Bootstrap-Starter-Site

Laravel 4 Starter Site is a basic blog application using several nice community packages.
1.77k stars 608 forks source link

Update UserController.php #401

Closed narfed closed 9 years ago

narfed commented 9 years ago

Fixes bug where it would return success when there were validation errors. Probably a result of confide removing ardent dependency. Implemented by following Laravel 4.2 documentation for validator class.

narfed commented 9 years ago

noticed this bug while testing. copied the fix over manually so feel free to test and change as you see fit.

narfed commented 9 years ago

whoosp I guess I should leave it open

nextpulse commented 9 years ago

This is not the right fix. (You probably saw that I raised this issue already - before you 'noticed this bug while testing.'?? coincidence ;) ? )

The issue is due to front end form validation and backend validation.

Your 'fix' actually breaks the existing back end validation (ie. it will show success if the user name already exist).

If I find a solution, I will post it here.

narfed commented 9 years ago

Seems that startersite uses depreciated $getUpdateRules in confide. Update the depreciated $updateRules to match $rules in vender/../Confide/ConfideUser.php. This will add the unique rules to the username and emails.

narfed commented 9 years ago

I spoke too soon. After updating $updateRules, you won't be able to change password because the username and email will be non unique and you will get a validation error. You can add code to make this an exception I guess but I think it might be better to spend time looking at Confide v4.

nextpulse commented 9 years ago

Pretty messy for a 'starter project' ;)

Did a quick attempt to try use confide v4 - this opened up a whole can of worm. Probably require some time to port it over.

nextpulse commented 9 years ago

So a quick fix is to do two validation checks.

First for the backend validation: as the code is currently (before your fix). Then if successful [empty($error)], do a form validation via $user->errors()->all()

[or vice versa, form first and then backend]

narfed commented 9 years ago

You probably meant to include your fix from https://github.com/andrewelkins/Laravel-4-Bootstrap-Starter-Site/issues/400

FYI: 
For now, in order to progress (until the above is cleared up), I converted:

$error = $user->errors()->all()
to
$error = $validator->messages()->all()
nextpulse commented 9 years ago

Also, there are some restrictions with the current starter implementation for the User class. (See my comments for https://github.com/andrewelkins/Laravel-4-Bootstrap-Starter-Site/issues/403)

I've started to port over to Confide 4.0. But it looks like it will be almost a complete rewrite.

andrewelkins commented 9 years ago

@nextpulse If you'd like to migrate to 4.0 and submit a pull request I'd love to merge it. It actually does need to be rewritten. The code base is pretty old a this point. I started on v5 starter site, but maybe this one should get some love first.

narfed commented 9 years ago

Oh I guess I can close this. Just a note since getUpdateRules and setUpdateRules is depreciated in confide you can use $rules directly when you update to latest confide. Up to you if you want to place $rules in the controller or model.

narfed commented 9 years ago

I have no idea what I'm doing. It didn't get merged so I will reopen till it gets fixed.

nextpulse commented 9 years ago

@andrewelkins I started to port my local project over to confide 4. Its quite substantial - since there are now the concept of UserRepository. The UserController is a total rewrite. As of now - its going to involve a lot of work to separate this from my own project. Hence I would not feel comfortable to attempt to merge this back. However, if anyone is willing to spend the time to do it - I will be happy to give some pointers.

chrispappas commented 9 years ago

@nextpulse I would love to hear some of those pointers. Starting to look at porting over Confide 4 to this project. Any help you can provide would be great.

nextpulse commented 9 years ago

Here is what I suggest. Install this starter package. Then follow the install info for confide 4:

https://github.com/Zizaco/confide

Updating all configs as recommended. You can bypass any confide table migration - since the starter package tables are compatible.

Create the Employee controller:

php artisan confide:controller --name=Employee

Making sure you have the UserRepository class generated as well.

Now replace the UserController to match the EmployeeController where relevant (for example, signup etc).

That should get you started (I am still working on it - so I have not come across other issues yet. The forgot/confirmation email seems to work as is)

chrispappas commented 9 years ago

Thanks for the quick reply. Looking into this right now. Hopefully will have something to submit today!

andrewelkins commented 9 years ago

I think this is fixed by #407