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

Fix Laravel 5.4 support #10

Merged
merged 3 commits into from
Mar 31, 2017
Merged

Fix Laravel 5.4 support #10

merged 3 commits into from
Mar 31, 2017

Conversation

tortuetorche
Copy link
Contributor

Fix the BindingResolutionException:

 in Container.php line 804: Target [Symfony\Component\HttpKernel\HttpKernelInterface] is not instantiable

Because, since Laravel 5.4:

the container's make method no longer accepts a second array of parameters.

We also can use this line of code, but it's only compatible with PHP >= 5.6.0:

$middleware = new $callable(...$params);

Source: http://stackoverflow.com/a/2409288

Since Laravel 5.4:
> the container's make method no longer accepts a second array of parameters.
@barryvdh
Copy link
Owner

We could wait for this to be tagged: laravel/framework@5d9b363
which brings back makeWith or app($instance, $parameters)

@tortuetorche
Copy link
Contributor Author

tortuetorche commented Mar 15, 2017

Oh, I wasn't aware of this makeWith() method.
Thank you Barry and have a good afternoon 👍

@tortuetorche
Copy link
Contributor Author

Laravel 5.4.16 is released!
So to keep Laravel < 5.4 compatibility, we can do something like:

$makeMethod = method_exists($this->container, 'makeWith') ?  'makeWith' : 'make';
$middleware = $this->container->$makeMethod($callable, $params);

@barryvdh What do you think about this?

@barryvdh
Copy link
Owner

Sounds good

Since Laravel 5.4.16, the Container::makeWith($class, $params) method replace the old behavior of Container::make() method.
@barryvdh
Copy link
Owner

Ready to go?

@tortuetorche
Copy link
Contributor Author

tortuetorche commented Mar 22, 2017

No, I need to do some manual tests...

@tortuetorche
Copy link
Contributor Author

tortuetorche commented Mar 22, 2017

The Container::makeWith() method doesn't work as expected and I don't know why yet.

@tortuetorche
Copy link
Contributor Author

tortuetorche commented Mar 22, 2017

I successfully call the Container::makeWith() method with some modifications:

In vendor/barryvdh/laravel-stack-middleware/src/StackMiddleware.php file:

            // Add kernel as 'app' parameter
            $params['app'] = $kernel;
            $makeMethod = method_exists($this->container, 'makeWith') ?  'makeWith' : 'make';
            $middleware = $this->container->$makeMethod($callable, $params);

And in my Turbolinks Service Provider vendor/efficiently/turbolinks/src/Frenzy/Turbolinks/TurbolinksServiceProvider.php, I replace this code:

        $stack->bind(
            'Frenzy\Turbolinks\Middleware\StackTurbolinks',
            'Helthe\Component\Turbolinks\StackTurbolinks',
            [$this->app['turbolinks']]
        );

By this one:

        $stack->bind(
            'Frenzy\Turbolinks\Middleware\StackTurbolinks',
            'Helthe\Component\Turbolinks\StackTurbolinks',
            ['turbolinks' => $this->app['turbolinks']]
        );

See StackTurbolinks::__construct() method, for more information.

The $params parameter, of Container::makeWith() method, need to have named keys, but I don't know why...

@tortuetorche
Copy link
Contributor Author

I've open an issue on the Laravel Framework repository, see: laravel/framework#18474

@tortuetorche
Copy link
Contributor Author

According to @themsaid laravel/framework#18474 (comment),

With Container::makeWith() method, you have to pass the parameter name.

So it's a breaking change compared to the behavior of Container::make() of Laravel 5.3.
Because instead of binding Laravel Stack Middleware, like this:

        $stack->bind(
            'Frenzy\Turbolinks\Middleware\StackTurbolinks',
            'Helthe\Component\Turbolinks\StackTurbolinks',
            [$this->app['turbolinks']]
        );

We need to bind Laravel Stack Middleware, like that:

        $stack->bind(
            'Frenzy\Turbolinks\Middleware\StackTurbolinks',
            'Helthe\Component\Turbolinks\StackTurbolinks',
            ['turbolinks' => $this->app['turbolinks']]
        );

@barryvdh
Copy link
Owner

OK can you update the pr?

@tortuetorche
Copy link
Contributor Author

@barryvdh Done, but these breaking changes should be documented in the README.md

@barryvdh
Copy link
Owner

Is it breaking?

@tortuetorche
Copy link
Contributor Author

When I said "it's a breaking change", I mean, you can't do this anymore:

app('stack')->bind('AddRobotsHeaders', 'League\StackRobots\Robots', ['production', 'APP_ENV']);

Instead, you need to do this:

app('stack')->bind('AddRobotsHeaders', 'League\StackRobots\Robots', ['env' => 'production', 'envVar' => 'APP_ENV']);

@barryvdh barryvdh merged commit 2e7de1d into barryvdh:master Mar 31, 2017
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.

2 participants