Fix Doctrine Connection service alias

[mrsombre]

Fix alias name for Doctrine Connection service. Now \Doctrine\DBAL\Connection retreived from _getEntityManager()->getConnection() and grabService(\Doctrine\DBAL\Connection) are different which is a bug.

[TavoNiievez]

@mrsombre It seems that currently our test project https://github.com/Codeception/symfony-module-tests (which is used for CI and test execution) is not ready for this change.

In order to merge this I would need you to also adapt the test project and (somehow) make the tests pass.

As soon as I can verify that your change works and that we are able to test it through the test project, I will gladly merge this.

Be sure to follow the contribution guidelines if you have any doubts and you can report any questions here, I (or someone else) will be happy to help you.

[mrsombre]

@TavoNiievez i see. Root of the problem in fact, that now connection is real and doctrine commits transaction, so rollback hack doesn't work anymore.
I'm looking into this problem. Trying to find a good solution to prevent transaction closing before module can revert it.

[mrsombre]

The root cause in vendor/doctrine/doctrine-bundle/DoctrineBundle.php:138:

• Every request (like form submit) cause to reboot kernel
• This cause doctrine bundle to close active connections
• Transaction opened by Doctrine module caused to commit / close too
• Then new connection opened for data insertion in the controller, but the Doctrine transaction hack hasn't work anymore and data are actually written to DB

I prevent this with a bit ugly solution using reflections, otherwise I can't anyhow override service param to prevent doctrine to close connections on reboot.

[mrsombre]

Hi, any updates on this, anything to fix/ improve? @TavoNiievez

[TavoNiievez]

@mrsombre I'm fine with changes, even using Reflections.
Still I think we should have a test for this.

[mrsombre]

Unfortunately, compiled container cannot be modified anyhow, so reflections are only option.
Any advices how implement test before change merged into master?

[TavoNiievez]

The following is an oversimplification, but it is a way to check for these changes. Currently this test should fail... and with your changes it should pass.

    /**
     * @see https://github.com/Codeception/module-symfony/pull/129
     */
    public function aDescriptiveTestName(FunctionalTester $I)
    {
        $containerDbalConnection = $I->grabService('doctrine.dbal.default_connection');
        $emDbalConnection = $I->getEm()->getConnection();
        $I->assertSame($emDbalConnection, $containerDbalConnection);
    }

Regarding 'where' to put this, I think symfony-module-tests/tests/Functional/IssuesCest.php would be a good place.

[mrsombre]

Yeah, I see how to write a test. The question was about how I'll add a test if it can't pass. Btw it's like I add test as PR, then we merge this PR and then test, right?

[TavoNiievez]

Yes, the idea is that:

1. You present a PR in symfony-module-tests (tests should fail).
2. I apply these changes locally and test it works. Then I merge this PR with master.
3. When re-executing the build in the PR of symfony-module-tests the tests should pass. That PR is merged.
4. Finally, a tag is created and the change is released.

I know it is a somewhat complex workflow, but we are open to suggestions.

[mrsombre]

Bundle tests with the project? xD I got an idea, shall add tests soon, thanks for your help, this change is important for our workflow (mixing orm with dbal).

[TavoNiievez]

@mrsombre Please take a look at: https://github.com/pimcore/pimcore/pull/10331/checks?check_run_id=3619279351#step:11:1078

these changes appear to be causing problems in Pimcore when trying to update.