Of course. I've added a piece of documentation. 😃

Just realized we could simplify this by making the marker interface a boolean on @Annotation - what do you think?

I can give it a try. The problem that I see with that approach is that the @Annotation annotation is currently "loaded" via a strpos(), probably to avoid recursion.

Implemented the property as #394. Both approaches would work for me. I'll let you decide. :-)

The problem that I see with that approach is that the @annotation annotation is currently "loaded" via a strpos(), probably to avoid recursion.

Sorry for the dumb questions, I am unfamiliar with the package… what do you mean by "loaded" via strpos()? Can you add a permalink to the relevant piece of code? 🙏
Also, how does it relate to recursion, and where would that cause a problem?

Sorry for the dumb questions, I am unfamiliar with the package…

So was I until I started working on this patch. 😅

what do you mean by "loaded" via strpos()? Can you add a permalink to the relevant piece of code? 🙏

Sure, here you go:

Also, how does it relate to recursion, and where would that cause a problem?

In order to enable a class to be used as annotation, you have to annotate it with the @Annotation annotation. Unlike other annotations, @Annotation is not implemented as an actual class. Otherwise you would end up with code like this:

/**
 * @Annotation
 */
class Annotation {}

In other words: The Annotation class would be annotated with itself. This is a recursion: While loading @Annotation, you look up the class Annotation which is annotated with @Annotation represented by the class Annotation which is annotated with @Annotation…

In #394 I had to add the Annotation class so I could attach a property to it. It works, but I believe not having a class representation of @Annotation is more robust and easier to maintain.

With this solution, what happens when we annotate a class with NamedArgumentConstructor but forget to annotate it with Annotation too? Is it something that could want to do deliberately?

I assume its the same problem with target annotation, it does nothing. Not something we need to care for anymore

I have as slight preference for the second solution because it's better DX IMO, but I'm fine with both solutions. What's your opinion @beberlei , taking into account @derrabus 's concerns?

but forget to annotate it with Annotation too?

The line of code I've linked earlier is the one that determines if a class is a valid annotation. If @Annotation is missing, @NamedArgumentConstructor won't do anything. As Benjamin said, we have this situaton with @Target and other annotations already.

I think this approach here is better then, i didn't realize Annotation was such a workaround to begin with.

Could we also introduce a way to indicate the default_property to target a property other than the property value with NamedArgumentConstructor? Unless it is already the case but I didn't understand how to do it?

use Attribute;
use Doctrine\Common\Annotations\NamedArgumentConstructorAnnotation;

/**
 * @Annotation
 * @Target({"CLASS"})
 */
#[Attribute(Attribute::TARGET_CLASS)]
class Foo implements NamedArgumentConstructorAnnotation
{
    /**
     * The field name.
     *
     * @var string
     */
    public ?string $name;

    public function __construct(?string $name)
    {
        $this->name = $name;
    }
}

The supported syntax are:

#[Foo("ok")]
class TestClass{}

#[Foo(name: "ok")]
class TestClass{}

/** @Foo(name="ok") */
class TestClass{}

but this one is not supported as it assume a value property.

/** @Foo("not ok") */
class TestClass{}

We could add an attribute defaultProperty (default: value) on the @Annotation or on the new @NamedArgumentConstructor?
What do you think?

@Vincz Let me tinker with that idea a bit. We would need something like that for Symfony as well. But I think, we can solve this in a follow-up PR.

@derrabus It is already used internally by the Doc Parser for specific annotations like Annotation\Attribute:

Annotation\Attribute::class => [
            'is_annotation'    => true,
            'has_constructor'  => false,
            'targets_literal'  => 'ANNOTATION_ANNOTATION',
            'targets'          => Target::TARGET_ANNOTATION,
            'default_property' => 'name',
           ...
]

As a workaround, we can add a property 'value' if it is not defined in the constructor and use it's value on another property but it doesn't work with required properties. I could submit a PR but I don't know where would be the best place to add this option and I wonder if it makes sense to have this feature outside the NamedArgumentConstructor context.

Hey guys. Any update about this? Is there anything I can do to help?

Sorry, I've been a bit busy this week. I hope I can work on this on the weekend.

This PR should not extend in scope for now, lets tackle this as an extra issue

Ok, I created a new issue here #396 in order to handle this feature.

Thanks @derrabus !

@derrabus This is game-changer! Thank you for this feature 👏 🙇

Fun fact: I always assumed annotations work this way by default :D

Thanks a lot. 😃