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

Use custom TeamInvitation model in TeamInvitationController #892

Merged
merged 1 commit into from
Sep 28, 2021
Merged

Use custom TeamInvitation model in TeamInvitationController #892

merged 1 commit into from
Sep 28, 2021

Conversation

ManuelLeiner
Copy link
Contributor

Description

Use the custom TeamInvitation class in the accept and destroy methods of the TeamInvitationController to retrieve the invitation model.

Motivation

Fixes #890.

Solution

Removes the type-hint from the controller methods. Therefore the model will not be resolved by the framework and the invitation id will be passed to the controller. Uses the invitation id and the Jetstream helper to query and retrieve an instance of the custom TeamInvitation. Uses firstOrFail as the route model binding responds with a ModelNotFoundException as well when an invalid id is passed.

@ManuelLeiner ManuelLeiner changed the title Use custom model in controller Use custom TeamInvitation model in TeamInvitationController Sep 28, 2021
Copy link
Member

@driesvints driesvints left a comment

Choose a reason for hiding this comment

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

I also think this is probably the best solution.

@taylorotwell taylorotwell merged commit aba641d into laravel:2.x Sep 28, 2021
@driesvints
Copy link
Member

Thanks @ManuelLeiner

@ManuelLeiner
Copy link
Contributor Author

@driesvints I actually think that this PR might have been merged to early.

As I mentioned in #890 there might be a potential breaking change as soon as someone extends from the Jetstream TeamInvitationController and overrides one of the two methods.

image

I quickly added an example controller.

What do you think? Might solution 1 (route model resolving) be safer? Should we roll back and add the change to the next major release? Or am I overthinking?

@driesvints
Copy link
Member

@ManuelLeiner let's tackle that when someone actually reports an issue there.

@freekmurze
Copy link
Contributor

freekmurze commented Sep 30, 2021

@driesvints I got bitten by this. It's a breaking change indeed. Luckily I did have tests to notify me of this change.

For me this is fine, but I guess for people that didn't write tests for their custom endpoint, this will silently break in their production environments.

@driesvints
Copy link
Member

@freekmurze sorry about that. We reverted this PR.

@ManuelLeiner it's probably best that we retarget master for this.

@ManuelLeiner
Copy link
Contributor Author

@driesvints I got bitten by this. It's a breaking change indeed. Luckily I did have tests to notify me of this change.

For me this is fine, but I guess for people that didn't write tests for their custom endpoint, this will silently break in their production environments.

@freekmurze sorry about that. We reverted this PR.

@ManuelLeiner it's probably best that we retarget master for this.

Hey @driesvints, @freekmurze,

sorry for the inconveniences.

Of course the next major release is perfectly fine for me/us. Anything I can do? New PR based on master?

@driesvints
Copy link
Member

New PR based on master?

Yeah probably best

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.

TeamInvitationController not using custom TeamInvitation model
4 participants