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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[7.x] Fix null value injected from container in routes #31867

Merged
merged 2 commits into from Mar 12, 2020

Conversation

@mathieutu
Copy link
Contributor

mathieutu commented Mar 9, 2020

Hi,

This PR fixes laravel/ideas#2114 (no idea why this bug have been moved to ideas, maybe I haven't made me understood 馃槙)

Context:
When injecting something witch can be null, the \Illuminate\Routing\RouteDependencyResolverTrait::resolveMethodDependencies method doesn't add it to parameters to inject, because it can't difference it from void return.

I've added a default return value, to indicate that we want to strip the value, to allow null value.

  • This PR should not be a breaking change: all tests pass, and the previous behavior was not changed.
  • A test to address this case was added.

You can reproduce the bug with:

class AppServiceProvider extends ServiceProvider
{
    public function register()
    {
        $this->app->bind(Service::class, fn() => null);
    }

    public function boot()
    {
        Route::get('/foo/{id}', function (?Service $service, string $id) {
            // Argument 1 passed ... must be an instance of Service or null, string given.
        });

        Route::get('/not-working', function (?Service $service) {
            // Too few arguments ... 0 passed ... and exactly 1 expected.
        });

        Route::get('/working', function (?Service $service = null) {
            // Working
        });
    }
}

And see the errors thrown:

Thanks.

Matt'

PS: If you read this, and can answer to this https://twitter.com/mathieutu/status/1236997027122249728?s=20, I would like to propose a PR for the router with a quite important breaking change, and so understand why the behavior of Laravel routing is like that, and why it is different from standard one.
Thanks.

@mathieutu

This comment has been minimized.

Copy link
Contributor Author

mathieutu commented Mar 9, 2020

StyleCI's not happy, but I'm not comfortable with the change proposed, as it makes the function not understandable.

-        $container->bind(RoutingTestUserModel::class, function() {
-            return null;
+        $container->bind(RoutingTestUserModel::class, function () {
         });
@mathieutu mathieutu changed the title Fix null value injected from container in routes. [7.x] Fix null value injected from container in routes. Mar 9, 2020
@GrahamCampbell GrahamCampbell changed the title [7.x] Fix null value injected from container in routes. [7.x] Fix null value injected from container in routes Mar 9, 2020
@GrahamCampbell

This comment has been minimized.

Copy link
Member

GrahamCampbell commented Mar 9, 2020

Laravel's code standard is like this because when a fucntion that has no return statement is called and the result is used or assigned to a variable, it is the same as using the value null in its place. The actual return statement can be optimized away.

@deleugpn

This comment has been minimized.

Copy link
Contributor

deleugpn commented Mar 10, 2020

I actually tried to fix this about 2 weeks ago and my patch was so bad that I never sent it. This looks like a really good patch with minor code changes to fix a moderate problem.
As someone who failed to find a good solution for this, I can appreciate the nice job you did there.

@mpyw

This comment has been minimized.

Co-Authored-By: mpyw <ryosuke_i_628@yahoo.co.jp>
@taylorotwell taylorotwell merged commit 4638617 into laravel:7.x Mar 12, 2020
6 of 7 checks passed
6 of 7 checks passed
P7.2 - Sprefer-lowest
Details
P7.2 - Sprefer-stable
Details
P7.3 - Sprefer-lowest
Details
P7.3 - Sprefer-stable
Details
P7.4 - Sprefer-lowest
Details
P7.4 - Sprefer-stable
Details
continuous-integration/styleci/pr Issues have been identified with 1 file
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can鈥檛 perform that action at this time.