Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[9.x] Let MustVerifyEmail to be used on models without id as primary key #44613

Merged

Conversation

arturzealves
Copy link
Contributor

@arturzealves arturzealves commented Oct 16, 2022

Description

I decided to change the primary key of my User model, from id to uuid and after that change, one of the feature tests started to fail.

I don't have the initial error anymore but I had to override this route on my routes/web.php:

Route::group(['middleware' => config('fortify.middleware', ['web'])], function () {
    $verificationLimiter = config('fortify.limiters.verification', '6,1');
    
    // Email Verification...
    if (Features::enabled(Features::emailVerification())) {
        Route::get('/email/verify/{uuid}/{hash}', [VerifyEmailController::class, '__invoke'])
            ->middleware([config('fortify.auth_middleware', 'auth').':'.config('fortify.guard'), 'signed', 'throttle:'.$verificationLimiter])
            ->name('verification.verify');
    }
});

only because I needed to change the route from /email/verify/{id}/{hash} to /email/verify/{uuid}/{hash}.

After that change, I was still having this error:
Screenshot 2022-10-16 at 11 17 25

So after some debug I found the source of the issues and the change I made on this pull request should allow other people on a similar situation, to change the name of the primary key to another name other than id without being backwards incompatible.

I'm not sure if this pull request should also have a fix for the route I mentioned above, but I'm ok with having an override on the routes/web.php.

@taylorotwell taylorotwell merged commit 1469ff7 into laravel:9.x Oct 17, 2022
@mtawil
Copy link
Contributor

mtawil commented Oct 20, 2022

This is a breaking change @taylorotwell!

@arturzealves This is a route function, and it's not related to ORM.
Changing the route parameter name is not related to your model changes or column name changes, so whatever changes you've made, you have to know what the issue is and fix it on your own.

My registration page is now crashed because of this change, and I've to override the fortify route file and change the {id} to {user_id} so it will fit this PR! That doesn't make any sense!

@mtawil mtawil mentioned this pull request Oct 20, 2022
taylorotwell pushed a commit that referenced this pull request Oct 20, 2022
@arturzealves
Copy link
Contributor Author

arturzealves commented Oct 20, 2022

Sorry for causing you issues @mtawil and @taylorotwell.

Well to me it doesn't make sense to have that route with an hard coded index id which will require the $notifiable to have the property id as well, when we have the chance to make it all dynamic and use the primary key name right away.

I was not expecting to have my first pull request approved right away and I was thinking that there was still something to change regarding the route part, to also make it dynamic and avoid having to override it, as I described in this PR description.

Do you have any suggestion for my issue then? I don't have an id on my User model anymore since I changed it to uuid.
Before writing this PR I had to create a new Trait similar to VerifyEmail and another class similar to this one, MustVerifyEmail, attach it to my User in order to make it work with the uuid as primary key.

@GrahamCampbell GrahamCampbell changed the title Let MustVerifyEmail to be used on models without id as primary key [9.x] Let MustVerifyEmail to be used on models without id as primary key Nov 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants