Handle annotation default property

[Vincz]

It would be great to be able to define the default property for annotation. At the moment, it always target the property named value. The ability to change this behaviour could lead to more explicit variables names and a better compatibility with PHP8 attributes.

See the following example.

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 expects a value property.

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

Note that this is already handled in the Doc Parser for annotation like @Annotation\Attribute, as the default property in this case is name.

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

We could support the attribute default_property in many ways:

1. Add it to the @Annotation annotation, like @Annotation(defaultProperty="name"). But as explained here Introduced annotation NamedArgumentConstructor #391 , this annotation is kind of special as it is detected through str_pos.
2. Add a new annotation @DefaultProperty, like @DefaultPropety("name")
3. Add this attribute with NamedArgumentConstructorAnnotation only.

Another path would be to force the DocParser to respect argument constructor arguments order if there is a mix of named / not named arguments.

[murtukov]

I stand for the second option, but with the name @Default similar to @Required.

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

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

[Vincz]

So, I did a bit of research, and here is a summary of how annotations handle the default property.

If annotation doesn't have a constructor

The default property is the first declared public property

Given:

/** @Annotation */
MyAnnotation {
    public $foo;
}

With:

/** @MyAnnotation("bar") */
$foo = "bar";

With:

/** @MyAnnotation("bar", foo="bar2") */
$foo = "bar2";

If annotation has a constructor

If It doesn't implement NamedArgumentsConstructor

The values are passed as an array in the first argument of the constructor
The default property is always value
Given:

/** @Annotation */
MyAnnotation {
    public function __construct(array $data) {

    }
}

With:

/** @MyAnnotation("bar") */
$data = ['value' => "bar"];

With:

/** @MyAnnotation("bar", foo="bar") */
$data = ['value' => "bar", "foo" => "var"];

With:

/** @MyAnnotation("bar", value="bar2") */
$data = ['value' => "bar2"]

If It implements NamedArgumentsConstructor

If PHP >= 8 - Constructor is called directly with the array of values
If PHP < 8 - Values are reordered based on constructor parameters and constructor is called
The default argument name will be value

Given:

/** @Annotation */
MyAnnotation implements NamedArgumentsConstructor {
    public function __construct(string $name) 
    {
    }
}

With:

/** @MyAnnotation(name="bar") */
$name = "bar";

With:

/** @MyAnnotation("bar") */
Failed because, argument named `value` doesn't exist

So we have two different ways to handle the problem.

1. Add an option to set the default property
   Advantage: We would give an option to set the default property to use. It would be backward compatible as it would be a new option and would change the default property only if annotation does have a constructor.
   Disadvantage: The usage could be unatural with NamedArgumentsConstructor as the default property / argument could be anywhere in the arguments list while with PHP attributes, it would be the "first".

/** @Annotation(defaultProperty="param2") */
MyAnnotation implements NamedArgumentsConstructor {
    public function __construct(int $param1, int $param2, string $param3) 
    {
    }
}

This would be a bit weird, no ?
/** @MyAnnotation(12,param1=1,param3="foo") */

2. We could consider the first constructor argument as the default one with NamedArgumentConstructor
   Advantage: It makes sense as it kind of "mimic" how attributes work (as it's a regular function call).
   Disadvantage: It breaks compatibiliy in the following case:

/** @Annotation */
MyAnnotation implements NamedArgumentsConstructor {
    public function __construct(string $name = "foo", string $value = "bar") 
    {
    }
}

With:

/** @MyAnnotation("baz") */

Before:
$name="foo"
$value="baz"

After:
$name="baz"
$value="bar"

let me know what you think!

[derrabus]

We could consider the first constructor argument as the default one with NamedArgumentConstructor
Advantage: It makes sense as it kind of "mimic" how attributes work (as it's a regular function call).

This would be my personal favorite for the same reason you mentioned.

Disadvantage: It breaks compatibiliy in the following case:

That's a bummer. I don't know if this kind of BC break is acceptable.

What we could do is: We keep the current behavior when the marker interface is used and implement your suggestion for the NamedArgumentConstructor annotation only. Because the annotation was not release yet, we can still change its behavior.

At the same time, we deprecate the marker interface. That would create a nice migration path.

Add an option to set the default property

If we neither want to introduce a BC break nor create a different behavior for the annotation than for the marker interface, this option would be acceptable for me.

[Vincz]

@derrabus Thanks for your reply. I agree that changing the behaviour only for the new @NamedArgumentConstructor annotation would be best if we don't want to introduce breaking change.

ping @beberlei @greg0ire What do you guys think about this?

[greg0ire]

I agree with you both, and am glad I hold back from releasing right after merging #391 😄

[beberlei]

Fixed in #402