Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adapt to new type hierarchy #776

Closed
wants to merge 2 commits into from

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Apr 20, 2022

In recent versions of ORM and ODM, annotation driver no longer extend a
base class in doctrine/persistence. Instead of testing for that base
class, detecting the number of constructor arguments seems like a more
robust way to figure out what arguments to pass.

Fixes #774

Alternative to #775

@greg0ire
Copy link
Member Author

I tried to cover my change with a new test, but am having a hard time figuring out what I'm doing wrong. I'm getting the following error:

Laminas\ServiceManager\Exception\ServiceNotFoundException: Unable to resolve service "doctrine.cache.array" to a factory; are you certain you provided it during configuration?

@greg0ire greg0ire requested review from driehle and TomHAnderson and removed request for driehle April 20, 2022 21:20
@greg0ire greg0ire force-pushed the adapt-to-new-type-hierarchy branch from c2c3429 to 0ce1129 Compare April 20, 2022 21:21
@ppaulis
Copy link

ppaulis commented Apr 21, 2022

I tried to cover my change with a new test, but am having a hard time figuring out what I'm doing wrong. I'm getting the following error:

Laminas\ServiceManager\Exception\ServiceNotFoundException: Unable to resolve service "doctrine.cache.array" to a factory; are you certain you provided it during configuration?

The following will do the trick in your test:

$serviceManager->setService('doctrine.cache.array', new \Doctrine\Common\Cache\ArrayCache());

In recent versions of ORM and ODM, annotation driver no longer extend a
base class in doctrine/persistence. Instead of testing for that base
class, detecting the number of constructor arguments seems like a more
robust way to figure out what arguments to pass.
@greg0ire greg0ire force-pushed the adapt-to-new-type-hierarchy branch from bd10f92 to 9b5d144 Compare April 21, 2022 12:45
$factory = new DriverFactory('testDriver');
$driver = $factory->__invoke($serviceManager, AnnotationDriver::class);
$this->assertInstanceOf(AnnotationDriver::class, $driver);
assert($driver instanceof AnnotationDriver);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this is necessary, but it is. PHPstan and phpstan-phpunit are installed and up to date, yet $driver is not narrowed down to AnnotationDriver.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could have solved that with a simple comment:

/** @var AnnotationDriverORM|AnnotationDriverODM|AnnotationDriverPersistence $driver */
$driver  = $factory->__invoke($serviceManager, AnnotationDriver::class);

but PHPCS forced me to do the assert(...). This seemed to be the only way to make both tools happy.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect phpstan not to need this thanks to line 117. I think that works in my other projects, so there must be a configuration issue somewhere. It's mentioned in this README: https://github.com/phpstan/phpstan-phpunit

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found the reason: we were only using that extension to add extra rules, not to add functionality.

@greg0ire greg0ire marked this pull request as ready for review April 21, 2022 12:46
@greg0ire greg0ire requested a review from derrabus April 22, 2022 11:57
@greg0ire
Copy link
Member Author

Now that doctrine/orm#9671 is merged, this MR makes sense only after doctrine/persistence 3 is supported. Reworking the DriverFactory more in depth before adding support might be better than merging this.

@greg0ire greg0ire closed this Apr 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

incompatible with doctrine/orm >= 2.12.0
2 participants