chore: merge the app with the container & implement the ApplicationContract #3862
Conversation
…ntract Illuminate components always expect the app to be the container, but also expect the app to be implementing the laravel app contract. This means that very often between minor illuminate updates we get a call to a method on the app that doesn't exist in the Flarum app. This fixes the issue once and for all.
| @@ -46,6 +46,8 @@ protected function tearDown(): void | |||
| protected function app() | |||
| { | |||
| if (is_null($this->app)) { | |||
| $this->config('env', 'testing'); | |||
There was a problem hiding this comment.
Why was this added / why wasn't it needed before?
There was a problem hiding this comment.
since we now implement the laravel ApplicationContract which has the environement method that can be called internally in illuminate components, we might as well also make sure the value is correct in testing. (that's the gist of the changes in this PR)
| * Implementation of the Laravel Application Contract, | ||
| * for the sake of better integration with Laravel packages/ecosystem. | ||
| */ | ||
| trait InteractsWithLaravel |
There was a problem hiding this comment.
A lot of this stuff feels familiar, didn't we have it written somewhere before?
There was a problem hiding this comment.
predates my time in flarum, but it might have implemented the laravel ApplicationContract at some point and then removed.
framework/core/src/Foundation/Concerns/InteractsWithLaravel.php
Outdated
Show resolved
Hide resolved
| return false; | ||
| } | ||
|
|
||
| public function registerConfiguredProviders() |
There was a problem hiding this comment.
What's up with the empty ones?
There was a problem hiding this comment.
has no actual meaning within our architecture, but have to implement the contract.
There was a problem hiding this comment.
Are any of the methods / code in this trait used for our needs at all?
There was a problem hiding this comment.
Also, could we mark all these as deprecated to discourage using them in Flarum code?
There was a problem hiding this comment.
Are any of the methods / code in this trait used for our needs at all?
Indirectly, within illuminate components we use, like runningInUnitTests or runningInConsole, terminating. At any moment an Illuminate component could start calling one of these methods.
SychO9
force-pushed
the
sm/laravel-tight-coupling
branch
from
cb9a9ff to
47a0298
Compare
SychO9
force-pushed
the
sm/laravel-tight-coupling
branch
from
fe68e67 to
3e49aeb
Compare
Changes proposed in this pull request:
Illuminate components always expect the app to be the container, but also expect the app to be implementing the laravel app contract. This means that very often between minor illuminate updates we get a call to a method on the app that doesn't exist in the Flarum app. This fixes the issue once and for all.
Reviewers should focus on:
The application contract implementation was separated into a trait to reduce the bloat because all those methods we don't actually need/use, so it can serve as a readable way of telling.
Necessity
Confirmed
composer test).