Adding TransactionalManagerInterface

[eerison]

Subject

I am targeting this branch, because it has the support for the php 7.4 version.

Related with sonata-project/SonataPageBundle#1550.

Changelog

### Added
- Added `TransactionalManagerInterface::class` interface.

### Changed
- Implements `TransactionalManagerInterface` in `BaseEntityManager` abstract class

[VincentLanglet]

You need to implement it in https://github.com/sonata-project/sonata-doctrine-extensions/blob/2.x/src/Entity/BaseEntityManager.php

[eerison]

You need to implement it in https://github.com/sonata-project/sonata-doctrine-extensions/blob/2.x/src/Entity/BaseEntityManager.php

BaseEntityManager or BaseManager I don't really get what is the difference between both?

[VincentLanglet]

You need to implement it in https://github.com/sonata-project/sonata-doctrine-extensions/blob/2.x/src/Entity/BaseEntityManager.php

BaseEntityManager or BaseManager I don't really get what is the difference between both?

BaseManager is a base for both doctrineORM and doctirneMOngo.
BaseEntityManager is a bath for doctrineORMManagers

There is transaction in EntityManager so we can add it to BaseEntityManager.
I'm not sure the same exists to MongoManager, and even if it exists, the impelmentation won't be the same

[VincentLanglet]

Linter is failing

[eerison]

Linter is failing

the phpcs I could run locally, But psalm doesn't work :/, Then I do not know it it was fixed!

[eerison]

I added the return type for tests

[eerison]

sorry for keeping you running the pipelines :(

[eerison]

let me know if it's missing something!

[eerison]

And what was the decision 👀

[jordisala1991]

Have mixed feelings about wrapping in our own exception. What are the benefits?

I have one con, people used to try catch normal doctrine exception for commit, will have to think about this new exception.

[eerison]

To solve this we could extends from doctrine exception!
But I would say that usually will be used Throwable or DoctrineException into the code, I guess this TransactionException won't be used!

IMO we could remove this try/catch and let Doctrine exception raise!

[VincentLanglet]

People are using commit on the Sonata EntityManager, not the doctrine one. So it would be weird to have doctrine exception.

Also, when you are working with the interface, ho do you know if it will be orm exception, mongo exception, something else exception etc.

The over option is to just use a throwable annotation... I thought it could be good to prefer a domain exception

[eerison]

Also, when you are working with the interface, ho do you know if it will be orm exception, mongo exception, something else exception etc.

In this case I'll know, because we are implementing only for EntityManager, mongo entity doesn't use this interface!

People are using commit on the Sonata EntityManager, not the doctrine one. So it would be weird to have doctrine exception.

Yeah I got your point, I agree that it's a bit strange implements a Sonata interface and throw an exception of other dependency!

well we have pros and cons 😄

But if we going to use a custom Exception I guess we should extends of DoctrineException 👀

Note: But looking for the benefit side, I would say it won't be too helpful, usually the catch will have only DoctrineException to catch some errors!

and maybe it will be a BC because for now we are throwing DoctrineException and if we return CustomException it won't go into the caches that already exist catching Doctrine stuffs.

[VincentLanglet]

In this case I'll know, because we are implementing only for EntityManager, mongo entity doesn't use this interface!

When I use this interface, I'm not supposed to know which one are implementing it and which one don't.
Maybe ORM will stop transaction one day. Maybe Mongo will start one day.
The goal is too decouple from the implementation.

But if we going to use a custom Exception I guess we should extends of DoctrineException 👀

I'm not sure you have something like a BaseDoctrineException, used by both ORM and Mongo.

and maybe it will be a BC because for now we are throwing DoctrineException and if we return CustomException it won't go into the caches that already exist catching Doctrine stuffs.

It's not a BC break since we're adding a method/interface.
If you use ->getEntityManager->commit you have the ORM exception
If you use ->commit you have the Sonata exception

[jordisala1991]

I think I found at least another case on SonataMediaBundle where this could be useful.

[VincentLanglet]

I think I found at least another case on SonataMediaBundle where this could be useful.

Can you explain what you have in mind ?

[jordisala1991]

Nothing, I have searched where do we use transactions and are only on PageBundle