Skip to content

[BridgeDoctrineMessenger] Doctrine ping connection middleware #31061

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

Jump to bottom
Merged
merged 1 commit into from
Apr 28, 2019
Merged

[BridgeDoctrineMessenger] Doctrine ping connection middleware #31061

merged 1 commit into from
Apr 28, 2019

Conversation

insidestyles
Copy link
Contributor

@insidestyles insidestyles commented Apr 10, 2019 •
edited
Loading

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 requested a review from sroze as a code owner April 10, 2019 14:57
@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
nicolas-grekas
nicolas-grekas reviewed Apr 11, 2019
View reviewed changes
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
sroze
sroze suggested changes Apr 22, 2019
View reviewed changes
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?

src/Symfony/Bridge/Doctrine/Messenger/DoctrinePingConnectionMiddleware.php Outdated
{
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 insidestyles mentioned this pull request Apr 22, 2019
Merged
@insidestyles
Copy link
Contributor Author

insidestyles commented Apr 22, 2019 •
edited
Loading

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

sroze
sroze approved these changes Apr 28, 2019
View reviewed changes
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
@sroze
feature #31061 [BridgeDoctrineMessenger] Doctrine ping connection mid…
67e1709 
…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
@javiereguiluz javiereguiluz mentioned this pull request May 7, 2019
Closed
@weaverryan weaverryan mentioned this pull request May 7, 2019
Closed
25 tasks
@fabpot fabpot mentioned this pull request May 9, 2019
Merged
@Koc Koc mentioned this pull request May 27, 2019
Closed
@weaverryan weaverryan mentioned this pull request Jul 8, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@OskarStark OskarStark OskarStark left review comments

@Nyholm Nyholm Nyholm left review comments

@sroze sroze sroze approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
DoctrineBridge Feature Messenger Status: Reviewed
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.
9 participants
@insidestyles @sroze @alcaeus @fabpot @nicolas-grekas @OskarStark @Nyholm @chalasr @carsonbot