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

Support Mixin's with constructor arguments #51918

Closed

Conversation

itsDavidSQ
Copy link

When defining a macro we can make use of dependencies by passing them to the closure's context via the use keyword. For example, we could define the following macro in a Service Provider and it would work with no issues:

When creating a macro, we can pass any dependencies that may be needed by using the use keyword when defining the closure. For example, we could define the following macro in a Service Provider and it would work:

$apiVersionResolver = $this->app->get(ApiVersionResolver::class);

Request::macro('apiVersion', function () use ($apiVersionResolver): string {
    return $apiVersionResolver->resolve($this);
});

However, there was no way to achieve the same result using a mixin, as trying to do so resulted in an exception:

ArgumentCountError: Too few arguments to function App\Http\RequestMixin::__construct(), 0 passed and exactly 1 expected

This PR excludes the constructor method from mixin objects, allowing us to specify constructor arguments, which can be used within the macro methods.

Once merged, the following will work as expected:

class RequestMixin
{
    public function __construct(
        private readonly ApiVersionResolver $apiVersionResolver
    ) {
    }

    public function apiVersion(): \Closure
    {
        $apiVersionResolver = $this->apiVersionResolver;

        return function () use ($apiVersionResolver): string {
            return $apiVersionResolver->resolve($this);
        };
    }
}

// In a Service Provider:
$apiVersionResolver = $this->app->get(ApiVersionResolver::class);
Request::mixin(new RequestMixin($apiVersionResolver));

Mixin classes allow to be defined with constructor arguments.
@itsDavidSQ itsDavidSQ marked this pull request as draft June 26, 2024 10:27
@itsDavidSQ itsDavidSQ marked this pull request as ready for review June 26, 2024 10:33
@henzeb
Copy link
Contributor

henzeb commented Jun 26, 2024

same can be achieved with resolve() helper.

@itsDavidSQ
Copy link
Author

That's right, but I feel having explicit dependencies makes it easier to reason about.

@rodrigopedra
Copy link
Contributor

One could argue mixin classes shouldn't have state, as every method should return a closure that will be bound to the extended object.

But considering a class __constructor can be public or protected, which are the filters applied to get the methods being mixed up, I guess filtering the constructor out makes sense.

When mixing up the methods, each method is invoked to get back the closure to be mixed as a macro. So calling an object's constructor again makes no sense.

I would take a step further and also filter out destructors:

public static function mixin($mixin, $replace = true)
{
    $methods = (new ReflectionClass($mixin))->getMethods(
        ReflectionMethod::IS_PUBLIC | ReflectionMethod::IS_PROTECTED
    );

    foreach ($methods as $method) {
        if ($method->isConstructor() || $method->isDestructor()) {
            continue;
        }

        if ($replace || ! static::hasMacro($method->name)) {
            static::macro($method->name, $method->invoke($mixin));
        }
    }
}

@taylorotwell
Copy link
Member

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If applicable, please consider releasing your code as a package so that the community can still take advantage of your contributions!

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.

4 participants