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

fix: preserve substitutions from To in Personalization #1101

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

toleabivol
Copy link

@toleabivol toleabivol commented Sep 4, 2022

If there are substitutions in the To object preserve them by transfering them to the Personalization object.

Currently this does not work (we loose all substitutions from the To):

$personalization0 = new Personalization();
$personalization0->addTo(new To(
        "[email protected]",
        "Example User2",
        [
            '-name-' => 'Example User 2'   // <--this is lost and not sent to sendgrid API
        ],
        "Example User2 -name-"
));
$email->addPersonalization($personalization0);

We can either add the substitution during addTo method (as current PR is proposing) or parse all Tos during getSubstitutions method and add them to the substitution property. I propose the first way because it is similar to what we have in https://github.com/sendgrid/sendgrid-php/blob/main/lib/mail/Mail.php#L205

We have to also think about implementations that worked around this issue (like myself) and not break them, so the following must still be working :

$personalization0 = new Personalization();
personalization->addSubstitution(new Substitution('-name-', 'Example User 1')); // <- we want this preserved even if added first
$personalization0->addTo(new To(
        "[email protected]",
        "Example User2",
        [
            '-name-' => 'Example User 2' // <- we want this to not be applied in this case as addSubstitution takes precedence
        ],
        "Example User2 -name-"
));

$email->addPersonalization($personalization0);

OR we don't but we risk to break some implementations. Are there means of communicating the change in this case ?

Checklist

  • I acknowledge that all my contributions will be made under the project's license
  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the Contribution Guidelines and my PR follows them
  • I have titled the PR appropriately
  • I have updated my branch with the main branch
  • I have added tests that prove my fix is effective or that my feature works
  • I have added the necessary documentation about the functionality in the appropriate .md file N/A
  • I have added inline documentation to the code I modified

If there are substitutions in the To object preserve them by transfering them to the Personalization object
@toleabivol toleabivol marked this pull request as ready for review September 4, 2022 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant