Possible BC break in nested array annotations with @NestedAnnotation

[TomasVotruba]

Hi everyone!

Thanks for adding #402 , it's a life saver with PHP 8 and migration to attributes 👏

I've been exploring one weird bug we had in our code base:

/**
 * @MainAnnotation(
 *     @NestedAnnotation('one'),
 *     @NestedAnnotation('two'),
 * )
 */

What is the problem?

This code returns 1 or 2 NestedAnnotation annitatons, depending if @NamedArgumentConstructor is used above NestedAnnotation.

It should fixed in userland like this:

 /**
- * @MainAnnotation(
+ * @MainAnnotation({
  *     @NestedAnnotation('one'),
  *     @NestedAnnotation('two'),
- * )
+ * })
 */

But at the moment it's a BC break, only first annotation is included. I think this BC breaks should not happen and there should be always 2 instances of NestedAnnotation regardless use of @NamedArgumentConstructor.

I'm looking into it 🙂 Any feedback appreciated 👍

[stof]

Well, doctrine/annotations has 3 ways to instantiate annotations. Among those, only @NamedArgumentConstructor supports arbitrary positional arguments (as it uses the constructor signature). Older instantiation method are mapping unnamed properties to a value property (and so cannot have multiple ones).

Note that your "userland workaround" produces a different API. It gives an array (with 2 items) as one argument, not 2 arguments.

But at the moment it's a BC break, only first annotation is included. I think this BC breaks should not happen and there should be always 2 instances of NestedAnnotation regardless use of @NamedArgumentConstructor.

I don't understand which BC break you are talking about. The existing behavior when @NamedArgumentConstructor is not used is the behavior that exists since doctrine/annotations 1.0. The BC break is what you request to do, not what existing releases are doing.

[stof]

the unfortunate thing is that doctrine/annotations silently ignores invalid usages of old annotations (using multiple positional arguments while only the last one was kept) instead of reporting it loudly.

[TomasVotruba]

Here is the PR in open-source project contributte/apitte#165
It is passing correctly (seeing 2 NestedAnnotations).

When we remove the {} it fails, as it found just 1 NestedAnnotation: contributte/apitte#165 (comment)

[stof]

@TomasVotruba as I said, when you use n{}, you have a single positional argument containing an array. That's not the same than omitting it.

[TomasVotruba]

@TomasVotruba as I said, when you use n{}, you have a single positional argument containing an array. That's not the same than omitting it.

I know. That's handled like this: https://github.com/apitte/core/blob/5fae3d46984b70e1ee908a4aa6b228e142d88bdb/src/Annotation/Controller/RequestParameters.php#L20-L32

[TomasVotruba]

Basically question is: how can I preserve original behavior (2 items found in any situation) with adding @NamedArgumentConstructor and without adding {} everywhere?

[stof]

you mean, allowing people to misuse the annotations where all nested positional annotations except the last one are silently lost ? Well, add a variadic constructor argument and discard all values except the last one...

[TomasVotruba]

No, I mean keeping the original behavior (2 items) that would not be influenced by the @NamedArgumentConstructor in one of them. Adding @NamedArgumentConstructor annotation adds the magic behavior you talk about. The 2nd+ annotaiton is silently skipped. Took me 3 hours to find out :/

If it's not possible, then I'll come up with fixer rule, no troubles :)

[stof]

ah indeed. I misread the code for the old way. In case of multiple positional arguments, it does not ignore them. It turns it into an array value instead (which makes the signature quite confusing as that value argument can then either be a NestedAnnotation object or an array of multiple NestedAnnotation objects).
For @NamedArgumentConstructor, there is indeed no equivalent magic built-in, as doctrine/annotations will use the constructor with whatever it is given. So if you want such magic, you need to implement it in your own constructor, by allowing to call it as either new MainAnnotation(array(new NestedAnnotation(), new NestedAnnotation())) or new MainAnnotation(new NestedAnnotation(), new NestedAnnotation()).

[TomasVotruba]

Thanks for patience with me. That's exactly what I meant the whole time 👍

I don't understanda how should I fix it. I thought this snippet already handles it, but it does not:

https://github.com/apitte/core/blob/5fae3d46984b70e1ee908a4aa6b228e142d88bdb/src/Annotation/Controller/RequestParameters.php#L20-L32

Could you explain me in real PHP code example what should I do with my annotation to make it work?

[stof]

@TomasVotruba your constructor only uses the first argument. So it does not handle being called as new MainAnnotation(new NestedAnnotation(), new NestedAnnotation()).

This code you linked deals with the weird signature provided by the old doctrine/annotations instantiator. As said before, if you want to migrate to @NamedArgumentConstructor, you need to make your own constructor support both signatures if you want to allow your users to use both (which is not specific to doctrine/annotations at all. My previous comments shows 2 PHP instantiation calls)

[TomasVotruba]

I see. But how can I handle new MainAnnotation(new NestedAnnotation(), new NestedAnnotation()) and include original behavior? I'm not sure how variadics works, so I'm completelly lost in my brain.

[stof]

well, using func_get_args() (or a variadic argument) to get all arguments.