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

[5.5] Optional parameter binding #17521

Merged
merged 2 commits into from Feb 3, 2017

Conversation

Projects
None yet
4 participants
@alepeino
Copy link
Contributor

alepeino commented Jan 24, 2017

(References Issue #13988)

Following @powelski 's reasoning, the behavior as implemented would be:

  • get corresponding model if exists
  • throw ModelNotFoundException if it doesn't exist (empty model or null when the request did have a parameter didn't make much sense to me)
  • bind null if no parameter is in URI (according to the default parameter value indicated by the developer).

(PR submitted to master even though it was tagged [5.3], for backwards compatibility)

@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Jan 25, 2017

How is this different than the other PR?

@alepeino

This comment has been minimized.

Copy link
Contributor Author

alepeino commented Jan 25, 2017

@taylorotwell you mean #16926? I guess there's some overlap (in fact I see now we have a conflict in a method) but this one was directed specifically at what we've been discussing at #13988, regarding the implicit binding of Eloquent models when the route as well as the controller method are defined with optional parameters.
I have not yet tried this other PR, particularly how it handles the case I mentioned, but it looks like it doesn't address this specific issue (would still bind an "empty" model instead of null, if I'm not mistaken).

@GrahamCampbell GrahamCampbell changed the title Optional parameter binding [5.5] Optional parameter binding Jan 25, 2017

@JosephSilber

This comment has been minimized.

Copy link
Contributor

JosephSilber commented Jan 25, 2017

empty model or null when the request did have a parameter didn't make much sense to me

Why? What if they want to show a custom 404 or some redirect?

I agree that injecting an empty model makes no sense.

@alepeino

This comment has been minimized.

Copy link
Contributor Author

alepeino commented Jan 25, 2017

@JosephSilber well, mostly for consistence with the current behavior. From the docs:

Laravel will automatically inject the model instance that has an ID matching the corresponding value from the request URI. If a matching model instance is not found in the database, a 404 HTTP response will automatically be generated.

If you want to do custom things, you can always customize the exception handler, or bind explicitly.

As I see it, optional parameter should mean "what to do when no parameter is supplied" (that is, bind null as implied by the developer in the controller method signature and let them handle that), not "what to do when a parameter is indeed supplied and then the database happens to fail in finding a corresponding model".

@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Jan 27, 2017

Can you provide a simple route and explicit binding an example I can paste into an app to see the currently broken behavior?

@alepeino

This comment has been minimized.

Copy link
Contributor Author

alepeino commented Jan 27, 2017

@taylorotwell Sure. I just tried this in a 5.3 application:

Route::bind('user', function($id, $route) {
    if ($id > 10) {
        throw new ModelNotFoundException();
    }

    return new User(['email' => $id]);
});

//

Route::get('/test/{user?}', function (User $user = null) {
    dd($user);
});

For /test/1 I got User ('email' => '1') (as expected).
For /test/11 I got 404 (as expected).
For /test I got "empty" User. (Not saying it's broken behavior, just it might be better that null is returned. By the way, notice in this case the binding function is not called.)

@cgoosey1

This comment has been minimized.

Copy link

cgoosey1 commented Jan 27, 2017

The empty models was on purpose to stick with current behaviour. :)

@taylorotwell taylorotwell merged commit 391d960 into laravel:master Feb 3, 2017

2 checks passed

continuous-integration/styleci/pr The StyleCI analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Feb 3, 2017

I think this makes sense.

@alepeino alepeino deleted the alepeino:optional-parameter-binding branch Feb 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment