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

Update constructor to signify which parameters are required for sending all email #408

Merged
merged 5 commits into from
Jul 1, 2017

Conversation

caseyw
Copy link
Contributor

@caseyw caseyw commented Jun 28, 2017

No description provided.

@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label Jun 28, 2017
@@ -948,12 +948,18 @@ class Mail implements \JsonSerializable

public function __construct($from = null, $subject = null, $to = null, $content = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should remove the ability to set these to null since the helper will not work unless all of these parameters are set.

Choose a reason for hiding this comment

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

What if the user wants to create the object and then add personalizations separately, e.g. for sending to many recipients?

Choose a reason for hiding this comment

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

Also what about using transactional templates where the subject isn't set programmatically at all?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Tzafra,

Would you mind opening up a new issue so that we can get better visibility on this issue?

…them as required. This is based on Elmer's comments on PR sendgrid#408
@caseyw
Copy link
Contributor Author

caseyw commented Jul 1, 2017

Updated. The branch doesn't match the change so if you'd prefer a new PR let me know.

@thinkingserious thinkingserious merged commit e6f7c9e into sendgrid:master Jul 1, 2017
@thinkingserious
Copy link
Contributor

Hello @caseyw,

Thanks again for the PR!

We appreciate your contribution and look forward to continued collaboration. Thanks!

Team SendGrid DX

@thinkingserious thinkingserious changed the title Optional mail constructor arguments now set those available Update constructor to signify which parameters are required for sending all email Jul 1, 2017
thinkingserious added a commit that referenced this pull request Jul 1, 2017
…ch parameters are required for sending all email
@ghost ghost mentioned this pull request Jan 24, 2018
@saruman
Copy link
Contributor

saruman commented Jan 29, 2018

Hello.
I don't understand exactly why you accept this PR.
Can you explain with details why is this change is so important to require now the parameters in Mail?
Please let me know.

@thinkingserious
Copy link
Contributor

Hello @saruman,

Those parameters are required by the underlying API.

That said, we've had conversations about removing this stringent requirement here. It's best if you voice your concerns there. Thank you!

With Best Regards,

Elmer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: code review request requesting a community code review or review from Twilio
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants