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

[Suggestion] renaming of the users table #591

Closed
WisdomSky opened this issue May 16, 2017 · 10 comments
Closed

[Suggestion] renaming of the users table #591

WisdomSky opened this issue May 16, 2017 · 10 comments

Comments

@WisdomSky
Copy link

WisdomSky commented May 16, 2017

When I was renaming the default users and password_resets tables into something else and then when I try to register an account, I then encountered an error:

screenshot

It seems that the query still uses the default table name, and when I went to the RegisterController.php,
the problem was caused by the validator rule unique:users for the email field. And to fix it, we need to update the validator rule to use the new table name.

What I'm trying to suggest is maybe we can make it a little more dynamic rather than hardcoding it.

Since the Registration Controller is somehow tied with the app/User model, I'm wondering if we can include a method in the model like:

    public static function getTableName()
    {
        return (new static)->table;
    }

and update the validator method in the RegisterController.php into something like:

    protected function validator(array $data)
    {
        return Validator::make($data, [
            'name' => 'required|string|max:255',
            'email' => 'required|string|email|max:255|unique:'.User::getTableName(),
            'password' => 'required|string|min:6|confirmed',
        ]);
    }

or simply:

    protected function validator(array $data)
    {
        return Validator::make($data, [
            'name' => 'required|string|max:255',
            'email' => 'required|string|email|max:255|unique:'.(new User())->getTable(),
            'password' => 'required|string|min:6|confirmed',
        ]);
    }

or any better ideas?

@laurencei
Copy link

The idea of the RegisterController is you can override any function you want.

So you can just override it and set your own validation rule?

@ConnorVG
Copy link

I do believe this probably makes more sense to have it automatically figure out the users table from the controller stub (for this specific issue) – that said, if you're changing your default table name then I guess it's sort of assumed that you understand more about how this works than your average Joe.

How difficult is it to get the users table from the default guard? We could always just:

attain current / default guard
grab it's user provider
retrieve it's model
use the model's table

On that note... I always thought it'd be nice to be able to call something like Rule::unique(User::class, $user->id) etc etc. This makes for a decent use case.

@timacdonald
Copy link
Member

@ConnorVG it's a bit thrown together for my projects, but I've just added your unique suggestion to this rule builder package if your interested in being able to do that. Sorry to highjack the thread with unrelated stuff.

@ConnorVG
Copy link

@timacdonald that is amazing dude – any plans to PR this sort of extension into laravel/framework?

@timacdonald
Copy link
Member

@ConnorVG kinda, but kinda not. Needs more fleshing out and what-not before it'd be a valid candidate + I think I saw there is going to be a new Rule class in 5.5 on twitter + it's so easy to just do stuff the 'stringy' way, I think majority of people would be against this being in the core...probs.

@ConnorVG
Copy link

I think it's more about extending the current Rule class that's already implemented. It won't take much to add the support here, I don't think: https://github.com/laravel/framework/blob/5.4/src/Illuminate/Validation/Rule.php#L56-L66

@timacdonald
Copy link
Member

Oh, totally, yea, for just the built in unique rule, yea. I thought you meant the whole package haha. I'd love someone to PR it, I might if I get time, but would love someone else to if they have the time for sure.

@ConnorVG
Copy link

It might even make sense just to support: Rule::unique($user) – all the information is available there to do it.

@timacdonald
Copy link
Member

yea, my package allows a table string, class string or Model instance. Makes it very nice.

@timacdonald
Copy link
Member

@ConnorVG Submitted an internals proposal to see community feedback and so we can stop polluting this thread (sorry everyone).

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

5 participants