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

[8.x] Add global extenders #35202

Closed
wants to merge 7 commits into from

Conversation

lorisleiva
Copy link
Contributor

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:

  • Implement a certain interface;
  • Use a certain trait;
  • Extend a certain super-class;
  • Or, more generally, follow any custom logic we might have.

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 and globalResolvingCallbacks).

Usage

The Illuminate\Container\Container@extend method has been extended to support both specific and global extenders.

// Current usage (still supported).
$this->app->extend(SomeSpecificThirdPartyService::class, function ($instance, $app) {
    return FakeServiceDecorator($instance);
});

// New (more flexible) usage.
$this->app->extend(function ($instance, $app) {
    if (! $app->isProduction() && $instance instanceof CanBeFaked) {
        return FakeServiceDecorator($instance);
    }

    return $instance;
});

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

  • Since we currently have the ability to forget specific extenders using forgetExtenders($abstract), a forgetGlobalExtenders() method has been added for consistency.
  • Similarly to the current behaviour of the extend method, when registering a new global extender:
    • The provided callback is applied to all previously resolved instances;
    • The rebinding callbacks are triggered for all $resolved and $instances elements.
  • Many tests have been added following the existing tests for specific extenders.
  • Please let me know if there is anything I can do to help improve this PR.

}

foreach (array_keys($this->resolved) as $abstract) {
$this->rebound($abstract);
Copy link
Member

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?

Copy link
Contributor Author

@lorisleiva lorisleiva Nov 13, 2020

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?

@taylorotwell
Copy link
Member

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.

@lorisleiva
Copy link
Contributor Author

lorisleiva commented Nov 14, 2020

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 ControllerDecorator that will call that asController method under the hood. This is particularly important for controllers since the instance is cached on the Route object and so we always end up calling the same instance.

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 beforeResolving callbacks that would be triggered at the beginning of the resolve method. This would give us the opportunity to dynamically add specific extenders as we identify the objects we want to decorate.

Thank you for reading this far. I hope this illustrates my problem a bit better.

@taylorotwell
Copy link
Member

Does the beforeResolving approach have any of the same overhead concerns as this approach. With this current PR the make method will be re-called for every single resolved binding when a global extender fires.

@lorisleiva
Copy link
Contributor Author

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.

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.

3 participants