Skip to content

[Messenger] Extract Dsn object #32566

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

Closed
wants to merge 2 commits into from
Closed

[Messenger] Extract Dsn object #32566

wants to merge 2 commits into from

Conversation

Koc
Copy link
Contributor

@Koc Koc commented Jul 16, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? no
BC breaks? yes
Deprecations? no
Tests pass? yes, AppVeyor failure unrelated
Fixed tickets part of #32546
License MIT
Doc PR -

During implementing transport factories for Mailer component I decided to extract Dsn object in Messenger component also instead of string + array of options.

Copy link
Member

@stof stof left a comment

Choose a reason for hiding this comment

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

Does it work properly in the DI container ?

@nicolas-grekas nicolas-grekas added this to the next milestone Jul 17, 2019
@Koc Koc force-pushed the messenger-dsn branch from d14e3db to 8a48e75 Compare July 18, 2019 22:49
@Koc
Copy link
Contributor Author

Koc commented Jul 18, 2019

Does it work properly in the DI container ?

I don't see any problems here. We can use factory method fromString with combination of factory class.

BTW, thanx for review.

@Koc Koc force-pushed the messenger-dsn branch 5 times, most recently from 2bfdeaa to c25295d Compare November 14, 2019 00:08
@Koc Koc marked this pull request as ready for review November 14, 2019 18:41
@Koc Koc requested a review from sroze as a code owner November 14, 2019 18:41
@Koc
Copy link
Contributor Author

Koc commented Nov 14, 2019

I hope that there is small change to merge this PR.

BC-breaks was documented, old tests fixed, new added. Please, review.

// cc @nicolas-grekas @fabpot @sroze

@nicolas-grekas
Copy link
Member

I'm closing now because this would be a BC break and I'm not sure this is worth it. That's an internal refacto that has a significant side effect on the public API, but has no practical side effect on the feature side.

Thanks for giving it a try.

@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
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.

6 participants