Decouple AttributeDriver from AnnotationDriver

[franmomu]

Q	A
Type	improvement
BC Break	yes
Fixed issues

Summary

This is a start so we can discuss what to do to decouple AttributeDriver from annotations, there are some BC breaks involved, so we need to see if this should be done in 2.5 or wait for 3.0.

The first one is that AttributeDriver no longer extends AnnotationDriver, I think this one is fine to do it in 2.5 (this is what this PR does at the moment).

But if we want to totally remove the dependency from AttributeDriver we should probably modify the constructor to remove the ?Reader $reader = null parameter, that would mean that the $reader property would always be of type AttributeReader (which would be also modified to not implement the Reader interface).

So, should we go all the way in? or postpone it for 3.0?

[franmomu]

I've created #2596 on top of this PR to test the attribute driver

[malarzm]

Shall we have a type check on $reader being an AttributeReader?

[franmomu]

what do you mean? the constructor is receiving a ?Reader (which is deprecated), if no argument is passed it will be an AttributeReader.

[malarzm]

I was thinking about throwing an exception in case somebody would pass a AnnotationReader by accident :)

[franmomu]

ahm, the thing is that passing any Reader (including AnnotationReader) would work, I haven't changed that to maintain BC

[malarzm]

But wouldn't the driver do nothing with anything else than an AttributeReader? Or am I missing something?

[franmomu]

I added these methods: https://github.com/doctrine/mongodb-odm/pull/2502/files#diff-ac3d9c518c39d96ef76766a33e63f5115a72783ae78bc99bb4b70405a04e718eR389-R416:

    /** @return object[] */
    private function getClassAttributes(ReflectionClass $class): array
    {
        if ($this->reader instanceof AttributeReader) {
            return $this->reader->getClassAttributes($class);
        }

        return $this->reader->getClassAnnotations($class);
    }

    /** @return object[] */
    private function getMethodAttributes(ReflectionMethod $method): array
    {
        if ($this->reader instanceof AttributeReader) {
            return $this->reader->getMethodAttributes($method);
        }

        return $this->reader->getMethodAnnotations($method);
    }

    /** @return object[] */
    private function getPropertyAttributes(ReflectionProperty $property): array
    {
        if ($this->reader instanceof AttributeReader) {
            return $this->reader->getPropertyAttributes($property);
        }

        return $this->reader->getPropertyAnnotations($property);
    }

to maintain BC in case someone is passing a Reader object

[malarzm]

I think it's rather unfortunate to have the code duplicated :/ Is there any other option?

[franmomu]

Yeah I know 😞, what I was thinking is after we do this, we can make doctrine/annotations optional and then we can also deprecate AnnotationDriver.

[malarzm]

Maybe we could have a trait with shared code? Or would it be possible to have AnnotationDriver extend AttributeDriver? My biggest concern is that we're far from removing AnnotationDriver as we've never talked 3.0

[alcaeus]

Only other option I can think of is to reverse the inheritance and have AnnotationDriver extend AttributeDriver, which again would remove the code duplication.

While we've never talked about ORM 3.0, I don't think it should be a "big thing" again. Instead, it should be released at the same time as a minor release, dropping all deprecated paths from that minor release.

[malarzm]

While we've never talked about ORM 3.0, I don't think it should be a "big thing" again. Instead, it should be released at the same time as a minor release, dropping all deprecated paths from that minor release.

I'd be fine with this approach, it works well for Symfony 👍

[franmomu]

Done, but something else came up #2502 (comment)

[malarzm]

Oh boy this is ugly 🙈 Not the point of this PR obviously, just pointing out ;)

[franmomu]

it is! I've aligned the attributes, not sure if there is something else we can do

[malarzm]

I'm not sure either :D but most probably it's not worth discussing in CS repo and if we can have it at least aligned it's OK :)

[franmomu]

This could also be BC break, that class exists to have compatibility with doctrine/persistence 2 and 3

We can drop support of version 2 or leave it like this (AnnotationDriver would stop extending Doctrine\Persistence\Mapping\Driver\AnnotationDriver)

[malarzm]

We've decided in doctrine/DoctrineMongoDBBundle#824 to drop support for doctrine/persistence v2 :)

[franmomu]

Let's deal with that in #2603

[franmomu]

thanks for the reviews!

[malarzm]

Sorry I didn't manage to get back in time. Anyway, happy new year folks!