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

Make DoctrineBundle compatible with ORM 2.9 #1327

Merged
merged 4 commits into from
May 6, 2021

Conversation

beberlei
Copy link
Member

@beberlei beberlei commented Apr 18, 2021

In ORM 2.9 similar to DBAL 2.11 we introduce a new way of injecting the EntityManager into commands via registries/providers. These register the option em, which would lead to conflicts in the proxy commands due to redefinition of the option.

The same change was already made for dbal in 86d2469

Once ORM 2.9 is out we can think of increasing the doctrine/orm dependency from 2.6 to 2.9, since both support PHP 7.1. Then we can replace the whole proxy command code with proper dependency injection and shipping an entity manager provider.

This PR adds one test for one proxy command to test the variable entity manager option behavior. In addition since commands are services for a while already, the ContainerTest is also extended to fetch two out of the container

Copy link
Member

@ostrolucky ostrolucky left a comment

Choose a reason for hiding this comment

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

Just a nitpick. This PR LGTM otherwise, thanks for doing it 👍 Merge with squash.

/**
* @return MockObject|Kernel
*/
private function setupKernelMocks()
Copy link
Member

Choose a reason for hiding this comment

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

return type should still be added for either Kernel or MockObject

@simPod
Copy link
Contributor

simPod commented Apr 19, 2021

Also note the parent option has default value while this has none.

->addOption('em', null, InputOption::VALUE_REQUIRED, 'Name of the entity manager to operate on', 'default')

I'm not entirely sure whats correct, though having default set as 'default' and not null disables AbstractManagerRegistry's functionality to use real default ($this->defaultManager) https://github.com/doctrine/persistence/blob/2.2.x/lib/Doctrine/Persistence/AbstractManagerRegistry.php#L143-L146 causing

Doctrine ORM Manager named "default" does not exist.

Issue for this doctrine/orm#8643

kbond added a commit to kbond/foundry that referenced this pull request Apr 19, 2021
@beberlei
Copy link
Member Author

@simPod hm thats annoying, i didn't want to make the manager name nullable in ORM if it could be avoided. :( Seems it is necessary just for backwards compatibility in Bundle

@simPod
Copy link
Contributor

simPod commented Apr 20, 2021

Understand. I'm using only one EM for now (used to have more so it has custom name). Though it's not working with this change and it requires to specify em option which is also very annoying when there's only one EM 😄

@ostrolucky ostrolucky force-pushed the ConsoleCommandFixes branch 5 times, most recently from 445431b to bd181de Compare May 6, 2021 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants