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] Allow non-ContextualAttribute attributes to have an after callback #52167

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

innocenzi
Copy link
Contributor

@innocenzi innocenzi commented Jul 17, 2024

This pull request is a follow-up of #51934.

It allows all attributes to have an after callback, not just ContextualAttribute ones. As a reminder, this callback is the equivalent of $this->app->afterResolvingAttribute, but directly on the attribute.

The restriction to have it only on ContextualAttribute was accidental due to it being implemented late in the previous pull request.

EDIT: PR reverted because of:

  • Poorly configured attributes (wrong target)
  • Dev-only attributes

@taylorotwell
Copy link
Member

Hmm, what would the be the use case of this?

@innocenzi
Copy link
Contributor Author

The same as before, except I don't register the binding in a service provider, but on the attribute itself.

In an application I'm working on, specifications have changed, and more parts of the app need to use the "tenancy system", which I wanted to extract and make more generic. While doing so, I wanted to write the following OnTenant attribute:

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

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

This contract is implemented by multiple classes in my application. I apply the OnTenant attribute on the constructor parameters that inject these classes.

interface HasTenant
{
    public function onTenant(Tenant $tenant): static;

    public function getTenant(): Tenant;

    public function hasTenant(): bool;

    public function ensureHasTenant(): void;
}

Except that I couldn't, because OnTenant is not a ContextualAttribute: its job is to mutate the class the attribute is used on, not to resolve it.

Prior to this pull request, I'd have to register a afterResolvingAttribute binding in a service provider. In this situation, I would prefer not to (even though it's honestly not a big deal, I just like having the logic in the same place).

@taylorotwell taylorotwell merged commit 45592e6 into laravel:11.x Jul 18, 2024
30 checks passed
@innocenzi innocenzi deleted the feat/attribute-after branch July 18, 2024 16:04
@devajmeireles
Copy link
Contributor

@taylorotwell I don't know if I'm sure about it, but when I upgraded one of my projects to Laravel 11.17 using a package created by me (TallStackUI) I started to get this error:

CleanShot 2024-07-23 at 16 46 19@2x

When I go back to Laravel 11.16 the error doesn't occur anymore. I think this PR introduced a breaking change. This was originally reported here: tallstackui/tallstackui#567

@devajmeireles
Copy link
Contributor

I'm guessing this because according to my analysis this was the only attribute-related PR in v11.17

C/C @driesvints

@devajmeireles
Copy link
Contributor

The source of the error is related to this PR:

CleanShot 2024-07-23 at 16 50 30@2x

@dwightwatson
Copy link
Contributor

This has caused an issue for me also - I use scribe to generate API documentation. I only have scribe installed as a dev-dependency as it builds the documentation during CI/CD and the attributes aren't required in production.

In Laravel 11.16 this wasn't an issue, but this change in Laravel 11.17 means Laravel is now trying to find this attribute.

use Knuckles\Scribe\Attributes\Group;


#[Group('Feeds', <<<'DESC'
Endpoints that return currently available listings that can be listed on other sites with permission.
DESC)]
class RoomController extends Controller
{
    //
}

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