-
Notifications
You must be signed in to change notification settings - Fork 11.2k
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
[8.x] Add global extenders #35202
[8.x] Add global extenders #35202
Conversation
} | ||
|
||
foreach (array_keys($this->resolved) as $abstract) { | ||
$this->rebound($abstract); |
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.
This feels a bit odd. We fire the rebound callback for everything every resolved by the container, even if it was not affected by this global extender?
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.
I agree. At first I planned on firing the rebound callback only for resolved instances but then I saw that we currently also fire them for resolved bindings so I decided to stay consistent with the current behaviour.
That being said, I understand that with specific extenders, we can be pretty sure the resolved bindings are affected by the extender whereas we cannot make the same assumption with global extenders.
I think a good compromise would be to:
- Keep firing the rebinding callbacks for resolved instances — since they are re-computed by the global extender.
- Stop firing the rebinding callbacks for resolved bindings — since they are not being re-computed and we cannot assume that the global extender affects them.
What do you think?
Can we start at square one and talk about why you want this feature? What are you trying to implement in your application / project? Maybe there is a better way. I don't really like any of the options here. |
No worries, thank you for your interest. This came about as I started designing the specs for Laravel Actions v2. I’d like to streamline this version and make it much less intrusive such that, at the beginning, your action is just a plain old PHP object that you have full control over. class RescheduleAppointment
{
protected $calendarService;
public function __construct(CalendarService $calendarService)
{
// Resolved from the container as usual.
$this->calendarService = $calendarService;
}
public function run(User $user, Appointment $appointment, Carbon $newDate)
{
// ...
}
} This means dropping the attributes system and wrapping actions in decorators when they need to be run as controllers, jobs, listeners, etc. The latter is especially important to me since that removes any cross-pattern conflicts we have or might have in future versions of the framework (e.g. middleware for controllers and jobs). That would also make Laravel Actions more scalable as we can support more patterns in isolation. For example, this is how we could add the ability for any action to be run as a controller. class RescheduleAppointment
{
use AsController;
// ...
public function asController(Request $request, Appointment $appointment)
{
$newDate = Carbon::parse($request->get('date'));
$this->run($request->user(), $appointment, $newDate);
return new AppointmentResource($appointment);
}
} Now all we need to do, is recognise objects that use that trait and swap them for a Since we do not know these objects in advance, the only way we can create extenders for them is to map all classes of the user’s project and create specific extenders for all of them, in the chance that they might be resolved. With global extenders or any other system that would enable us to hook custom logic into the container resolution, we can identify these objects as and when they are actually used and swap them with the appropriate decorator. Whilst this is particularly useful for Laravel Actions in this instance, I believe this could bring a lot of benefits for other Laravel users and package developers that want their classes to be recognised as and when they are being resolved from the container, without having to manually register them in a service provider. Another alternative would be to add Thank you for reading this far. I hope this illustrates my problem a bit better. |
Does the |
No, this will simply trigger some new callbacks at the beginning of the resolve method. I will make a quick new PR so you can tell me what you thing. Additionally, I'm going to streamline this PR so that it does not add any overhead on anything that's already resolved. That way you'll have two PR to choose from if you think any of this is worth merging. |
Current limitations
It is currently impossible to hook some logic into the container resolution unless we know in advance the exact class that will be resolved.
This makes it impossible to extend classes that:
The container offer four types of resolution callbacks, namely:
globalResolvingCallbacks
globalAfterResolvingCallbacks
resolvingCallbacks
afterResolvingCallbacks
However, none of the callbacks can affect what is being returned by the
resolve
method and, therefore, cannot be used to extend a class that follows some custom logic.Solution
The solution I offer in this PR is to support global extenders in addition to the existing extenders (following the same convention as
resolvingCallbacks
andglobalResolvingCallbacks
).Usage
The
Illuminate\Container\Container@extend
method has been extended to support both specific and global extenders.As you can see, this enables us to properly hook some custom logic at the core of the container resolution. This offers a new set of possibilities for both Laravel user and package developers.
For example, we could use this mechanism to dynamically catch certain classes that are being used as controllers and swap them for a more appropriate decorator. As such, this would allow packages like Laravel Actions to decorate their own sub-classes dynamically without having to extend core Laravel components that might conflict with other packages.
Backward compatibility
This PR does not add any breaking changes.
It adjusts the current signature of
extend
in a way that does not break the current usage of this method.Other notes
forgetExtenders($abstract)
, aforgetGlobalExtenders()
method has been added for consistency.extend
method, when registering a new global extender:$resolved
and$instances
elements.