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

[Messenger] Add a middleware that wraps all handlers in one Doctrine transaction. #26647

Closed
wants to merge 24 commits into
base: master
from

Conversation

@Nyholm
Member

Nyholm commented Mar 23, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets na
License MIT
Doc PR coming up

This is greatly inspired by SimpleBus. This middleware will rollback a transaction if the message handler throws an exception.

@Nyholm Nyholm changed the title from Add a middleware that wraps all handlers in one Doctrine transaction. to [Messenger] Add a middleware that wraps all handlers in one Doctrine transaction. Mar 23, 2018

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Mar 23, 2018

@@ -984,6 +984,12 @@ private function addMessengerSection(ArrayNodeDefinition $rootNode)
->end()
->end()
->end()
->arrayNode('doctrine')

This comment has been minimized.

@sroze

sroze Mar 23, 2018

Member

I'd suggest making it more explicit: doctrine_transaction.

->arrayNode('doctrine')
->canBeEnabled()
->children()
->scalarNode('entity_manager')->info('The name of the entity manager to use')->defaultValue('default')->end()

This comment has been minimized.

@sroze

sroze Mar 23, 2018

Member

entity_manager => entity_manager_name to clarify is not a service name?

class WrapsMessageHandlingInTransaction implements MiddlewareInterface
{
/**
* @var ManagerRegistry

This comment has been minimized.

@sroze

sroze Mar 23, 2018

Member

This can be removed.

private $managerRegistry;
/**
* @var string

This comment has been minimized.

@sroze

sroze Mar 23, 2018

Member

This can be removed as well.

private $entityManagerName;
/**
* @param ManagerRegistry $managerRegistry

This comment has been minimized.

@sroze

sroze Mar 23, 2018

Member

All of these as well :)

public function handle($message, callable $next)
{
/** @var $entityManager EntityManagerInterface */

This comment has been minimized.

@sroze

sroze Mar 23, 2018

Member

I'd suggest to check with instanceof the type, rather than assuming it is always going to be EntityManagerInterface. If not, we can simply throw an InvalidArgumentException.

$result = null;
try {
$entityManager->transactional(

This comment has been minimized.

@sroze

sroze Mar 23, 2018

Member

It looks like to me that using the beginTransaction, commit and rollback methods would be much more explicit that this callback-based transaction that forces you to do this weird resetManager later :)

@Nyholm

This comment has been minimized.

Member

Nyholm commented Mar 23, 2018

Thank you for the review

@@ -1445,6 +1445,12 @@ private function registerMessengerConfiguration(array $config, ContainerBuilder
$container->getDefinition('messenger.sender_locator')->replaceArgument(0, $senderLocatorMapping);
$container->getDefinition('messenger.asynchronous.routing.sender_locator')->replaceArgument(1, $messageToSenderIdsMapping);
if ($config['doctrine']['enabled']) {

This comment has been minimized.

@stloyd

stloyd Mar 23, 2018

Contributor

Array keys mismatch after recent rename.

This comment has been minimized.

@Nyholm

Nyholm Mar 23, 2018

Member

Thanks!

@@ -1445,6 +1445,12 @@ private function registerMessengerConfiguration(array $config, ContainerBuilder
$container->getDefinition('messenger.sender_locator')->replaceArgument(0, $senderLocatorMapping);
$container->getDefinition('messenger.asynchronous.routing.sender_locator')->replaceArgument(1, $messageToSenderIdsMapping);
if ($config['doctrine']['enabled']) {
$container->getDefinition('messenger.middleware.doctrine_transaction')->replaceArgument(1, $config['doctrine']['entity_manager']);

This comment has been minimized.

@chalasr

chalasr Mar 23, 2018

Member

$config['doctrine'] should be $config['doctrine_transaction'] and ['entity_manager'] should be ['entity_manager']['name'], we will need tests for this if it's not already covered

This comment has been minimized.

@Nyholm

Nyholm Mar 23, 2018

Member

Thank you

@chalasr chalasr added the Messenger label Mar 23, 2018

@Nyholm

This comment has been minimized.

Member

Nyholm commented Mar 23, 2018

@chalasr The tests currently fails due invalid xml config. Do I need to modify the symfony-1.0.xsd?

@chalasr

This comment has been minimized.

Member

chalasr commented Mar 23, 2018

@Nyholm yes

@Nyholm

This comment has been minimized.

Member

Nyholm commented Mar 23, 2018

I've got an error I cannot figure out:

Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\XmlFrameworkExtensionTest::testMessenger
Symfony\Component\Config\Definition\Exception\InvalidConfigurationException: Unrecognized options "0, 1" under "framework.messenger.routing.sender"

Any help would be appreciated.

$entityManager = $this->managerRegistry->getManager($this->entityManagerName);
if (!$entityManager instanceof EntityManagerInterface) {
throw new \InvalidArgumentException(sprintf('The ObjectManager with name "%s" must be an instance of EntityManagerInterface', $this->entityManagerName));

This comment has been minimized.

@yceruto

yceruto Mar 23, 2018

Contributor

This exception seems unnecessary IMHO, if $this->entityManagerName exists the $entityManager should be always an EntityManagerInterface instance (see), otherwise if it doesn't exists Doctrine library throws its own \InvalidArgumentException (see).

This comment has been minimized.

@Nyholm

Nyholm Mar 23, 2018

Member

Hm.. But the doc block says ObjectManager..

This comment has been minimized.

@yceruto

yceruto Mar 23, 2018

Contributor

Ah! I see, NVM, sorry for the noise.

This comment has been minimized.

@Nyholm

Nyholm Mar 23, 2018

Member

Do not be sorry. Thank you for challenging this =)

->arrayNode('doctrine_transaction')
->canBeEnabled()
->children()
->scalarNode('entity_manager_name')->info('The name of the entity manager to use')->defaultValue('default')->end()

This comment has been minimized.

@yceruto

yceruto Mar 23, 2018

Contributor

Could we make it null by default and let Doctrine determine the default name? (see)

This comment has been minimized.

@Nyholm

Nyholm Mar 23, 2018

Member

Excellent idea. Thanks

*
* @author Tobias Nyholm <tobias.nyholm@gmail.com>
*/
class WrapsMessageHandlingInTransaction implements MiddlewareInterface

This comment has been minimized.

@ro0NL

ro0NL Mar 24, 2018

Contributor

shouldnt this be moved tot the doctrine bridge instead? Otherwise i expect naming like DoctrineMessageWrappingMiddleware or so :)

This comment has been minimized.

@ogizanagi

ogizanagi Mar 24, 2018

Member

What about DoctrineTransactionMiddleware ?

@sroze

This comment has been minimized.

Member

sroze commented Mar 26, 2018

@Nyholm could you move the middleware to the DoctrineBridge and let Doctrine decide the default manager name?

@Nyholm

This comment has been minimized.

Member

Nyholm commented Mar 26, 2018

Done

<xsd:complexType name="messenger_doctrine_transaction">
<xsd:sequence>
<xsd:element name="entity-manager-name" type="xsd:string" />

This comment has been minimized.

@stof

stof Mar 26, 2018

Member

this one could be an attribute rather than a child element IMO

This comment has been minimized.

@Nyholm

Nyholm Mar 26, 2018

Member

Could you please assist me here? I've had a hard time making this work. That is why the tests are failing.

This comment has been minimized.

@Nyholm

Nyholm Mar 26, 2018

Member

Fixed!

This comment has been minimized.

@sroze

sroze Mar 26, 2018

Member

Last thing I guess: can you move this to be an attribute? It would mean this XML would look like that instead:

    <framework:config>
        <framework:messenger>
            <framework:doctrine-transaction entity-manager-name="foobar" />
        </framework:messenger>
    </framework:config>
@Nyholm

This comment has been minimized.

Member

Nyholm commented Mar 26, 2018

Finally done with the config. It took me way more time than I want to admit. Anyhow. I learnt something =)

Example XML:

<framework:config>
        <framework:messenger>
            <framework:routing message-class="App\Bar">
                <framework:sender service="sender.bar"></framework:sender>
                <framework:sender service="sender.biz"></framework:sender>
            </framework:routing>
            <framework:routing message-class="App\Foo">
                <framework:sender service="sender.foo"></framework:sender>
            </framework:routing>
        </framework:messenger>
    </framework:config>

Example Yaml:

framework:
    messenger:
        routing:
            'App\Bar': ['sender.bar', 'sender.biz']
            'App\Foo': 'sender.foo'
if ($config['doctrine_transaction']['enabled']) {
if (!class_exists(DoctrineTransactionMiddleware::class)) {
throw new LogicException('You must install symfony/doctrine-bridge to use the "DoctrineTransactionMiddleware"');

This comment has been minimized.

@sroze

sroze Mar 26, 2018

Member

I'd suggest the following:

The Doctrine transaction middleware is only available when the doctrine bridge is installed. Try running "composer require symfony/doctrine-bridge".

<xsd:complexType name="messenger_doctrine_transaction">
<xsd:sequence>
<xsd:element name="entity-manager-name" type="xsd:string" />

This comment has been minimized.

@sroze

sroze Mar 26, 2018

Member

Last thing I guess: can you move this to be an attribute? It would mean this XML would look like that instead:

    <framework:config>
        <framework:messenger>
            <framework:doctrine-transaction entity-manager-name="foobar" />
        </framework:messenger>
    </framework:config>
<framework:config>
<framework:messenger>
<framework:routing message-class="App\Bar">
<framework:sender service="sender.bar"></framework:sender>

This comment has been minimized.

@sroze

sroze Mar 26, 2018

Member

I guess you can end them with /> instead of the full XML closing tag.

Nyholm added some commits Mar 23, 2018

cs
@Nyholm

This comment has been minimized.

Member

Nyholm commented Mar 27, 2018

Thank you for the reviews. I've updated accordingly and rebased the PR on master.

framework:
messenger:
doctrine_transaction:
entity_manager_name: 'foobar'

This comment has been minimized.

@ro0NL

ro0NL Mar 27, 2018

Contributor

missing eol :}

This comment has been minimized.

@Nyholm

Nyholm Mar 27, 2018

Member

Thank you

@chalasr

This comment has been minimized.

Member

chalasr commented Mar 27, 2018

Just wondering, should this new config be wrapped in a middlewares node for explicitness? Also thinking about #26652, will we want the ability to define a different middleware set per bus, overriding the default one?

fixed

@Nyholm

This comment has been minimized.

Member

Nyholm commented Mar 27, 2018

Good idea. I've updated the PR

$config['middlewares'] = array();
return $config;
})
->end()

This comment has been minimized.

@Nyholm

Nyholm Mar 27, 2018

Member

Is this the proper way of making sure that there always is a config key named "middlewares"?

This comment has been minimized.

@Nyholm

Nyholm Mar 27, 2018

Member

No, it is not. We should of course use ->addDefaultsIfNotSet()

Nyholm added some commits Mar 27, 2018

cs
@Nyholm

This comment has been minimized.

Member

Nyholm commented Mar 27, 2018

Example Yaml:

framework:
    messenger:
        routing:
            'App\Bar': ['sender.bar', 'sender.biz']
            'App\Foo': 'sender.foo'
        middlewares: 
            doctrine_transaction: ~
@sroze

sroze approved these changes Mar 27, 2018

@sroze

This comment has been minimized.

Member

sroze commented Mar 27, 2018

Just wondering, should this new config be wrapped in a middlewares node for explicitness? Also thinking about #26652, will we want the ability to define a different middleware set per bus, overriding the default one?

@chalasr I think we might want to do this in the future. I guess it really depends on the "demand" as it isn't that hard to register your own bus services yourself either.

@sroze

This comment has been minimized.

Member

sroze commented Mar 27, 2018

Thank you @Nyholm.

@sroze sroze closed this in ea6d861 Mar 27, 2018

@Nyholm Nyholm deleted the Nyholm:middleware-doctrine branch Mar 27, 2018

@Nyholm

This comment has been minimized.

Member

Nyholm commented Mar 27, 2018

Thank you for merging

@fabpot

This one has been merged too fast. I don't want to have Doctrine specific code in FrameworkBundle. It should be moved to DoctrineBundle.

$entityManager = $this->managerRegistry->getManager($this->entityManagerName);
if (!$entityManager instanceof EntityManagerInterface) {
throw new \InvalidArgumentException(sprintf('The ObjectManager with name "%s" must be an instance of EntityManagerInterface', $this->entityManagerName));

This comment has been minimized.

@fabpot

fabpot Mar 27, 2018

Member

missing dot at the end of the exception message.

->arrayNode('middlewares')
->addDefaultsIfNotSet()
->children()
->arrayNode('doctrine_transaction')

This comment has been minimized.

@fabpot

fabpot Mar 27, 2018

Member

This should not be configured via Framework, but via DoctrineBundle. We don't have any other Doctrine config here AFAIR.

This comment has been minimized.

@sroze

sroze Mar 27, 2018

Member

@fabpot are you happy about the middleware being in the bridge, right?

This comment has been minimized.

@fabpot

This comment has been minimized.

@sroze

sroze Mar 27, 2018

Member

@fabpot actually, the cache also relies on Doctrine in FrameworkBundle. Note that this doesn't configure Doctrine either it justs allows enables the middleware. I'll submit a PR to remove it from here but I believe it might makes sense (follows the same pattern as cache configuration) to be here 🤔

This comment has been minimized.

@sroze

This comment has been minimized.

@chalasr

chalasr Mar 27, 2018

Member

I also think this makes sense as is. Moving it to doctrine bundle would mean configuring messenger middlewares in two different places, I can't think of a good final result.
We enable logging features for some components via framework config in the same way

This comment has been minimized.

@ogizanagi

ogizanagi Mar 27, 2018

Member

I agree with @chalasr , it's only a feature flag. It does no harm having this in the fwb whereas having to configure middlewares in 2 different config keys is really bad, DX wise. WDYT?

@fabpot

This comment has been minimized.

Member

fabpot commented Mar 27, 2018

/cc @sroze

@sroze

This comment has been minimized.

Member

sroze commented Mar 27, 2018

@fabpot OK, PR on the way.

fabpot added a commit that referenced this pull request Apr 4, 2018

feature #26684 [Messenger] Remove the Doctrine middleware configurati…
…on from the FrameworkBundle (sroze)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[Messenger] Remove the Doctrine middleware configuration from the FrameworkBundle

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #26647
| License       | MIT
| Doc PR        | ø

As per @fabpot's request, remove the enabling feature of the DoctrineMiddleware from the FramworkBundle.

/cc @Nyholm

Commits
-------

27a8b1d Remove the Doctrine middleware configuration from the FrameworkBundle

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment