Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
Nyholm wants to merge24 commits intosymfony:masterfromNyholm:middleware-doctrine

Conversation

@Nyholm
Copy link
Member

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsna
LicenseMIT
Doc PRcoming up

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

linaori, Shine-neko, andreybolonin, yceruto, ro0NL, and theofidry reacted with thumbs up emoji
@NyholmNyholm changed the titleAdd 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-grekasnicolas-grekas added this to the4.1 milestoneMar 23, 2018
->end()
->end()
->end()
->arrayNode('doctrine')
Copy link
Contributor

Choose a reason for hiding this comment

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

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()
Copy link
Contributor

Choose a reason for hiding this comment

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

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

class WrapsMessageHandlingInTransactionimplements MiddlewareInterface
{
/**
* @var ManagerRegistry
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be removed.

private$managerRegistry;

/**
* @var string
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be removed as well.

private$entityManagerName;

/**
* @param ManagerRegistry $managerRegistry
Copy link
Contributor

Choose a reason for hiding this comment

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

All of these as well :)


publicfunctionhandle($message,callable$next)
{
/** @var $entityManager EntityManagerInterface */
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest to check withinstanceof the type, rather than assuming it is always going to beEntityManagerInterface. If not, we can simply throw anInvalidArgumentException.


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

Choose a reason for hiding this comment

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

It looks like to me that using thebeginTransaction,commit androllback methods would be much more explicit that this callback-based transaction that forces you to do this weirdresetManager later :)

@Nyholm
Copy link
MemberAuthor

Thank you for the review

$container->getDefinition('messenger.sender_locator')->replaceArgument(0,$senderLocatorMapping);
$container->getDefinition('messenger.asynchronous.routing.sender_locator')->replaceArgument(1,$messageToSenderIdsMapping);

if ($config['doctrine']['enabled']) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Array keys mismatch after recent rename.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Thanks!

chalasr
chalasr previously requested changesMar 23, 2018
$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']);
Copy link
Member

Choose a reason for hiding this comment

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

$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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Thank you

@Nyholm
Copy link
MemberAuthor

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

@chalasr
Copy link
Member

chalasr commentedMar 23, 2018•
edited
Loading

@Nyholm yes

@Nyholm
Copy link
MemberAuthor

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 (!$entityManagerinstanceof EntityManagerInterface) {
thrownew \InvalidArgumentException(sprintf('The ObjectManager with name "%s" must be an instance of EntityManagerInterface',$this->entityManagerName));
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Hm.. But the doc block says ObjectManager..

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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()
Copy link
Member

Choose a reason for hiding this comment

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

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

sroze and ogizanagi reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Excellent idea. Thanks

*
* @author Tobias Nyholm <tobias.nyholm@gmail.com>
*/
class WrapsMessageHandlingInTransactionimplements MiddlewareInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

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

sroze reacted with thumbs up emoji
Copy link
Contributor

Choose a reason for hiding this comment

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

What aboutDoctrineTransactionMiddleware ?

ro0NL reacted with thumbs up emoji
@sroze
Copy link
Contributor

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

@Nyholm
Copy link
MemberAuthor

Done


<xsd:complexTypename="messenger_doctrine_transaction">
<xsd:sequence>
<xsd:elementname="entity-manager-name"type="xsd:string" />
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

@NyholmNyholmMar 26, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Fixed!

Copy link
Contributor

Choose a reason for hiding this comment

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

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

    <framework:config>        <framework:messenger>            <framework:doctrine-transactionentity-manager-name="foobar" />        </framework:messenger>    </framework:config>

ogizanagi reacted with thumbs up emoji
@Nyholm
Copy link
MemberAuthor

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:routingmessage-class="App\Bar">                <framework:senderservice="sender.bar"></framework:sender>                <framework:senderservice="sender.biz"></framework:sender>            </framework:routing>            <framework:routingmessage-class="App\Foo">                <framework:senderservice="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 reacted with thumbs up emoji


if ($config['doctrine_transaction']['enabled']) {
if (!class_exists(DoctrineTransactionMiddleware::class)) {
thrownewLogicException('You must install symfony/doctrine-bridge to use the "DoctrineTransactionMiddleware"');
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Nyholm reacted with thumbs up emoji

<xsd:complexTypename="messenger_doctrine_transaction">
<xsd:sequence>
<xsd:elementname="entity-manager-name"type="xsd:string" />
Copy link
Contributor

Choose a reason for hiding this comment

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

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

    <framework:config>        <framework:messenger>            <framework:doctrine-transactionentity-manager-name="foobar" />        </framework:messenger>    </framework:config>

ogizanagi reacted with thumbs up emoji
<framework:config>
<framework:messenger>
<framework:routingmessage-class="App\Bar">
<framework:senderservice="sender.bar"></framework:sender>
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@Nyholm
Copy link
MemberAuthor

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

framework:
messenger:
doctrine_transaction:
entity_manager_name:'foobar' No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

missing eol :}

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Thank you

@chalasr
Copy link
Member

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

@Nyholm
Copy link
MemberAuthor

Good idea. I've updated the PR

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

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
MemberAuthor

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()

@Nyholm
Copy link
MemberAuthor

Example Yaml:

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

@sroze
Copy link
Contributor

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
Copy link
Contributor

Thank you@Nyholm.

@NyholmNyholm deleted the middleware-doctrine branchMarch 27, 2018 10:50
@Nyholm
Copy link
MemberAuthor

Thank you for merging

Copy link
Member

@fabpotfabpot left a comment

Choose a reason for hiding this comment

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

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 (!$entityManagerinstanceof EntityManagerInterface) {
thrownew \InvalidArgumentException(sprintf('The ObjectManager with name "%s" must be an instance of EntityManagerInterface',$this->entityManagerName));
Copy link
Member

Choose a reason for hiding this comment

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

missing dot at the end of the exception message.

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

sroze reacted with thumbs up emoji
Copy link
Contributor

@srozesrozeMar 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 inFrameworkBundle. 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

sroze, ogizanagi, Nyholm, and yceruto reacted with thumbs up emoji
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?

@fabpot
Copy link
Member

/cc@sroze

@sroze
Copy link
Contributor

@fabpot OK, PR on the way.

fabpot added a commit that referenced this pull requestApr 4, 2018
…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@NyholmCommits-------27a8b1d Remove the Doctrine middleware configuration from the FrameworkBundle
@fabpotfabpot mentioned this pull requestMay 7, 2018
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot left review comments

@stofstofstof left review comments

@ycerutoycerutoyceruto left review comments

@chalasrchalasrchalasr approved these changes

+4 more reviewers

@stloydstloydstloyd left review comments

@srozesrozesroze approved these changes

@ro0NLro0NLro0NL left review comments

@ogizanagiogizanagiogizanagi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.1

Development

Successfully merging this pull request may close these issues.

11 participants

@Nyholm@chalasr@sroze@fabpot@stloyd@stof@ro0NL@yceruto@ogizanagi@nicolas-grekas@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp