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

Allow to specify CC and BCC in sender #152

Merged
merged 4 commits into from
Jun 28, 2022

Conversation

Zales0123
Copy link
Member

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Related tickets replaces #11
License MIT

This PR is based on #11, fixed, rebased and improved 🖖 My biggest concern is a Sender - I wanted to avoid adding another interface (as I did with AdapterInterface), so I made some shenanigans with arguments counting... but I'm not convinced it's the best choice 🤷‍♂️ Still, BC breaks-free for sure.

It would be great to merge it before 1.8 release 🚀

@Zales0123 Zales0123 added the Feature New feature proposals. label Jun 21, 2022
@Zales0123 Zales0123 requested a review from a team as a code owner June 21, 2022 13:30
@Zales0123 Zales0123 force-pushed the allow-to-specify-cc-and-bcc-in-sender branch 2 times, most recently from 5f33e3f to acec554 Compare June 22, 2022 13:56
src/Bundle/Sender/Adapter/SymfonyMailerAdapter.php Outdated Show resolved Hide resolved
@@ -59,6 +67,23 @@ public function send(string $code, array $recipients, array $data = [], array $a

$renderedEmail = $this->rendererAdapter->render($email, $data);

if (count($arguments) > 5 && $this->senderAdapter instanceof CcAwareAdapterInterface) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if we shouldn't cover the case, where someone is giving more than 5 arguments, but the sender is not CcAwareAdapterInterface

Copy link
Member Author

Choose a reason for hiding this comment

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

If someone is giving more than 5 arguments and it's not CCAware... then it's fulfilling a different contract and should be customized nevertheless... I think so :D

src/Component/Sender/Sender.php Show resolved Hide resolved
@Zales0123 Zales0123 force-pushed the allow-to-specify-cc-and-bcc-in-sender branch from acec554 to 8b9adbd Compare June 23, 2022 16:16
@GSadee GSadee merged commit 8c32f65 into Sylius:master Jun 28, 2022
@GSadee
Copy link
Member

GSadee commented Jun 28, 2022

Thank you, Mateusz! 🎉

@Zales0123 Zales0123 deleted the allow-to-specify-cc-and-bcc-in-sender branch June 28, 2022 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature proposals.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants