Annotation receives array of paths instead of reader

[ppaulis]

Bug Report

2.12.0 doesn't work with doctrine/doctrine-orm-module : 5.1.0.

Q	A
BC Break	yes?
Version	2.12.0

Summary

After upgrading from 2.11.1 to 2.12.0, I'm experiencing the following issue:

/var/www/html # vendor/bin/doctrine-module orm:generate-proxies
PHP Fatal error:  Uncaught Error: Call to a member function getClassAnnotations() on array in /var/www/html/vendor/doctrine/orm/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php:82
Stack trace:
#0 /var/www/html/vendor/doctrine/persistence/src/Persistence/Mapping/Driver/MappingDriverChain.php(79): Doctrine\ORM\Mapping\Driver\AnnotationDriver->loadMetadataForClass('tms\\V1\\Rest\\Sub...', Object(Doctrine\ORM\Mapping\ClassMetadata))
#1 /var/www/html/vendor/doctrine/orm/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php(133): Doctrine\Persistence\Mapping\Driver\MappingDriverChain->loadMetadataForClass('tms\\V1\\Rest\\Sub...', Object(Doctrine\ORM\Mapping\ClassMetadata))

Current behavior

Reading Metadata no longer works through the doctrine-orm-module.

In the following line:

orm/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php

Line 82 in d550364

$classAnnotations = $this->reader->getClassAnnotations($class);

$this->reader no longer contains the reader, but the array of paths:

[
    __DIR__ . '/../../module/xxx/src/V1/Rest',
    __DIR__ . '/../../module/xxx/src/V1/Rpc',
    __DIR__ . '/../../module/xxx/src',
]

Currently, my best guess is, that this class is instanciated with a missing first parameter. And the paths are then passed as first parameter in the readers place.

How to reproduce

doctrine/doctrine-orm-module : 5.1.0
doctrine/orm: 2.12.0
PHP : 8.0.16

[ppaulis]

Seems that this is happening here:
https://github.com/doctrine/DoctrineModule/blob/61dc07830df8eb7046dd2e791286b6e2854980e4/src/Service/DriverFactory.php#L75-L76

In my case, $class has the value Doctrine\ORM\Mapping\Driver\AnnotationDriver, but for this to work, it should probably be Doctrine\Persistence\Mapping\Driver\AnnotationDriver (which is deprecated).

I'll try to prepare a fix asap. @Ocramius

[ppaulis]

has already been reported here: doctrine/DoctrineModule#774
Suggested PR : doctrine/DoctrineModule#775

[greg0ire]

Did you also upgrade (voluntarily or not) doctrine/persistence? It just got a major release.

[ppaulis]

@greg0ire not possible as this is a Laminas project, and doctrine/doctrine-orm-module is not (yet) compatible to doctrine/persistence: 3.0.0

[greg0ire]

This is caused by a BC-break I introduced in #9587, where the ORM annotation driver no longer extends the one from persistence 😬

The assumption being made here is that the abstract class we are no
longer extending is not used in type declarations and instanceof checks.

Wrong assumption, @greg0ire from the past!

[Ocramius]

Ref: Roave/psr-container-doctrine#70

[Ocramius]

Suggestion on my end is to revert the offending (involuntary) BC break for now, and release 2.12.1 with that fix.

That means re-introducing some tech debt, and planning its removal for 3.0.x.

[ppaulis]

@Ocramius @greg0ire As the deprecated AnnotationDriver has been removed in doctrine/persistence: 3.0, does this mean, that we need to change the dependency "doctrine/persistence": "^2.4 || ^3" to "doctrine/persistence": "^2.4?

[greg0ire]

In ORM? I figured it does cause an issue while trying to revert: #9670, but I think conditionally defining the class based on some condition should allow us to retain compatibility with Persistence 3.

[Ocramius]

conditionally defining the class based on some condition

Depends if the consumer would observe an API change or not: remember that any API change caused by third-party packages being installed or not is still an API change -> BC break.

EDIT: anyway, I suggest:

1. reverting
2. releasing
3. creating a follow-up issue with ideas around this

[greg0ire]

The API change would occur if a consumer was not pinning doctrine/persistence to 2, unlike here: https://github.com/doctrine/DoctrineModule/blob/5.2.x/composer.json#L53

I think it's fair to have this BC break in the case persistence is upgraded to 3, because if you depend on the annotation driver and it extends the API from persistence, then you depend on persistence.

I've made an attempt at the revert because it sounded like it would be fast, but it looks like we already built on top of this: #9670 , so it won't be a simple revert I'm afraid.

[Ocramius]

I think it's fair to have this BC break in the case persistence is upgraded to 3, because if you depend on the annotation driver and it extends the API from persistence, then you depend on persistence.

