-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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.8] Fix Application contract violations #26477
[5.8] Fix Application contract violations #26477
Conversation
The foundation application contract is seriously pretty pointless. 😆 |
Yeh, this has come up before. Lumen doesn't even implement it. The point of the Laravel application class is that it is specific to the Laravel framework, and thus shouldn't be used in components. We should probably resolve the cyclic dependency issues, and remove the contract. |
Yeah, I agree. Maybe we should just remove it? |
The only time it's sort of useful is maybe when building a package and you need to mock it or something so it lets you mock it without pulling in the whole framework. But, I agree there will likely never be a custom implementation of it and it's a constant source of head-ache adding all these methods to it. |
To be honest, I extended Lumen to implements |
I agree with @crynobone. If you want to remove the |
The segregation of the Foundation Application contract is a pretty decent idea. |
I'm all for segregation of the Application, this PR just make it even harder to create an alternative |
I've already fixed some other contracts lately and now it is time for the
Illuminate\Contracts\Foundation\Application
contract which is the most egregious offender of them all.A couple of months ago we were refactoring an application at work which was written in our legacy in-house framework and we wanted to switch to Laravel. Of course that couldn't be completed overnight and had to be done in smaller steps.
Step one was (obviously) to implement the
Application
contract so that it can work with the legacy framework and that the new stuff can be written using the Laravel framework features (so that when the time comes and we refactor everything that's needed we could switch to a "normal" Laravel just by binding in the container the regular LaravelApplication
implementation instead of using our custom one). We were like "Laravel provides us with a nice contract, that should work without a problem", but oh boy were we wrong.Turns out the
Application
contract is missing a bunch of methods that are getting called throughout the framework's code (some constructor docblocks are even saying that the class needsIlluminate\Foundation\Application
class just to obscure the fact that the contract is useless/missing some methods). Just take a look at PRs like #25830 -> this is a big red flag that something isn't right here :)We obviously implemented all the missing methods in our
Application
implementation until the app finally worked, but these methods should really be in the contract - or the contract in it's current state can be deleted from Laravel as it's basically useless. Since the contract definitely isn't useless as evidenced by our use-case I went through the code and put all methods that should be there (and that get called throughout the framework's code) in the contract.