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

[11.x] Add support for acting on attributes through container #51934

Merged
merged 13 commits into from
Jul 1, 2024

Conversation

innocenzi
Copy link
Contributor

@innocenzi innocenzi commented Jun 27, 2024

Follow-up of #51115, @ollieread should be accredited as co-author of this pull request

This pull request adds support for resolving instances marked by attributes as well as acting on resolved instances marked by attributes.

 

Resolving instances through attributes

This is done by creating an attribute that implements ContextualAttribute, and binding it to the container via whenHas:

#[Attribute(Attribute::TARGET_PARAMETER)]
class AuthGuard implements ContextualAttribute
{
	public function __construct(
		public readonly string $name
	) {}
}
Container::getInstance()->whenHas(AuthGuard::class, function (AuthGuard $attribute) {
	return Auth::guard($attribute->name);
});

When an attribute is bound through whenHas, any class constructor parameters marked with the corresponding attribute will be resolved by the associated closure:

final class MyService
{
	public function __construct(
        #[AuthGuard('api')]
        private readonly Guard $guard
    ) {}
}

$service = Container::getInstance()->make(MyService::class);

For context on this specific example, you may read the description of the previous pull request.

 

Note

The pull request was updated to also support resolving with a resolve method on the attribute itself instead of having to bind a whenHas callback:

#[Attribute(Attribute::TARGET_PARAMETER)]
class AuthGuard implements ContextualAttribute
{
	public function __construct(
		public readonly string $name
	) {}

    public function resolve(self $attribute, Container $container): void
    {
        return Auth::guard($attribute->name);
    }
}

 

Acting on resolved instances through attributes

The other scenario this pull request helps with is acting on resolved dependencies or classes through an attribute.

This is similar to the previous feature, except the container will resolve the dependency first, so you don't have to do it yourself. This is preferable than using whenHas when the dependency is already bound to the container—for instance, in a third-party package—and you want to further configure it. You may also configure multiple callbacks using this method.

#[Attribute(Attribute::TARGET_PARAMETER)]
final class OnTenant
{
    public function __construct(
        public readonly Tenant $tenant
    ) {
    }
}
Container::getInstance()->afterResolvingAttribute(
    attribute: OnTenant::class,
    callback: function (OnTenant $attribute, Connector $connector) {
        $connector->onTenant($attribute->tenant);
    }
);

Using the example above, the following class will have its Connector class resolved first, and the "after resolving attribute" callback called after, so that the connector may be configured accordingly:

public function __construct(
    #[OnTenant(Tenant::FLY7)]
    private readonly Connector $connector
) {}

 

Additionally, you may also add an attribute directly to a class. The following example implements an attribute that calls the booting method when a class is resolved:

#[Attribute(Attribute::TARGET_CLASS)]
final class Bootable
{
}
Container::getInstance()->afterResolvingAttribute(
    attribute: Bootable::class,
    callback: function ($_, mixed $instance, Container $container) {
        if (! method_exists($instance, 'boot')) {
            return;
        }

        $container->call([$instance, 'boot']);
    }
);
#[Bootable]
final class MyService
{
    public function boot(): void
    {
         // Do something...
    }
}

 

Note

The pull request was updated to also support the "after" callback on the attribute itself instead of having to bind a afterResolvingAttribute callback:

#[Attribute(Attribute::TARGET_PARAMETER)]
final class OnTenant
{
    public function __construct(
        public readonly Tenant $tenant
    ) {
    }

    public function after(self $attribute, HasTenant $hasTenant) {
        $hasTenant->onTenant($attribute->tenant);
    }
}

@taylorotwell
Copy link
Member

This real world test doesn't work for me. My whenHas callback is invoked but the container complains the $value dependency of Something is not resolvable.

Also, I wonder if this would feel more natural if the attribute class itself had a method that did the resolution (receiving the attribute and container instance) instead of having to bind a whenHas callback at all?

<?php

namespace App;

use Attribute;
use Illuminate\Contracts\Container\ContextualAttribute;

#[Attribute(Attribute::TARGET_PARAMETER)]
class ConfigValue implements ContextualAttribute
{
    /**
     * Create a new class instance.
     */
    public function __construct(public string $key)
    {
    }
}
<?php

namespace App\Providers;

use App\ConfigValue;
use Illuminate\Support\ServiceProvider;

class AppServiceProvider extends ServiceProvider
{
    /**
     * Register any application services.
     */
    public function register(): void
    {
        $this->app->whenHas(ConfigValue::class, function ($attribute) {
            return config($attribute->key);
        });
    }

    /**
     * Bootstrap any application services.
     */
    public function boot(): void
    {
        //
    }
}
<?php

namespace App;

use App\ConfigValue;

class Something
{
    /**
     * Create a new class instance.
     */
    public function __construct(
        #[ConfigValue('app.timezone')]
        public string $value
    )
    {
    }
}

@ollieread
Copy link
Contributor

Also, I wonder if this would feel more natural if the attribute class itself had a method that did the resolution (receiving the attribute and container instance) instead of having to bind a whenHas callback at all?

The idea behind keeping the two separate was to allow for more flexibility. It would be much easier to override the behaviour, that it would if it was part of the attribute.

@innocenzi
Copy link
Contributor Author

@taylorotwell
This real world test doesn't work for me. My whenHas callback is invoked but the container complains the $value dependency of Something is not resolvable.

The implementation didn't work with primitives, but I just fixed that and added relevant tests. Good catch :)

@taylorotwell
Also, I wonder if this would feel more natural if the attribute class itself had a method that did the resolution (receiving the attribute and container instance) instead of having to bind a whenHas callback at all?

I like the idea! I added support for this, in addition to whenHas.

@ollieread
The idea behind keeping the two separate was to allow for more flexibility. It would be much easier to override the behaviour, that it would if it was part of the attribute.

Fair point. I'm not sure what should take precedence between resolve and whenHas. I think it could be confusing if whenHas took priority, because resolve would then be useless. But at the same time, whenHas could allow overriding an attribute in a third-party package.

In the current implementation, whenHas takes priority.

@taylorotwell do you have an opinion on this?

@taylorotwell
Copy link
Member

I think whenHas having priority makes sense from a package override perspective.

Added support for "after" directly on the attribute. Also added built in Config attribute since it's a fairly straight-forward use case.

@taylorotwell taylorotwell merged commit cb64466 into laravel:11.x Jul 1, 2024
28 checks passed
@ollieread
Copy link
Contributor

@innocenzi There were some modifications I was going to make to #51115, but I've been ill for the last couple of days, primarily:

  • Passing the ReflectionParameter to the handler, optionally
  • Adding a few default implements
    • AuthGuard for the Guard
    • Connection for a database connection
    • Authed for the current Authenticatable
    • Possibly some others for first-party stuff

Though I see now that @taylorotwell merged this while I was writing, I'll create a separate PR with the changes.

@ollieread
Copy link
Contributor

@innocenzi I know this has long been merged, but I've noticed a bit of an issue with the 'resolve' method approach.

You're calling resolve() on an instance of the attribute and then passing the instance as the first parameter, which is redundant.

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.

None yet

3 participants