The referenced symbol (Doctrine\ORM\Mapping\Driver\AnnotationDriver) is the one in in doctrine/orm: if its structure changes due to inherited API, it is a BC break also in doctrine/orm. Changing signature of AnnotationDriver based on the third-party package is still a BC break.

A suggestion could be to:

1. polyfill the inheritance in doctrine/orm, keep Doctrine\ORM\Mapping\Driver\AnnotationDriver parity (should be possible to check this with roave/backward-compatibility-check)
2. deprecate Doctrine\ORM\Mapping\Driver\AnnotationDriver completely: call it "dead"
3. create a new AnnotationDriver (completely different name, like AnnotationMappingDriver or such) and point new consumers to that

[greg0ire]

1. What do you mean exactly by "polyfill the inheritance"? Do you mean defining the persistence class if it does not exist? Something like if (!class_exists(TheClass)) { require_once 'some_file.php'; } reminds me of https://dev.to/greg0ire/how-to-deprecate-a-type-in-php-48cf but maybe in our case it's unlikely AnnotationDriver is ever used in a signature.
2. This implies finding a new name, which isn't too bad for AnnotationDriver because it's ultimately going to be removed, but if this has to be done for AttributeDriver as well, then it means we will break the naming consistency between XMLDriver, YamlDriver, PHPDriver etc.

At that point I think we would be better off merging either doctrine/DoctrineModule#776 or doctrine/DoctrineModule#775 and hope this is the only legitimate issue caused by this BC break.

[Ocramius]

What do you mean exactly by "polyfill the inheritance"? Do you mean defining the persistence class if it does not exist?

Correct: this allows moving to newer doctrine/persistence without breaking BC, but needs to be carefully checked.

it means we will break the naming consistency between

Consistency is still secondary to BC, heh.

At that point I think we would be better off merging either doctrine/DoctrineModule#776 or doctrine/DoctrineModule#775 and hope this is the only legitimate issue caused by this BC break.

The BC break is still there for anyone using doctrine/orm: remember that these modules just happen to be public projects that we are aware of, and are direct consumers of the ORM, but the source of all this is the BC break, not the consumers. All consumers will have to adapt once a new major of the ORM is out, but until then, some cruft needs to stay, for the sake of stability.

[greg0ire]

The cure seems worse than the disease here. I agree that you have to think of the consumers, but they are usually pretty vocal when it comes to BC-breaks, and so far this is the only complaint we got about it. I asked other maintainers what they thought and tried hard not to steer them in any direction, and they pointed out that the code in the module will need to be fixed for the new class anyway. I think we should merge a quick fix, and ask the maintainers of the Laminas integration to rethink that piece of code entirely, and be done with it. Same goes for https://github.com/Roave/psr-container-doctrine/blob/3.6.x/src/DriverFactory.php#L52

Note that both pieces of code got worse when AttributeDriver was introduced, and that things will likely continue to get worse every time we touch this area in ORM.

[Ocramius]

I'm not sure if my message is getting through: Roave/* and the doctrine/*Module are a spit in an ocean of consumers.

As I already mentioned, code will be improved where/when there is time to do so (especially when required in a new major release of doctrine/orm), but until then, BC is sacred. If you break BC, declare it with a new major, and consumers will adapt when they support the new version range.

[greg0ire]

a spit in an ocean of consumers

I don't doubt that, what I doubt is that there are that many type checks on AnnotationDriver. So far this has only be reported to happen in DoctrineModule and Roave/psr-container-doctrine, and it seems easier to fix both packages than implement a fix in doctrine/orm. I can certainly dedicate some time to writing a fix for psr-container-doctrine once we agree on one for DoctrineModule.

[Ocramius]

Be my guest in fixing symptoms.

Seriously, just fix the BC break, and all is good 😛

I'll gladly accept patches that improve the PSR integration too, but beware that this still broke all released versions anyway.

[greg0ire]

Seriously, just fix the BC break, and all is good 😛

I would if it wasn't that hard, but I'm more confident in fixing the symptoms as you say 😅 Hopefully people who upgrade the ORM will also be able to upgrade both other packages 🤞

[derrabus]

While I agree that changing the inheritance chain is a potential BC break, I'm not sure I 100% follow the argument of the BC break here. The code in question assumes that all driver classes that require an annotation reader are child classes of the AnnotationDriver class from doctrine/persistence and vice versa. This assumption did fail previously (doctrine/DoctrineModule#728), it fails now and is doomed to fail again.

That being said, we have to acknowledge that this flawed piece of code has existed for roughly a decade now and that we would probably need to patch multiple legacy branches of DoctrineModule because applications that use older PHP versions than 7.4 have to pin DoctrineModule to an old version while still being able to upgrade to the latest ORM.

My proposal to tackle the issue: #9671

We would still need to change DoctrineModule though, but we can do so when working on making it compatible with Persistence 3.

[Ocramius]

Awesome, thanks!