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

allow to use Doctrine/Persistence:^2 #1305

Merged
merged 3 commits into from
Jan 9, 2021

Conversation

jkabat
Copy link
Contributor

@jkabat jkabat commented Oct 29, 2020

Q A
Branch? 2.0
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT

@coveralls
Copy link

coveralls commented Oct 29, 2020

Coverage Status

Coverage remained the same at 83.645% when pulling be594f1 on jkabat:fix-doctrine-persistance into 14206fb on liip:2.x.

$omClassName = LegacyObjectManager::class;
} elseif (interface_exists(ObjectManager::class)) {
$omClassName = ObjectManager::class;
} else {
$this->markTestSkipped('Requires the doctrine/orm package.');
Copy link
Contributor

Choose a reason for hiding this comment

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

As doctrine/orm is in require-dev it will always be installed when running tests.Therefore you can remove the else statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

composer.json Outdated
@@ -35,6 +35,7 @@
"aws/aws-sdk-php": "^2.4",
"doctrine/cache": "^1.1",
"doctrine/orm": "^2.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this require-dev entry is useless. There isn't any code in this bundle which depends on Doctrine\\ORM!

We should remove it and keep doctrine/persistence as the true optional dependency thus we only provide an AbstractDoctrineLoader to help developers to implement their own doctrine ORM/ODM loader.

@lsmith77
Copy link
Contributor

lsmith77 commented Jan 5, 2021

I just merged #1337

@fbourigault
Copy link
Contributor

Could you rebase? #1337 replaced the doctrine/orm dev dependency with doctrine/persistence ^1.3.

@lsmith77 lsmith77 changed the base branch from master to 2.x January 5, 2021 12:49
@jkabat
Copy link
Contributor Author

jkabat commented Jan 7, 2021

@fbourigault Ill do that today/tomorrow.

@jkabat jkabat force-pushed the fix-doctrine-persistance branch from 2cd9a7d to 949996d Compare January 7, 2021 10:01
@jkabat
Copy link
Contributor Author

jkabat commented Jan 7, 2021

rebased and fixed based on suggestions

@lsmith77 lsmith77 merged commit ef53840 into liip:2.x Jan 9, 2021
@lsmith77
Copy link
Contributor

lsmith77 commented Jan 9, 2021

thank you!

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.

4 participants