Edge-case regression with 2.9.x

[albe]

We (neos/flow framework) suffer from a slight change in expectations for some annotation classes since #8266 - namely the ManyToOne, ManyToMany and Embedded now have a required first constructor argument. While those arguments were always considered "required" in a pure doctrine environment, we partially enhanced the DX by extracting some information for the annotations from other reflection data (like the targetEntity/class the annotation targets). Hence we have usage of doctrine annotations like:

@var Collection<SomeEntity>
@ORM\ManyToMany

which now breaks, because the ManyToMany is attempted to be constructed with null as targetEntity in the DocParser. Until now we solved the "missing" targetEntity (and other things) via a custom AnnotationDriver and that worked out quite nicely. With this change we are at a loss though, because we'd need to hook into the DocParser now, which happens way too early.

See neos/flow-development-collection#2487 (comment)

While we can "solve" this by restricting doctrine/orm to <2.9, we would also like to move towards PHP8 support and attributes. For us the best solution would be if those three constructors would allow null for their first argument, but I understand that would mean you'd need to check for that null at runtime which is a step backward. Optimally the Annotation would not be instanciated until it is used for mapping, but that would be too great a change.
Are there any other possible ways to "enhance" the annotations and hook into the instanciation process?

[albe]

friendly ping @beberlei @greg0ire

[greg0ire]

@var SomeEntity

Shouldn't that be @var ArrayCollection<int, SomeEntity>, or are you really using it that way?

Anyway, adding these constructors was technically a BC break, so it would make sense to me to allow null until next major to give you some wiggle room. A deprecation would have to be triggered when null is passed.

[beberlei]

Yes that makes sense, can you make a PR for that change? Including the deprecation about nullability if the value is passed this way to the attribute/annotation class:

Deprecation::trigger('doctrine/orm', 'https://github.com/doctrine/orm/issues/8753', 'Passing no target entity is deprecated.');

[greg0ire]

I can give it a try, yes

[greg0ire]

Wait, this has already been fixed in a223048, hasn't it? The only thing that seems to be missing are deprecations…

[albe]

Shouldn't that be @var ArrayCollection<int, SomeEntity>, or are you really using it that way?

Yes, indeed. That was just an error in the manually typed example code :)

Wait, this has already been fixed in a223048

👀 That does indeed seem to fix it for ManyToOne annotation, but I can't see the same change for ManyToMany (https://github.com/doctrine/orm/blob/2.9.x/lib/Doctrine/ORM/Mapping/ManyToMany.php#L64)? Embedded seems to be changed already in #8724

[greg0ire]

Ah indeed, ManyToMany needs to be handled as well 👍