-
Notifications
You must be signed in to change notification settings - Fork 10.9k
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.5] Refactor container #19201
[5.5] Refactor container #19201
Conversation
* | ||
* @param string $abstract | ||
* @param array $parameters | ||
* @return mixed | ||
*/ | ||
public function makeWith($abstract, array $parameters) | ||
public function makeWith($abstract, array $parameters = []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's only an alias, maybe deprecate it in favor of make
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although we could - I was just thinking about minimising unnecessary breaking changes for 5.5 or onwards.
For 2-3 lines of code it seems simple to keep - and I've added a framework test to ensure it works as intended (so wont be lost in future refactors).
Up to Taylor - happy to remove if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh - and some Packages already use makeWith()
- so it'll create issues about dual 5.4 & 5.5 maintainability if it was removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely do not remove it! I was thinking about annotate as @deprecated
for future removal.
* 'master' of github.com:laravel/framework: (264 commits) fix bug Ensure getJobIds always return an array. (laravel#19232) Use Str:: class. (laravel#19221) Add a PasswordReset event. (laravel#19188) Remove all Arr:: helpers. (laravel#19219) Also test PHP 7.1 with lowest and stable setup. (laravel#19209) No need to reinstall Travis as we only test PHP>=7.0 (laravel#19210) Also disable Xdebug for PHP 7.1 (laravel#19211) Fix typo. (laravel#19216) Remove all Arr:: helpers. (laravel#19218) refactor container (laravel#19201) formatting fix formatting add thumb_url to Slack Attachment https://api.slack.com/docs/message-attachments#thumb_url Updated v5.5 release notes Use Authenticatable-functionality to update remember_token add method for easier error bag checking add `abilities` method to Gate contract (laravel#19173) update allow using assertSessionHasErrors to look into different error bags ...
Is there any interest in adding
class MyImplementation implements MyContract {
public function __construct(SomeDependency $dep, $primitiveArg){}
}
$app->bind(MyContract::class, function ($app) {
return $app->make(MyImplementation::class, ['primitiveArg' => 99]);
});
$instance = $app->make(MyContract::class); There is currently no way to inject primitives without a contract: class MyImplementation {
public function __construct(DependencyContract $dc, $primitiveArg){}
}
$app->bind(MyImplementation::class, function ($app) {
// infinite loop among other issues
// return $app->make(MyImplementation::class, ['primitiveArg' => 99]);
return $app->build(MyImplementation::class, ['primitiveArg' => 99]);
});
$instance = $app->make(MyImplementation::class); Maybe there is a workaround I'm not aware of. Every feature of the container seems to be designed to allow it to function by binding interfaces and binding direct implementations. I think it would be strange to leave this ability out. |
The maker class was originally added in octobercms/library@d0f7c0b to fix an issue in Laravel that ended up being fixed by Laravel in laravel/framework#19201 just three days later. October apparently ended up removing this two years ago as well (octobercms/library@d0f7c0b) but we only recently ran into an issue where this code was causing problems.
With 5.5 we can refactor Container with a small contract change, which should greatly improve the maintainability of Container moving forward. We only need one
make()
path to handle, rather than the current situation of bothmakeWith()
andmake()
.This PR includes keeping
makeWith()
as an alias ofmake()
- so people already usingmakeWith()
in their code do not need to change anything.The only "break" is the contract for custom implementations - but even that is a simply optional parameter addition and would not require changing any current code in applications (apart from the contract implementation).