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

Nyholm · 2018-03-23T08:41:49Z

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.

sroze · 2018-03-23T10:02:28Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

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


I'd suggest making it more explicit: doctrine_transaction.

sroze · 2018-03-23T10:02:53Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

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


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

sroze · 2018-03-23T10:03:11Z

src/Symfony/Component/Messenger/Middleware/WrapsMessageHandlingInTransaction.php

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


This can be removed.

sroze · 2018-03-23T10:03:20Z

src/Symfony/Component/Messenger/Middleware/WrapsMessageHandlingInTransaction.php

+    private $managerRegistry;
+
+    /**
+     * @var string


This can be removed as well.

sroze · 2018-03-23T10:03:27Z

src/Symfony/Component/Messenger/Middleware/WrapsMessageHandlingInTransaction.php

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


All of these as well :)

sroze · 2018-03-23T10:05:55Z

src/Symfony/Component/Messenger/Middleware/WrapsMessageHandlingInTransaction.php

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


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.

sroze · 2018-03-23T10:06:42Z

src/Symfony/Component/Messenger/Middleware/WrapsMessageHandlingInTransaction.php

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


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 · 2018-03-23T10:27:55Z

Thank you for the review

stloyd · 2018-03-23T10:30:02Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -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']) {


Array keys mismatch after recent rename.

chalasr · 2018-03-23T10:30:54Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -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']);


$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

Nyholm · 2018-03-23T11:55:25Z

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

chalasr · 2018-03-23T11:57:01Z

@Nyholm yes

Nyholm · 2018-03-23T14:50:28Z

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.

yceruto · 2018-03-23T18:42:27Z

src/Symfony/Component/Messenger/Middleware/WrapsMessageHandlingInTransaction.php

+        $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 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).

Hm.. But the doc block says ObjectManager..

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

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

yceruto · 2018-03-23T18:46:54Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

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


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

Excellent idea. Thanks

ro0NL · 2018-03-24T08:22:23Z

src/Symfony/Component/Messenger/Middleware/WrapsMessageHandlingInTransaction.php

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


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

What about DoctrineTransactionMiddleware ?

sroze · 2018-03-26T09:56:10Z

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

Nyholm · 2018-03-26T10:50:04Z

Done

stof · 2018-03-26T11:02:56Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd

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


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

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

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 · 2018-03-26T20:20:27Z

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'

sroze · 2018-03-26T20:25:43Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

+
+        if ($config['doctrine_transaction']['enabled']) {
+            if (!class_exists(DoctrineTransactionMiddleware::class)) {
+                throw new LogicException('You must install symfony/doctrine-bridge to use the "DoctrineTransactionMiddleware"');


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".

sroze · 2018-03-26T20:29:43Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd

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


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>

sroze · 2018-03-26T20:30:16Z

src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/xml/messenger.xml

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


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

Nyholm · 2018-03-27T07:47:59Z

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

ro0NL · 2018-03-27T07:53:06Z

...Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/yml/messenger_doctrine.yml

+framework:
+    messenger:
+        doctrine_transaction:
+            entity_manager_name: 'foobar'


missing eol :}

chalasr · 2018-03-27T09:05:52Z

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 · 2018-03-27T09:21:32Z

Good idea. I've updated the PR

Nyholm · 2018-03-27T09:22:51Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

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


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

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

Nyholm · 2018-03-27T09:38:21Z

Example Yaml:

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

sroze · 2018-03-27T10:03:04Z

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 · 2018-03-27T10:44:49Z

Thank you @Nyholm.

Nyholm · 2018-03-27T10:51:00Z

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.

fabpot · 2018-03-27T11:51:37Z

src/Symfony/Bridge/Doctrine/Messenger/DoctrineTransactionMiddleware.php

+        $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));


missing dot at the end of the exception message.

fabpot · 2018-03-27T11:53:11Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

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


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

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

@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 🤔

#26684 is in.

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

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 · 2018-03-27T11:54:08Z

/cc @sroze

sroze · 2018-03-27T12:41:44Z

@fabpot OK, PR on the way.

@fabpot

…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

carsonbot added Status: Needs Review Feature labels Mar 23, 2018

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

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

sroze suggested changes Mar 23, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Mar 23, 2018

stloyd reviewed Mar 23, 2018

View reviewed changes

chalasr previously requested changes Mar 23, 2018

View reviewed changes

chalasr added the Messenger label Mar 23, 2018

yceruto reviewed Mar 23, 2018

View reviewed changes

ro0NL reviewed Mar 24, 2018

View reviewed changes

stof reviewed Mar 26, 2018

View reviewed changes

sroze suggested changes Mar 26, 2018

View reviewed changes

Nyholm added 9 commits March 27, 2018 09:47

Add a middleware that wraps all handlers in one Doctrine transaction.

e62a0c9

Updated extension

78f021d

cs

50f2cb8

Fixed tests

11a3aab

Handle a return value

373a2d4

minor

4f54988

Updates according to feedback

1eb6f4d

Do not use an anonymous function

8b87f2b

Added tests

a32e6b8

Nyholm force-pushed the middleware-doctrine branch from d653fb6 to dcb07ca Compare March 27, 2018 07:47

ro0NL reviewed Mar 27, 2018

View reviewed changes

Fixed EOL

1603708

Nyholm mentioned this pull request Mar 27, 2018

[Messenger] Added a middleware that validates messages #26648

Merged

Added config key "middlewares"

399f7c5

Nyholm commented Mar 27, 2018

View reviewed changes

Nyholm added 2 commits March 27, 2018 11:23

cs

5a6b8de

Use addDefaultsIfNotSet()

68131f4

carsonbot added Status: Reviewed and removed Status: Needs Work labels Mar 27, 2018

chalasr approved these changes Mar 27, 2018

View reviewed changes

sroze approved these changes Mar 27, 2018

View reviewed changes

sroze closed this in ea6d861 Mar 27, 2018

Nyholm deleted the middleware-doctrine branch March 27, 2018 10:50

fabpot reviewed Mar 27, 2018

View reviewed changes

sroze mentioned this pull request Mar 27, 2018

[Messenger] Remove the Doctrine middleware configuration from the FrameworkBundle #26684

Merged

javiereguiluz mentioned this pull request Apr 20, 2018

[Messenger] Add a middleware that wraps all handlers in one Doctrine transaction symfony/symfony-docs#9651

Closed

fabpot mentioned this pull request May 7, 2018

Release v4.1.0-BETA1 #27181

Merged

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

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

Conversation

Nyholm commented Mar 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Mar 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Mar 23, 2018

chalasr commented Mar 23, 2018 • edited Loading

Nyholm commented Mar 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sroze commented Mar 26, 2018

Nyholm commented Mar 26, 2018

Choose a reason for hiding this comment

Nyholm Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Mar 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Mar 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr commented Mar 27, 2018

Nyholm commented Mar 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Mar 27, 2018

sroze commented Mar 27, 2018

sroze commented Mar 27, 2018

Nyholm commented Mar 27, 2018

fabpot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sroze Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Mar 27, 2018

sroze commented Mar 27, 2018

chalasr commented Mar 23, 2018 •

edited

Loading

Nyholm Mar 26, 2018 •

edited

Loading

sroze Mar 27, 2018 •

edited

Loading