fix: Optional Personalization arguments handling

[kampalex]

Fixes

Fixes #961.

Methods addTo, addCc, addBcc, addTos, addCcs, addBccs, setSubject, addHeader, addSubstitution, addCustomArg, setSendAt offers support to link/reuse Personalization instances using optional $personalization and $personalizationIndex arguments.
Handling of linking/storing/fetching using these arguments have been rewritten to be more predictable.

Breaking change may be:

• Non-existent $personalizationIndex is allowed for addTo(s), usage in other methods require existence of $personalizationIndex. According to the SendGrid API docs, at least To must exists in a Personalization, so it makes sense to allow this scenario. (This must/may be added in the notes/docs.)

Can be improved further:

• Merging Personalization (if using existing $personalizationIndex with different $personalization instance)

Other fixes while working on this fix:

• addRecipientEmails: Added missing null argument for substitutions in forwarding $emailFunctionCall()
• Implemented maximum number of 1000 Personalization instances (according to the API docs)
• Added TypeException throw type hints for methods which rely on addRecipientEmail(s) - thrown by EmailAddress if email address is invalid.

Feedback is highly appreciated!
(I have to look for possible side effects to be sure it doesn't break anything)

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 master branch
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation about the functionality in the appropriate .md file
• I have added inline documentation to the code I modified

[childish-sambino]

I understand this is trying to make dealing with personalizations more intuitive (a primary part of what this library is supposed to do), but if we're going to make a breaking change (MV roll), I'd prefer it be really breaking and try to get it right.

There are parts of the old code which I simply think should not be supported, specifically passing in a personalization while setting some properties on it and adding to the array of personalizations but not even returning that thing that was added. I'd rather drop this ability all together.

Working from the end user backwards, how would they expect to use this helper functionality? Personally, I'd expect to be able to build out the personalizations in 1 of 3 ways:

1. I'll create the personalization, populate it, then add it to the mail object.
2. I'll ask the mail object to create and add a new (blank) personalization object, return it to me, and I'll populate it how I want.
3. I'm only gonna be dealing with a single personalization (I don't even care about the notion of a personalization, I just want to send mail) so let me tell the mail object what should be in the email.

No dealing with personalization indices or hybrid add-and-update functions needed/wanted. To me this is simple and intuitive, but breaking.

That said, if the larger feeling is "don't break things and make us change our code" then I think tweaking the existing implementation is okay as suggested here, to keep the logic simple. No merging personalizations and whatnot. More code and logic is more cost.

[kampalex]

Thanks for your extensive review and opinion.

I've created my PR in mind to have the least possible chance of having a breaking change, but I can't rule out because of the variation of supported scenarios and code usage in the wild.

Working from the end user backwards, how would they expect to use this helper functionality?

Well... user 1 creates a Personalization instance, completes it and appends it by addPersonalization() (preferred!), while user 2 uses a $personalizationIndex onto addTo() as 'different Personalization marker', just like the way it used in __construct(). That doesn't work always, and this causes confusion because the user thinks the library doesn't work consistent.

In the use cases docs, no sample code is present to use Personalization (comment note != code). Only an example file which exhibit the purpose insufficient (helloEmail) or too complicated (kitchenSink). I have seen a lot of complicated code in filed issues...

We may first start to create documentation how to code Personalizations right and declaring unwanted features deprecated, before forcing a breaking change. In addition to this, it might me helpful to add every code sample in an unit test.

There are parts of the old code which I simply think should not be supported, specifically passing in a personalization while setting some properties on it and adding to the array of personalizations but not even returning that thing that was added. I'd rather drop this ability all together.

I agree that providing a Personalization as argument is unwanted - decorations must be applied onto that instance itself and it should be added by addPersonalization() method. I'm about to create a PR which forwards Personalization operations of Mail to Personalization, but a few unit tests must be made first.

(Note: Maybe a check has to be implemented which prevents the addition of possible duplicated Personalization instances.)

3. I'm only gonna be dealing with a single personalization

Please don't consider this as real option. I'm using this library for transactional mail and bulk mailings. The Personalization instance offers confidence to compose and send email separately.
Avoiding leakage/privacy issues is priority 1 nowadays.

No dealing with personalization indices or hybrid add-and-update functions needed/wanted. To me this is simple and intuitive, but breaking.

Perfect world scenario. But as you see, it's in use and useful as 'different/specific Personalization marker'. The $personalizationIndex suggest reuse. Why not offering this to give users control to fetch a specific one?

...I think tweaking the existing implementation is okay as suggested here, to keep the logic simple

I'll customize this PR and will remove parts which aren't necessary anymore. (It will conflict upcoming PR which I mentioned above, so I have to update it again if merged.)

[kampalex]

I've applied the suggested solution including unit tests to cover multiple scenarios. getPersonalization() is rewritten in order to minimize duplicate code and make decisions more clearly. As far as I known it doesn't have regressions compare to current implementation.
Feel free to change/remove comments if you want.

Summary of fixes:

• Allow next predicted personalizationIndex depending on count of added Personalizations
• addRecipientEmails(): Added missing null argument for substitutions in forwarding $emailFunctionCall()
• Added TypeException throw type hints for methods which rely on addRecipientEmail(s) - thrown by EmailAddress if email address is invalid.

Maybe still point of attention:
If starting $personalizationIndex = 1, scenario

$objEmail = new Mail();
$objEmail->addTo('[email protected]', 'foo bar1', null, 1);
$objEmail->addTo('[email protected]', 'foo bar2', null, 2);
$objEmail->addTo('[email protected]', 'foo bar3', null, 3);

will pass, but

$objEmail = new Mail(new From('[email protected]'));
$objEmail->addTo('[email protected]', 'foo bar1', null, 1);

will throw an InvalidArgumentException.
This is because Personalization 0 isn't created due to the 'single argument' in the constructor.

[childish-sambino]

Looks good overall.

Think the ! can be dropped from the title as I don't consider this breaking.

[childish-sambino]

Yeah, that’s fine, but the array itself is publicly accessible so you could just fiddle with the contents elsewhere and set things to null or unset them.

[kampalex]

Yeah, that’s fine, but the array itself is publicly accessible so you could just fiddle with the contents elsewhere and set things to null or unset them.

If you mean the $personalization property of Mail, It's not:

    /** @var $personalization Personalization[] Messages and their metadata */
    private $personalization;

Even if you're using the Mail getPersonalizations() method to retrieve them, you can't overwrite the entry by assigning null value. The array you're receiving is not a reference to the inner property.

When earlier suggested validation is implemented, all entries of $personalization will be an instance of Personalization.

[childish-sambino]

I thought there was a public method that returned the array itself.

[childish-sambino]

Never mind last comments; forgot PHP was pass-by-value by default.

[childish-sambino]

🥇 Dropping the ! from the title as I don't consider this breaking.