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

Remove route model binding for users #3088

Merged
merged 7 commits into from
Jan 12, 2021
Merged

Remove route model binding for users #3088

merged 7 commits into from
Jan 12, 2021

Conversation

duncanmcclean
Copy link
Member

This pull request resolves #2316, where issues crop up in existing Laravel apps if people are using route model binding to bind to their User model.

This PR just straight out removes the route model binding and replaces it with a lookup in the controllers where it is referenced with Entry::find($id).

While working on this PR, I've also found posts on the Discord by some developers complaining that Statamic's route model binding is a bit generic and can cause issues if used side by side on an existing Laravel app. I'm happy to make any changes to any of the other route model binding stuff if that's something you'd like me to do.

Let me know if you have anything you'd like me to change.

sorry styleci
@duncanmcclean
Copy link
Member Author

Instead of removing route model binding entirely, maybe we could just rename the route model binds to being Statamic specific, like statamic_user, etc which would resolve any Statamic binding issues. Feel free to decline this PR if you'd like me to do that instead.

(thanks @edalzell for that idea)

@riasvdv
Copy link
Contributor

riasvdv commented Jan 11, 2021

To be functionally the same as route model binding you need to do User::findOrFail($id)

@duncanmcclean
Copy link
Member Author

To be functionally the same as route model binding you need to do User::findOrFail($id)

Ah, good point! The only issue is... there's no findOrFail method on the UserRepository, see here.

@edalzell
Copy link
Contributor

Also see here: #3055

Copy link
Member

@jasonvarga jasonvarga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

404s need to be thrown when the user in the url doesn't exist. You can copy the logic that you removed from the service provider.

@duncanmcclean
Copy link
Member Author

404s need to be thrown when the user in the url doesn't exist. You can copy the logic that you removed from the service provider.

Added the throw_unless to the controllers.

Copy link
Member

@jasonvarga jasonvarga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either remove the messages from the exceptions, or make sure you're passing the handle in from the route argument.

Right now since you're using $user = User::find($user) before new-ing up the exception, the message will be User [] not found. but it should be User [whatever] not found.

Since you don't ever see the exception messages, it's probably fine to just remove them.

@duncanmcclean
Copy link
Member Author

Either remove the messages from the exceptions, or make sure you're passing the handle in from the route argument.

Right now since you're using $user = User::find($user) before new-ing up the exception, the message will be User [] not found. but it should be User [whatever] not found.

Since you don't ever see the exception messages, it's probably fine to just remove them.

I've removed the exception messages.

@jasonvarga jasonvarga merged commit ce5ef7c into statamic:3.0 Jan 12, 2021
@duncanmcclean duncanmcclean deleted the fix/route-model-binding branch January 12, 2021 20:49
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.

Remove user route model binding
4 participants