Skip to content
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

[BridgeDoctrineMessenger] Doctrine ping connection middleware #31061

Merged

Conversation

insidestyles
Copy link
Contributor

@insidestyles insidestyles commented Apr 10, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #...
License MIT
Doc PR symfony/symfony-docs#...
  • Check and reconnect if mysql has gone away:

      <service id="messenger.middleware.doctrine_ping_connection" class="Symfony\Bridge\Doctrine\Messenger\DoctrinePingConnectionMiddleware" public="false">
          <argument type="service" id="doctrine" />
      </service>
    
  • Close and save opened connections (not active worker):

      <service id="messenger.middleware.doctrine_close_connection" class="Symfony\Bridge\Doctrine\Messenger\DoctrineCloseConnectionMiddleware" public="false">
          <argument type="service" id="doctrine" />
      </service>
    

@insidestyles insidestyles force-pushed the feature/doctrine-ping-connection-middleware branch from 452dba4 to 11c4a4b Compare April 11, 2019 05:27
@nicolas-grekas nicolas-grekas added this to the next milestone Apr 11, 2019
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

CS-related comments only :)

@insidestyles insidestyles force-pushed the feature/doctrine-ping-connection-middleware branch 2 times, most recently from 58e0480 to 5fa2957 Compare April 11, 2019 12:56
Copy link
Contributor

@sroze sroze left a comment

Choose a reason for hiding this comment

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

It's a good idea. One simple comment.

Also, can you open a WIP pull-request on the DoctrineBundle as well so the service messenger.middleware.doctrine_ping_connection would be available by default? Maybe we should provide two services with the different values of forceCloseConnection?

{
private $managerRegistry;
private $entityManagerName;
private $forceCloseConnection = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need this default value as it's given by the constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you said, I think that creating 2 separated middlewares is better. Is it ok?

@sroze
Copy link
Contributor

sroze commented Apr 22, 2019

As long as it’s clear in your documentation PR I think it’s okay 👍

@sroze
Copy link
Contributor

sroze commented Apr 22, 2019

@insidestyles what I meant is only one class here but two service definitions within the DoctrineBundle where you need to register the services (in the messenger.xml).

@insidestyles
Copy link
Contributor Author

insidestyles commented Apr 22, 2019

@insidestyles what I meant is only one class here but two service definitions within the DoctrineBundle where you need to register the services (in the messenger.xml).

I understood. Should I revert the last commit?

I also created doctrine/DoctrineBundle#956

@alcaeus
Copy link
Contributor

alcaeus commented Apr 23, 2019

@sroze what Symfony version are you targeting with this? Is this still eligible for 4.3 or will this land in 4.4 by default?

@fabpot
Copy link
Member

fabpot commented Apr 24, 2019

I will stop merging new features for 4.3 probably at the end of the week. But I would really like this one to be part of 4.3 :) We need some approvals.

Copy link
Contributor

@sroze sroze left a comment

Choose a reason for hiding this comment

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

Can you add a note in the CHANGELOG as well as the @experimental in 4.3 annotation on the two classes, please?

@insidestyles
Copy link
Contributor Author

Can you add a note in the CHANGELOG as well as the @experimental in 4.3 annotation on the two classes, please?

@sroze Done.

@sroze sroze force-pushed the feature/doctrine-ping-connection-middleware branch from 9dd9276 to 6fd9f6a Compare April 28, 2019 15:15
@sroze
Copy link
Contributor

sroze commented Apr 28, 2019

Thank you @insidestyles.

@sroze sroze merged commit 6fd9f6a into symfony:master Apr 28, 2019
sroze added a commit that referenced this pull request Apr 28, 2019
…dleware (insidestyles)

This PR was squashed before being merged into the 4.3-dev branch (closes #31061).

Discussion
----------

[BridgeDoctrineMessenger] Doctrine ping connection middleware

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

- Check and reconnect if mysql has gone away:

        <service id="messenger.middleware.doctrine_ping_connection" class="Symfony\Bridge\Doctrine\Messenger\DoctrinePingConnectionMiddleware" public="false">
            <argument type="service" id="doctrine" />
        </service>

- Close and save opened connections (not active worker):

        <service id="messenger.middleware.doctrine_close_connection" class="Symfony\Bridge\Doctrine\Messenger\DoctrineCloseConnectionMiddleware" public="false">
            <argument type="service" id="doctrine" />
        </service>

Commits
-------

6fd9f6a [BridgeDoctrineMessenger] Doctrine ping connection middleware
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
@fabpot fabpot mentioned this pull request May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants