8000 [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
8000
Prev Previous commit
Use addDefaultsIfNotSet()
  • Loading branch information
@Nyholm
Nyholm committed Mar 27, 2018
commit 68131f43bc806c93cca6617d78d6f311539cc5d6
11 changes: 1 addition & 10 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,16 +966,6 @@ 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 empty($config['middlewares']);
})
->then(function ($config) {
$config['middlewares'] = array();

return $config;
})
->end()
->children()
->arrayNode('routing')
->useAttributeAsKey('message_class')
Expand Down Expand Up @@ -1009,6 +999,7 @@ function ($a) {
->end()
->end()
->arrayNode('middlewares')
->addDefaultsIfNotSet()
->children()
->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 th 8000 is 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 8000 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()
Expand Down
8 changes: 5 additions & 3 deletions src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,9 +253,11 @@ class_exists(SemaphoreStore::class) && SemaphoreStore::isSupported() ? 'semaphor
'messenger' => array(
'enabled' => !class_exists(FullStack::class) && class_exists(MessageBusInterface::class),
'routing' => array(),
'doctrine_transaction' => array(
'enabled' => false,
'entity_manager_name' => null,
'middlewares' => array(
'doctrine_transaction' => array(
'enabled' => false,
'entity_manager_name' => null,
),
),
),
);
Expand Down
0