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

[Mailer] Extract transport factory and allow create custom transports#31946

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

Merged
fabpot merged 1 commit intosymfony:4.4fromKoc:mailer-transport-factory
Jul 17, 2019

Conversation

@Koc
Copy link
Contributor

@KocKoc commentedJun 7, 2019•
edited
Loading

QA
Branch?4.4
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes, failure unrelated (master hasn't this PR)
Fixed tickets#31385,#32523
LicenseMIT
Doc PRTBD

Alternative approach to allow create custom transports and register DSN for it. Replaces#31931,#31935 . Similar to already existent TansportFactory from Messenger.

TODO:

  • Update changelog
  • Add more tests for factories
  • Add test for configuration + DI extension

onEXHovia, Simperfit, and francoispluchino reacted with thumbs up emojiKocal reacted with heart emoji
fabpot added a commit that referenced this pull requestJun 11, 2019
…rom Component to Contracts (Koc)This PR was merged into the 4.4 branch.Discussion----------[Mailer] Changed EventDispatcherInterface dependency from Component to Contracts| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | no| New feature?  | no| BC breaks?    | yes| Deprecations? | no| Tests pass?   | almost yes, see#31956 (comment)| Fixed tickets | -| License       | MIT| Doc PR        | -Follow up of#31946 (comment) . I hope this kind of changes are allowed for experimental components.BTW,@nicolas-grekas , why Psr14 interface is optional for Contract's `EventDispatcherInterfacehttps://github.com/symfony/symfony/blob/4.4/src/Symfony/Contracts/EventDispatcher/EventDispatcherInterface.php#L16 ?Commits-------bdb6217 Changed EventDispatcherInterface dependency from Component to Contracts
@KocKocforce-pushed themailer-transport-factory branch 3 times, most recently from816f403 to9af7d94CompareJune 11, 2019 20:23
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.

Looks promising. Can you finish the PR? We should probably remove the current way or at least deprecate it.

@Koc
Copy link
ContributorAuthor

Koc commentedJun 30, 2019

@fabpot I've fixed comments from code review. Now working on adding tests (I've updated TODO list in PR header)

@KocKoc marked this pull request as ready for reviewJune 30, 2019 19:04
@KocKocforce-pushed themailer-transport-factory branch from593acbc to02aa069CompareJune 30, 2019 19:06
Copy link
Contributor

@TaluuTaluu left a comment

Choose a reason for hiding this comment

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

Awesome. 👍

@Koc
Copy link
ContributorAuthor

Koc commentedJul 7, 2019

I've fixed tests and removed BC-breaks, so bc-break label non-actual anymore. More tests for all factories should be added, but I need 1 week more due to vacation.

BTW PR is ready for another round of review.

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.

Looks good to me. Thanks for the hard work so far!

@KocKocforce-pushed themailer-transport-factory branch fromaea9b37 toa016fc2CompareJuly 14, 2019 08:00
@KocKocforce-pushed themailer-transport-factory branch 2 times, most recently from29d9a30 to5029ed6CompareJuly 14, 2019 15:01
@KocKoc mentioned this pull requestJul 15, 2019
@fabpot
Copy link
Member

Not sure if this PR is ready now but fabbot has an interesting patch to apply.

@Koc
Copy link
ContributorAuthor

Koc commentedJul 15, 2019

Almost ready. Failure caused because#32553 not merged yet.

@fabpot
Copy link
Member

@Koc#32553 has been merged now

@KocKocforce-pushed themailer-transport-factory branch from1d701e1 to5b9cdedCompareJuly 16, 2019 19:16
@Koc
Copy link
ContributorAuthor

Koc commentedJul 16, 2019

Finally PR is ready 🎉

Travis is green exceptdeps=high (because master hasn't this PR yet). To be honest I still haven't performed manual testing of the branch, but we already have test for DI extension and each factory properly tested.

@fabpot
Copy link
Member

Thank you@Koc.

@fabpotfabpot merged commit5b9cded intosymfony:4.4Jul 17, 2019
fabpot added a commit that referenced this pull requestJul 17, 2019
…stom transports (Koc)This PR was merged into the 4.4 branch.Discussion----------[Mailer] Extract transport factory and allow create custom transports| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes, failure unrelated (master hasn't this PR)| Fixed tickets |#31385,#32523| License       | MIT| Doc PR        | TBDAlternative approach to allow create custom transports and register DSN for it. Replaces#31931,#31935 . Similar to already existent TansportFactory from Messenger.TODO: - [x] Update changelog - [x] Add more tests for factories - [x] Add test for configuration + DI extensionCommits-------5b9cded Add transport factories (closes#31385,closes#32523)
}

$user =urldecode($parsedDsn['user'] ??null);
$password =urldecode($parsedDsn['pass'] ??null);
Copy link
Member

Choose a reason for hiding this comment

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

this is broken. It is passingnull tourldecode, and it won't getnull as$user then.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

But it already covered with testhttps://github.com/symfony/symfony/pull/31946/files#diff-e3a1e78245b60fed36ed6ab4e56023ceR48 . Strange. Maybe becauseassertEquals compares'' equal tonull

Copy link
Contributor

Choose a reason for hiding this comment

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

@KocassertEquals() does not make a strict comparison unlikeassertSame(). It is therefore recommended to useassertSame() (orassertNull() to check the null value).

@KocKoc deleted the mailer-transport-factory branchJuly 22, 2019 12:10
fabpot added a commit that referenced this pull requestJul 25, 2019
…es (Koc)This PR was squashed before being merged into the 4.4 branch (closes#32609).Discussion----------[Mailer][DX][RFC] Rename mailer bridge transport classes| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | yno| New feature?  | no| BC breaks?    | yes| Deprecations? | no| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -During working on#31946 I realized how painful to work with multiple classes which has same name. [Nice article](https://www.tomasvotruba.cz/blog/2019/05/02/alias-as-a-code-smell/) by@TomasVotruba with explanation of problems  with such approach.~Built on top of#32608 , so only [2nd commit](bbf7e99) is actual.~Also I've changed namespaces to make bridge structure much simpler and be linear. All classes located on same level now. See how [bridge](https://github.com/symfony/symfony/tree/bbf7e99e89c70fab372929827ae509b41280ce40/src/Symfony/Component/Mailer/Bridge/Amazon) looks like now.Now in RFC state to get approve for such king of changes and update all other bridges.Commits-------eda4f01 [Mailer][DX][RFC] Rename mailer bridge transport classes
@nicolas-grekasnicolas-grekas modified the milestones:next,4.4Oct 27, 2019
This was referencedNov 12, 2019
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull requestJan 18, 2024
This PR was submitted for the 4.4 branch but it was squashed and merged into the 5.4 branch instead.Discussion----------[Mailer] Add custom transport factoriesThis might be useful for very custom use cases where we need to extract some data or do something before being able to create the actual transport.The feature apparently was added here:symfony/symfony#31946Commits-------72b69c1 [Mailer] Add custom transport factories
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@stofstofstof left review comments

+5 more reviewers

@ajgarlagajgarlagajgarlag left review comments

@francoispluchinofrancoispluchinofrancoispluchino left review comments

@kevin-verschaevekevin-verschaevekevin-verschaeve left review comments

@onEXHoviaonEXHoviaonEXHovia left review comments

@TaluuTaluuTaluu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

10 participants

@Koc@fabpot@Taluu@nicolas-grekas@ajgarlag@stof@francoispluchino@kevin-verschaeve@onEXHovia@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp