Skip to content

Add lumen bootstrap #57

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

Closed
wants to merge 1 commit into from
Closed

Add lumen bootstrap #57

wants to merge 1 commit into from

Conversation

KenoKokoro
Copy link

Lumen bootstrap class. Laravel bootstrap can't be used for Lumen since it doesn't have the bootstrap/autoload.php file and it needs to instantiate the Laravel\Lumen\Application instead of the Illuminate\Contracts\Http\Kernel class.

@marcj
Copy link
Member

marcj commented Jun 29, 2017

Then I'd adjust https://github.com/php-pm/php-pm-httpkernel/blob/master/Bootstraps/Laravel.php instead of writing a whole new class.

@KenoKokoro
Copy link
Author

The bootstrap is different than the Laravel's. It diverse with the autoload.php file, and the Application instance as well.
Why would you like complicated method with conditions, instead of single and simple class?
My suggestion is to leave the lumen class. It follows the single responsibility principle as well.
Or, if you don't like the duplicating code on the rest of the methods, just wrap them in parent class, and leave the getApplication() method on Laravel and Lumen class.

This is just my suggestion and opinion, something that I would do.

This was referenced Nov 21, 2017
@andig
Copy link
Contributor

andig commented Dec 12, 2017

Can this be closed now that #68 has been merged?

@andig
Copy link
Contributor

andig commented Apr 14, 2018

ping @KenoKokoro is this still relevant?

@KenoKokoro
Copy link
Author

@andig
I am not sure, I was just experimenting then with the Lumen framework, but as I can see on the Lumen repository, the bootstrap file is still under bootstrap/app.php so seems that should be relevant.

@andig
Copy link
Contributor

andig commented Apr 16, 2018

I'm closing this PR as implemented after having taken another look at https://github.com/php-pm/php-pm-httpkernel/blob/master/Bootstraps/Laravel.php. At this time, Laravel bootstrap supports both Laravel and Lumen.

Only thing remaining from usability perspective might be to subclass Laravel bootstrap as Lumen without adding any new functionality just to make this more obvious to the user, but that would be another PR.

Thank you for your contribution and staying with us through the discussion!

@andig andig closed this Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants