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

[5.8] Fix Application contract violations #26477

Merged

Conversation

X-Coder264
Copy link
Contributor

@X-Coder264 X-Coder264 commented Nov 11, 2018

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 Laravel Application 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 needs Illuminate\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.

@taylorotwell
Copy link
Member

The foundation application contract is seriously pretty pointless. 😆

@GrahamCampbell
Copy link
Member

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.

@driesvints
Copy link
Member

Yeah, I agree. Maybe we should just remove it?

@taylorotwell
Copy link
Member

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.

@crynobone
Copy link
Member

To be honest, I extended Lumen to implements Illuminate\Contracts\Foundation\Application, it is possible and it would be nice if we can split some of the Laravel specific implementations as separate contracts, such as getCachedCompilePath(), getCachedServicesPath(), getCachedPackagesPath(), some of the helpers class may be reused such as configPath(), basePath() etc.

@X-Coder264
Copy link
Contributor Author

I agree with @crynobone. If you want to remove the Application contract then the service providers should only rely on the Container contract, but some service providers also need some of the methods that are not on that contract (for example the DatabaseServiceProvider needs the databasePath method) so it'd be nice to have a contract or something for such cases.

@deleugpn
Copy link
Contributor

The segregation of the Foundation Application contract is a pretty decent idea.
As a soft migration path, we could even have the Foundation Application implementing a bunch of contracts and tag it as deprecated for removal on Laravel 5.9 or 6.0.

@taylorotwell taylorotwell merged commit d52298a into laravel:master Nov 13, 2018
@X-Coder264 X-Coder264 deleted the fix-application-contract-violations branch November 13, 2018 16:05
@crynobone
Copy link
Member

I'm all for segregation of the Application, this PR just make it even harder to create an alternative Application other than extending Illuminate\Foundation\Application.

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.

6 participants