themosis / theme

The Themosis framework theme.
http://framework.themosis.com/
GNU General Public License v2.0
104 stars 35 forks source link

Kernel instantiation is not handled as a singleton #38

Closed ntsim closed 5 years ago

ntsim commented 5 years ago

The current implementation of the theme's index.php instantiates a non-singleton instance of App\Http\Kernel when $app->manage is called. This is despite the container registering an instance already through the Illuminate\Contracts\Http\Kernel contract in bootstrap/app.php.

By this point the App\Http\Kernel may have already been instantiated (e.g. through a service provider), so we can potentially end up having multiple instances of App\Http\Kernel floating around in the context.

From my perspective, this is incorrect behaviour, but I don't know if there is some more background to how this theme worked in v1 (I have only used v2), or how it is intended to be used.

Regardless, it has personally been problematic as middleware registered by a third-party service provider were seemingly being unregistered (even though they appeared to be registered when the application first bootstraps). In reality, the middleware was being registered on the older Kernel instance.

If I have correctly identified this as a bug, then it can simply be fixed by providing Illuminate\Contracts\Http\Kernel to $app->manage.

jlambe commented 5 years ago

From my understanding I'm not sure the service container is building multiple instances of the Kernel but it is true that it is best to reference the kernel interface instead of the class directly as like you mentioned, we have mapped the App\Http\Kernel class to the Illuminate\Contracts\Http\Kernel interface at application bootstrap.

It is indeed way better to change the kernel reference to the interface on the index.php file of the theme. Would you like to submit a PR for this ?

ntsim commented 5 years ago

PR created :smile:. I've added an explanation as well in the description.