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

Debug Sylius Mailer command #223

Open
wants to merge 8 commits into
base: 2.1
Choose a base branch
from

Conversation

lchrusciel
Copy link
Member

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets
License MIT

updated version of #201

I was playing with SyliusMailerBundle a little bit yesterday and wanted to get information about emails that are configured. As it is a project using Sylius with some customizations, my first thought was "so what, I'm gonna browse yaml files like some kind of animal?!". My second thought was "but I can use debug:config sylius_mailer command to dump the bundle configuration which did the trick but looked rough and ugly.

So my final resolution was - what if we have a nice debug command for the configured emails? And I wrote it. There are obviously a lot of things to improve (I still don't know if dumping the email template brings any value 🤷‍♂️), but looks nicer 🚀

Let me know what you think about it 🫡

Zrzut ekranu 2023-01-18 o 23 30 41

PS. Thanks @loic425 for the inspiration with debug serializer command - I definitely belive following the "it's useful for me, maybe it's useful for others" path is a good idea 🖖

@lchrusciel lchrusciel requested a review from a team as a code owner March 4, 2024 14:16
@lchrusciel lchrusciel force-pushed the debug-sylius-mailer-command branch 14 times, most recently from bc26212 to 05c9ae4 Compare March 5, 2024 08:41
@Zales0123 Zales0123 added the Feature New feature proposals. label Mar 5, 2024
phpunit.xml.dist Outdated Show resolved Hide resolved
Co-authored-by: Olivier ALLAIN <oallain@users.noreply.github.com>
Comment on lines +13 to +16
<directory>./src/Bundle/tests/</directory>

<exclude>./src/Bundle/tests/Integration/Cli/</exclude>
</testsuite>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<directory>./src/Bundle/tests/</directory>
<exclude>./src/Bundle/tests/Integration/Cli/</exclude>
</testsuite>
<directory>./src/Bundle/tests/</directory>
<exclude>./src/Bundle/tests/Integration/Cli/</exclude>
</testsuite>

Copy link
Member

@Zales0123 Zales0123 left a comment

Choose a reason for hiding this comment

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

👌

$dumper->dump($input, $output);
}

return 0;
Copy link
Member

Choose a reason for hiding this comment

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

return Command::SUCCESS 💃

Comment on lines +43 to +47
foreach ($this->dumpers as $dumper) {
$dumper->dump($input, $output);
}

return 0;
Copy link
Member

Choose a reason for hiding this comment

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

What would you say for extracting it to the $this->dumpAllEmails function? And $this->dumpEmailDetails below? we would have a nice 5-lines long execute function here 🚀

/*
* This file is part of the Sylius package.
*
* (c) Paweł Jędrzejewski
Copy link
Member

Choose a reason for hiding this comment

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

Should be changed and will be after merging #233 👌

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.

None yet

4 participants