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

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

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

Conversation

@insidestyles
Copy link
Contributor

@insidestylesinsidestyles commentedApr 10, 2019•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#...
LicenseMIT
Doc PRsymfony/symfony-docs#...

  • Check and reconnect if mysql has gone away:

      <service public="false">      <argument type="service" />  </service>

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

      <service public="false">      <argument type="service" />  </service>

@insidestylesinsidestylesforce-pushed thefeature/doctrine-ping-connection-middleware branch from452dba4 to11c4a4bCompareApril 11, 2019 05:27
@nicolas-grekasnicolas-grekas added this to thenext milestoneApr 11, 2019
Copy link
Member

@nicolas-grekasnicolas-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 :)

@insidestylesinsidestylesforce-pushed thefeature/doctrine-ping-connection-middleware branch 2 times, most recently from58e0480 to5fa2957CompareApril 11, 2019 12:56
Copy link
Contributor

@srozesroze 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 servicemessenger.middleware.doctrine_ping_connection would be available by default? Maybe we should provide two services with the different values offorceCloseConnection?

{
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
ContributorAuthor

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

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

@sroze
Copy link
Contributor

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

@insidestyles
Copy link
ContributorAuthor

insidestyles commentedApr 22, 2019•
edited
Loading

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

I understood. Should I revert the last commit?

I also createddoctrine/DoctrineBundle#956

@alcaeus
Copy link
Contributor

@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

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.

insidestyles reacted with heart emoji

Copy link
Contributor

@srozesroze 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
ContributorAuthor

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

@sroze Done.

@srozesrozeforce-pushed thefeature/doctrine-ping-connection-middleware branch from9dd9276 to6fd9f6aCompareApril 28, 2019 15:15
@sroze
Copy link
Contributor

Thank you@insidestyles.

@srozesroze merged commit6fd9f6a intosymfony:masterApr 28, 2019
sroze added a commit that referenced this pull requestApr 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     <!-- seehttps://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 public="false">            <argument type="service" />        </service>- Close and save opened connections (not active worker):        <service public="false">            <argument type="service" />        </service>Commits-------6fd9f6a [BridgeDoctrineMessenger] Doctrine ping connection middleware
@nicolas-grekasnicolas-grekas modified the milestones:next,4.3Apr 30, 2019
@fabpotfabpot mentioned this pull requestMay 9, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@OskarStarkOskarStarkOskarStark left review comments

@NyholmNyholmNyholm left review comments

@chalasrchalasrchalasr approved these changes

+1 more reviewer

@srozesrozesroze approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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
[8]ページ先頭
©2009-2025 Movatter.jp