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

Email modification after creation #192

Merged
merged 7 commits into from
Jan 9, 2023

Conversation

Zales0123
Copy link
Member

@Zales0123 Zales0123 commented Jan 2, 2023

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets it probably need this to be merged before for the green build
License MIT

While the default configuration-based email creation is sufficient in most cases, it's not uncommon for one to need it conditional or dynamic in any other way. I thought about modifying the EmailProvider::getEmail(string $code) method to accept the array $data argument, but thought it would be more extendible to provide a set of modifiers services, that could be easily implemented in the application. Maybe it should also contain the support(EmailInterface $email): bool method, to not force checking the email code inside the modify method 🤔

Let me know what you think about such a feature 🖖 I will add some documentation to this feature if we would like to proceed with it 🫡

@Zales0123 Zales0123 added the Feature New feature proposals. label Jan 2, 2023
@Zales0123 Zales0123 requested a review from a team as a code owner January 2, 2023 15:11
@Zales0123 Zales0123 force-pushed the email-modification-after-creation branch 2 times, most recently from 66d59e7 to 8f3cb39 Compare January 3, 2023 07:40
Copy link
Member

@GSadee GSadee left a comment

Choose a reason for hiding this comment

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

This PR should be probably opened to 2.1 branch (which should be created as well)

@GSadee
Copy link
Member

GSadee commented Jan 3, 2023

The base of this pull-request was changed, you need fetch and reset your local branch
if you want to add new commits to this pull request. Reset before you pull, else commits
may become messed-up.

Unless you added new commits (to this branch) locally that you did not push yet,
execute git fetch origin && git reset "email-modification-after-creation" to update your local branch.

Feel free to ask for assistance when you get stuck 👍

@GSadee GSadee changed the base branch from 2.0 to 2.1 January 3, 2023 12:49
@GSadee GSadee force-pushed the email-modification-after-creation branch from 898cb97 to b7b4480 Compare January 6, 2023 12:06
@Zales0123 Zales0123 force-pushed the email-modification-after-creation branch from b7b4480 to d837ca7 Compare January 9, 2023 12:39
@GSadee GSadee merged commit afeba89 into Sylius:2.1 Jan 9, 2023
@GSadee
Copy link
Member

GSadee commented Jan 9, 2023

Thank you, Mateusz! 🎉

@Zales0123 Zales0123 deleted the email-modification-after-creation branch January 9, 2023 19:47
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