Add PHP 8.0 Attribute mapping support

[IonBazan]

Hi guys, this is my first contribution here 👾

Q	A
Type	feature
BC Break	minor, due to @NamedArgumentConstructor - see UPGRADE-2.3.md
Fixed issues	#2337
Conflicts with	#2243 (some changes are duplicated there but we need ti check for conflicts or differences)

Summary

This PR adds AttributeDriver to allow mapping documents using PHP 8 Attributes to align with ORM functionality.
The driver is actually same as the AnnotationDriver and using custom Reader.

I noticed some annotations are not documented in the Annotations Reference page:

• @DiscriminatorValue - not documented at all
• @Inheritance - not documented at all, seems not used anywhere Deleted in 5b4a286
• @QueryResultDocument - fixed in add @QueryResultDocument annotation reference #2347

Progress

• Add the Driver
• Annotate all Annotations/Attributes with @NamedArgumentConstructor and add constructors
• Test the mapping against existing drivers
• Mention the new driver in the documentation
• Review Annotations that are not present in the Annotation Reference

Notes

• All annotations are now using @NamedArgumentConstructor and thus strict_types and Enum checks are skipped (see https://github.com/doctrine/annotations/blob/f96567ba9e3848f59256b473d730e5ba39d81bfe/lib/Doctrine/Common/Annotations/DocParser.php#L569). I removed the tests relying on these features as they are no longer relevant.
• #[Document]'s indexes property cannot be used with PHP Attributes as nested Attributes are not supported. This property is deprecated anyway and top level #[Index] attribute should be used instead.
• #[Indexes] attribute cannot be used for the same reason but it is deprecated too.

[alcaeus]

Thank you for the work done so far! @beberlei could I ask you to help out with reviewing this since you made the same change in ORM?

I'm a little strapped for time right now, but we'll try to give feedback as quick as possible. We definitely want this feature in ODM, so thank you again for getting this started!

[beberlei]

I see this extends the AnnoationDriver with no extra changes. This will not work or you must require PHP 8.1, because nested attributes are not supported in 8.0.

I am not super familiar with the ODM, one example i saw #[Indexes] nesting index attributes.

What we did in ORM is copy adapted the Attribute driver and flattened the attribute hierachy

[IonBazan]

@beberlei You are right but the #[Indexes] and #[Document(indexes: ...)] are the only places we use nested annotations and both are deprecated. That's why I'd suggest to keep it simple and keep the attribute hierarchy flat in the future. I saw the ORM implementation where mapping relies on nested annotations but since we are dropping existing nested annotations soon, I don't think it's worth implementing that in AttributeDriver.

[beberlei]

That makes sense then, i never used the ODM before and am clueless about use of nested annotations there, just wanted to make sure that its adressed

[IonBazan]

OK I revisited all annotations and compared with changes from #2243 - there were a few places where I missed the default values or properties that shouldn't be set in constructor.

The only problematic one is Indexes which I can't make work with NamedArgumentConstructor because $value can be an array or a single item and can be constructed either by passing an array of indexes or a list of arguments like:
@Indexes(@Index, @Index, @Index so I left it intact as it's deprecated anyway.

[alcaeus]

I think we're fine not supporting #[Indexes] until initialisers are done. In the meantime, people have other ways to map indexes using attributes. Let's just make sure to call this out in our docs so that people know they can't just change annotation syntax into an attribute syntax without making some minor tweaks.

[alcaeus]

Gave this a review, but I'd still appreciate someone else taking a look. Great work @IonBazan!

[malarzm]

Just wanted to let you know I'll get to this PR somewhen next week. I'm a bit swamped at work before going on vacation ;)

[franmomu]

I was trying the branch and I had to add the constructor like in https://github.com/doctrine/orm/blob/106ed8009a0c581a1566775cbd4d2c4cc93976b1/lib/Doctrine/ORM/Mapping/Driver/AttributeDriver.php#L33-L39

Looks like the create method was introduced for testing purposes in 5d783c3

[IonBazan]

I've added the constructor but it seems that it doesn't make much sense here. I think we should use the one from parent class.

[franmomu]

The thing here is that using the bundle it creates the service definition for the driver here I think:

https://github.com/symfony/symfony/blob/83bdd2eacf593ec8ab4511afccba75bcaf92ef66/src/Symfony/Bridge/Doctrine/DependencyInjection/AbstractDoctrineExtension.php#L210-L212

And it only receives the array of paths.

[IonBazan]

Thanks for clarification - in that case, the current constructor should work with that. I will try adding the integration to MongoDBBundle just to make sure it works.

[alcaeus]

I will try adding the integration to MongoDBBundle just to make sure it works.

For prior art, you can check out the same change in DoctrineBundle: doctrine/DoctrineBundle#1322. Changes should be pretty much the same, since the bundles share the same base architecture.

[Ahummeling]

So I feel like I've already gone into too much technical detail already, feel free to ignore / resolve my comments at will. This is my first code review I've done outside of my own company. If my comments seem strange or out of place, please let me know, so I can work on it.

[alcaeus]

Changes LGTM, only a question about the handling of the @Indexes annotation which seems to have changed slightly.

[alcaeus]

@IonBazan the test failure on a sharded cluster seems unrelated - this might be a case of us not using the correct write concern for those tests on a sharded cluster. I'll investigate separately but have kicked the build to see if it passes now.

[alcaeus]

LGTM. Great work @IonBazan, this is very much appreciated! 💚

[alcaeus]

@franmomu @malarzm happy to merge this if one of you gives it another seal of approval :)

[franmomu]

Awesome work! thank you! 🚀

[malarzm]

@IonBazan one last thing: could you please rebase your branch atop current 2.3.x? I'd like to al least get rid of merge commits from the history, also there were commits for CI that are no longer needed. Ideally the commit history should make sense on its own, but probably it's too late now so we may squash everything together :)

[IonBazan]

@malarzm I managed to squash and edit some of the commits to get rid the ones that have been reverted so the history makes more sense now. I kept co-author info where @franmomu helped. Let me know if it needs further squashing.

[alcaeus]

Thanks @IonBazan! Great work here!