-
Notifications
You must be signed in to change notification settings - Fork 103
Compatibility with Doctrine ORM 3.0 and DBAL 4.0 #535
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
Conversation
3fc1093
to
e2635ca
Compare
tests/bootstrap.php
Outdated
!is_file(__DIR__ . '/../vendor/doctrine/orm/src/Mapping/Driver/AnnotationDriver.php') | ||
&& !is_file(__DIR__ . '/../vendor/doctrine/orm/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php') | ||
) { | ||
require_once __DIR__ . '/../compatibility/AnnotationDriver.php'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice idea indeed in order to have less code to update currently.
But I feel like it could still be useful in long term (certainly in another PR which can be merged before or after the doctrine bump) to add Attributes everywhere Annotation is used in tests in order to tests them too. Also, rather having some time data/
and data-attributes
folders with duplicated tests, a unique data/
folder can be used with both annotation and attribute in the same class without any issue.
This way the logic
if (! class_exists(AnnotationDriver::class) {
// use AttributeReader
} else {
// use AnnotationReader (or decide based on PHP version if wanted)
}
can be use in the entity-manager.php.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to add Attributes everywhere Annotation
I disagree with this approach. I consider existing tests to be immutable. By changing them we might start testing different things by accident than originally intended.
e2635ca
to
e978bec
Compare
f20fe7b
to
0046cf0
Compare
For all the
PHPStan errors, it's because the I fixed them by adding I can extract the commit from #531 if you want to cherry pick it. (also, do you want / can I help for something ?) |
Please do, that would be helpful 😊 |
Commit is here 3ebf91f |
Can you send it as a PR with base branch doctrine-orm-3, so I can just merge it from my phone? Thanks. |
|
…e ORM 3 for unit tests
6009f91
to
359ccf3
Compare
359ccf3
to
544bbea
Compare
Signing off today, if you could look at any of these failures https://github.com/phpstan/phpstan-doctrine/actions/runs/7833385749 @VincentLanglet, I'd appreciate it :) Thanks. |
I fixed some tests on #540 I let you the PHPStan error, since I'm not sure how you prefer to fix them. |
I tried this branch and everythings work fine for me
👏 |
Related: doctrine/orm#11240 Related: doctrine/orm#11241
10869b3
to
453e6fa
Compare
hey @ondrejmirtes it looks like this was completed and merged into I'm running: phpstan-doctrine When I upgraded Am I missing something, or is orm3 only supported WITH dbal4? I opened another issue before finding this one. EDIT: bumping to dbal4 is throwing different errors now:
|
No description provided.