Fix create snapshots

[jordisala1991]

Subject

Broken only on 4.x due an update of phpstan with this copy paste typo.

[VincentLanglet]

Do you see an easy functional test to add ?

[jordisala1991]

Does a functional test is possible ?

Not sure, my functional test for createsnapshotcommand didn't catch this bug.

Maybe a functional test if it is possible to create snapshots from the sonata admin interface.

[eerison]

Wait it's the same problem for cleanup snapshot: f8022f6#diff-112198c8b3816a6af7235c0c526ba39544d633700e4538d9ab1fc7e7c770d8f6R38

maybe we should revert the changed made there: #1536

[jordisala1991]

Wait it's the same problem for cleanup snapshot: f8022f6#diff-112198c8b3816a6af7235c0c526ba39544d633700e4538d9ab1fc7e7c770d8f6R38

maybe we should revert the changed made there: #1536

NOpe, the problem is not the if, is the second condition changed from commit to beginTransaction, because I copy pasted. I double checked the other file, and I didn't make this error there.

[eerison]

Other thing I guess we should create a test to see if beginTransaction and commit are called
But what do you think we add this into the managerInterface? and use just

$this->snapshotManager->commit();

this way we could remove the couple from Doctrine methods and let the manager handle this!

in the end I'm proposing to follow the demeter law

[eerison]

Wait it's the same problem for cleanup snapshot: f8022f6#diff-112198c8b3816a6af7235c0c526ba39544d633700e4538d9ab1fc7e7c770d8f6R38
maybe we should revert the changed made there: #1536

NOpe, the problem is not the if, is the second condition changed from commit to beginTransaction, because I copy pasted. I double checked the other file, and I didn't make this error there.

True, it's using commit, then it could be other issue! ok good

[eerison]

Other thing I guess we should create a test to see if beginTransaction and commit are called But what do you think we add this into the managerInterface? and use just

$this->snapshotManager->commit();

this way we could remove the couple from Doctrine methods and let the manager handle this!

in the end I'm proposing to follow the demeter law

in theory we should add here:

https://github.com/sonata-project/sonata-doctrine-extensions/blob/2.x/src/Model/ManagerInterface.php

or Add a new interface like ManagerTransactionInterface and add

commit and beginTransaction there and make SnapshotManagerInterface extends this class too

[VincentLanglet]

Other thing I guess we should create a test to see if beginTransaction and commit are called But what do you think we add this into the managerInterface? and use just

$this->snapshotManager->commit();

this way we could remove the couple from Doctrine methods and let the manager handle this!

You're kinda creating coupling then.
Because not all managers are able to handle transaction.

It would be better to create a new interface here: https://github.com/sonata-project/sonata-doctrine-extensions/tree/2.x/src/Model

Which would be TransactionalManagerInterface with beginTransaction and commit.
Then the BaseEntityManager will implements it.

Do you want to do the PR @eerison ?

[eerison]

TransactionalManagerInterface

yeah it's the second option that I suggested 😄

But Yeah I can open a PR with TransactionalManagerInterface!

Create small interface are better! you are right ;)

[jordisala1991]

But that change is independent from this fix right?

[eerison]

But that change is independent from this fix right?

well I can open a new PR improving this, and creating for test it!

[VincentLanglet]

Let's merge this so far