10000 [Messenger] Add a middleware that wraps all handlers in one Doctrine transaction. by Nyholm · Pull Request #26647 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 24 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
e62a0c9
Add a middleware that wraps all handlers in one Doctrine transaction.
Nyholm Mar 23, 2018
78f021d
Updated extension
Nyholm Mar 23, 2018
50f2cb8
cs
Nyholm Mar 23, 2018
11a3aab
Fixed tests
Nyholm Mar 23, 2018
373a2d4
Handle a return value
Nyholm Mar 23, 2018
4f54988
minor
Nyholm Mar 23, 2018
1eb6f4d
Updates according to feedback
Nyholm Mar 23, 2018
8b87f2b
Do not use an anonymous function
Nyholm Mar 23, 2018
a32e6b8
Added tests
Nyholm Mar 23, 2018
0261a51
Adding configuration definition
Nyholm Mar 23, 2018
f362f50
cs
Nyholm Mar 23, 2018
a629620
cs
Nyholm Mar 23, 2018
05ac7cf
Allow null
Nyholm Mar 26, 2018
0e0ab07
Moved to DoctrineBridge
Nyholm Mar 26, 2018
af65748
Added extra check
Nyholm Mar 26, 2018
701c665
Updating configuration
Nyholm Mar 26, 2018
b336d04
Fixed tests
Nyholm Mar 26, 2018
7d84394
cs
Nyholm Mar 26, 2018
09e5daf
cs and minors
Nyholm Mar 27, 2018
dcb07ca
Fixed tests
Nyholm Mar 27, 2018
1603708
Fixed EOL
Nyholm Mar 27, 2018
399f7c5
Added config key "middlewares"
Nyholm Mar 27, 2018
5a6b8de
cs
Nyholm Mar 27, 2018
68131f4
Use `addDefaultsIfNotSet()`
Nyholm Mar 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Added config key "middlewares"
  • Loading branch information
@Nyholm
Nyholm committed Mar 27, 2018
commit 399f7c5f37c0dad4d65d8e0b3a20072fcb0e4ab2
18 changes: 15 additions & 3 deletions src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -966,6 +966,15 @@ private function addMessengerSection(ArrayNodeDefinition $rootNode)
->arrayNode('messenger')
->info('Messenger configuration')
->{!class_exists(FullStack::class) && class_exists(MessageBusInterface::class) ? 'canBeDisabled' : 'canBeEnabled'}()
->beforeNormalization()
->ifTrue(function($config) {
return 8000 empty($config['middlewares']);
})
->then(function($config) {
$config['middlewares'] = array();
return $config;
})
->end()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

->children()
->arrayNode('routing')
->useAttributeAsKey('message_class')
Expand Down Expand Up @@ -998,10 +1007,13 @@ function ($a) {
->end()
->end( 8000 )
->end()
->arrayNode('doctrine_transaction')
->canBeEnabled()
->arrayNode('middlewares')
->children()
->scalarNode('entity_manager_name')->info('The name of the entity manager to use')->defaultNull()->end()
->arrayNode('doctrine_transaction')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor
@sroze sroze Mar 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#26684 is in.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

->canBeEnabled()
->children()
->scalarNode('entity_manager_name')->info('The name of the entity manager to use')->defaultNull()->end()
->end()
->end()
->end()
->end()
Expand Down
4 changes: 2 additions & 2 deletions src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1447,11 +1447,11 @@ 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_transaction']['enabled']) {
if ($config['middlewares']['doctrine_transaction']['enabled']) {
if (!class_exists(DoctrineTransactionMiddleware::class)) {
throw new LogicException('The Doctrine transaction middleware is only available when the doctrine bridge is installed. Try running "composer require symfony/doctrine-bridge".');
}
$container->getDefinition('messenger.middleware.doctrine_transaction')->replaceArgument(1, $config['doctrine_transaction']['entity_manager_name']);
$container->getDefinition('messenger.middleware.doctrine_transaction')->replaceArgument(1, $config['middlewares']['doctrine_transaction']['entity_manager_name']);
} else {
$container->removeDefinition('messenger.middleware.doctrine_transaction');
}
Expand Down
8 changes: 7 additions & 1 deletion src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,7 @@
<xsd:complexType name="messenger">
<xsd:sequence>
<xsd:element name="routing" type="messenger_routing" minOccurs="0" maxOccurs="unbounded" />
<xsd:element name="doctrine-transaction" type="messenger_doctrine_transaction" minOccurs="0" maxOccurs="1" />
<xsd:element name="middlewares" type="messenger_middleware" minOccurs="0" maxOccurs="unbounded" />
</xsd:sequence>
</xsd:complexType>

Expand All @@ -362,6 +362,12 @@
<xsd:attribute name="service" type="xsd:string" use="required"/>
</xsd:complexType>

<xsd:complexType name="messenger_middleware">
<xsd:sequence>
<xsd:element name="doctrine-transaction" type="messenger_doctrine_transaction" minOccurs="0" maxOccurs="1" />
</xsd:sequence>
</xsd:complexType>

<xsd:complexType name="messenger_doctrine_transaction">
<xsd:attribute name="entity-manager-name" type="xsd:string" />
<xsd:attribute name="enabled" type="xsd:boolean" />
Expand Down
6 changes: 4 additions & 2 deletions ...fony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/php/messenger_doctrine.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@

$container->loadFromExtension('framework', array(
'messenger' => array(
'doctrine_transaction' => array(
'entity_manager_name' => 'foobar',
'middlewares' => array(
'doctrine_transaction' => array(
'entity_manager_name' => 'foobar',
),
),
),
));
4 changes: 3 additions & 1 deletion ...fony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/xml/messenger_doctrine.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@

<framework:config>
<framework:messenger>
<framework:doctrine-transaction entity-manager-name="foobar" />
<framework:middlewares>
<framework:doctrine-transaction entity-manager-name="foobar" />
</framework:middlewares>
</framework:messenger>
</framework:config>
</container>
5 changes: 3 additions & 2 deletions ...fony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/yml/messenger_doctrine.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
framework:
messenger:
doctrine_transaction:
entity_manager_name: 'foobar'
middlewares:
doctrine_transaction:
entity_manager_name: 'foobar'
0