Bug: `Attribute "..." cannot target parameter (allowed targets: property)`

[devajmeireles]

Laravel Version

11.17

PHP Version

8.2.21

Database Driver & Version

No response

Description

The PR: #52167 accidentally introduced what I think is a potential breaking change related to attribute usage through container. This bug was originally reported here: tallstackui/tallstackui#567.

---

One of my open-source package contains an attribute that we use only to mark properties to skip certain conditions (see below). After upgrading any of my projects using this package to Laravel 11.17 this error happens:

---

The source of the error matches the changes made in the PR mentioned above:

Here: https://github.com/laravel/framework/pull/52167/files#diff-cc160328d7d447677b8d359453f4416c820a9117c5efd500a3f0361d1d5bab1a

Steps To Reproduce

IDK exactly how to reproduce this, but I guess it just try to use an attribute in a public parameter, like this:

<?php

namespace App\Attributes;

use Attribute;

#[Attribute(Attribute::TARGET_PROPERTY)]
class SkipDebug
{
    //
}

use App\Attributes\SkipDebug;

public function __construct(
    #[SkipDebug]
    public ?string $size = null,
) {
    //
}

[devajmeireles]

The fix in my case seems easy:

#[Attribute(Attribute::TARGET_PROPERTY | Attribute::TARGET_PARAMETER)]
class SkipDebug
{
    //
}

... But I just want to confirm that this will not generate a breaking change in other applications using this new version of Laravel 🤔

[innocenzi]

While technically this was, in fact, a breaking change, you have to specify TARGET_PROPERTY and TARGET_PARAMETER if you are going to use an attribute on a parameter using property promotion. That's why newInstance throws here.

I'll see tomorrow if we can mitigate this breaking change by looking for the attribute targets before creating the instance (or rescuing the throw), because marker attributes that are not properly configured will all throw now.

[innocenzi]

Okay, I was trying to make a getTarget check, but the more I thought about it, the more I realized that this is just trying to work around userland bugs.

When using an attribute in a constructor, you need that attribute to have the right target for it. Otherwise it means that the attribute wasn't supposed to be used in that place.

The fact that this new version of Laravel throws because of that is just revealing of an issue in your code, and the breaking of the package is a side-effect of that.

I think this is not an actual issue with Laravel, and should be taken care of in userland packages.

[devajmeireles]

Okay, I was trying to make a getTarget check, but the more I thought about it, the more I realized that this is just trying to work around userland bugs.

When using an attribute in a constructor, you need that attribute to have the right target for it. Otherwise it means that the attribute wasn't supposed to be used in that place.

The fact that this new version of Laravel throws because of that is just revealing of an issue in your code, and the breaking of the package is a side-effect of that.

I think this is not an actual issue with Laravel, and should be taken care of in userland packages.

I guess I disagree because it worked perfectly fine in Laravel 11.16, but not in the new version. Anyway, I will update it to the expected format. I believe that thousands of other applications and packages will have the same problem.

[innocenzi]

I believe that thousands of other applications and packages will have the same problem.

I really don't think so, because the issue only exists due to misuse of the target restriction of your attribute.

Since your attribute is meant to be used on constructor parameters and class properties, it should either specify no target, or the right ones (Attribute::TARGET_PROPERTY | Attribute::TARGET_PARAMETER).

Ultimately, this is a userland bug, that has only been revealed because Laravel now supports attributes in the container. If your attribute were to be properly defined for its use case, the error wouldn't have happened.

[driesvints]

we reverted the causing PR.