Adding method chaining into the Mail helpers' setters #287

vitya1 · 2016-09-05T16:08:41Z

No description provided.

thinkingserious · 2016-09-07T02:22:32Z

This is awesome @vitya1!

Could you please sign our CLA so I can merge this? https://github.com/sendgrid/sendgrid-php/blob/master/CONTRIBUTING.md#cla

vitya1 · 2016-09-07T10:48:24Z

@thinkingserious I did

vitya1 · 2016-09-13T10:30:15Z

Here is big psr-4 refactoring in additional to the main commit

thinkingserious · 2016-09-14T20:34:15Z

Hi @vitya1,

Could you please resolve the conflicts so that I may test and merge? Thanks!

# Conflicts: # lib/helpers/mail/Mail.php

vitya1 · 2016-09-14T21:52:31Z

Hi @thinkingserious. Conflicts have fixed!

thinkingserious

Great work!

I have one change request. The Mail Helper is supposed to be a single unit, so let's move it to lib\Helper\Mail\Mail.php (and the rest of the classes to lib\Helper\Mail\Objects or similar), so that I can just do use SendGrid\Helper\Mail. The next two helpers we plan to add are for templates and attachments. Those would be under lib\Helper\Templates and lib\Helper\Attachments.

Please let me know if you have questions about this or have other ideas.

vitya1 · 2016-09-15T10:10:32Z

As my mind, in case you plan to add pretty big number of classes in lib\Helper\Templates or lib\Helper\Attachments - it's good idea to group helper's classes. But if you just want to extend mail helper functional - better do not move classes for some time. For ex. maybe better to extend existing Attachment class (especially it has only getters and setters)?

thinkingserious · 2016-09-15T17:27:53Z

@vitya1,

Thanks for taking the time to elaborate your thoughts. Your proposal makes sense.

I would suggest we then just move everything to Helper\Mail. So that if we create a helper for say, contact management, that would go in Helper\ContactManagement

vitya1 · 2016-09-15T20:44:36Z

Ok, let I move

vitya1 · 2016-09-19T08:26:27Z

Done

thinkingserious · 2016-09-27T20:27:33Z

@vitya1

Can you please update the dependency to php-http-client to v3.4?

Check out this release: sendgrid/php-http-client#6

Thanks!

vitya1 · 2016-09-27T21:00:28Z

@thinkingserious
Sure

thinkingserious · 2016-09-27T21:08:55Z

@vitya1,

Ah, your tests are failing because of this pull request: sendgrid/php-http-client#9

Could you please update your tests accordingly?

Also, please use:
"sendgrid/php-http-client": "~3.4"

Thanks!

vitya1 · 2016-09-27T21:41:52Z

@thinkingserious
forgot about tests, but now it's ok.

thinkingserious · 2016-10-05T20:15:04Z

@vitya1,

It looks like we have some conflicts now. Could you please update the branch? Thanks!

# Conflicts: # test/unit/SendGridTest.php

vitya1 · 2016-10-06T21:35:41Z

@thinkingserious,
Done. All conflicts are fixed now. Could the branch be merged before the next ones?

thinkingserious · 2016-10-12T19:41:08Z

@vitya1,

This ticket finally came back up to the top of our queue. It looks like it needs some further conflicts to be resolved. Can you take care of that please? Thanks!

# Conflicts: # lib/SendGrid.php

vitya1 · 2016-10-13T11:08:51Z

@thinkingserious, it's actually nice.
I resolved conflicts and the branch good to merge

SendGridDX · 2017-05-23T14:48:40Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 4 committers have signed the CLA.

✅ vitya1
✅ thinkingserious
❌ Victor Sheremetov
❌ skytime

Victor Sheremetov, skytime seem not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

vitya1 · 2017-05-23T14:56:53Z

@thinkingserious
Finally, I did it. But I'm not sure about version. It was 6.0.0 in my old commits, actual product version is 5.X.X. Could you check which is correct?

thinkingserious · 2017-05-23T16:04:16Z

Hello @vitya1,

Thanks!

Please revert any changes relating to versioning. You can leave those as-is. I will go ahead and update those after merge.

Additionally, it looks like there are several people who have contributed commits that have not signed a CLA. I can not merge these changes unless everyone who has committed signs the CLA. This is now much easier, just click on the link and there is short form to fill out. No more downloading and signing. But, each person must have an email attached to their GitHub account. Please advise.

With Best Regards,

Elmer

thinkingserious · 2017-06-07T18:13:22Z

Hi @vitya1,

Is there anything I can do to help?

vitya1 · 2017-06-22T08:47:27Z

Hi @thinkingserious,
These commits were made in some different computers with different git author configurations. And unfortunately I don't access to one of those emails now :(
And that is a trouble with signing.

vitya1 · 2017-06-22T08:50:46Z

I can try to cherry pick commits to another branch, but I'm not sure it will be helpful.
How do you think?

thinkingserious · 2017-06-23T16:25:56Z

Hi @vitya1,

Since we can't merge this as is, I would suggest you create a new PR with your current GitHUb account. I hope that is not too much trouble :(

With Best Regards,

Elmer

albert-agelviz · 2017-09-11T09:23:52Z

lib/Helper/Mail/Attachment.php

+ * @package SendGrid\Helper
+ */
+class Attachment implements \JsonSerializable
+{


If a user needs an Attachment, why can't we use the constructor to set the properties?, also, I don't see any type hint in the code, this feature has been able since 5.1 version and we have a base compatibility with 5.6.

No doc blocks, if I want to contribute, I need to know what values I expect for the scalar values since these can't be type hinted, please add them, let the people understand your code !

instead of setContent($content what about setContent(Content $content), or better yet:

public function __construct(Content $content, Type $type... etc)

albert-agelviz · 2017-09-11T09:26:04Z

lib/Helper/Mail/BccSettings.php

+ * Class BccSettings
+ * @package SendGrid\Helper
+ */
+class BccSettings implements \JsonSerializable


Same comment for Attachment.

albert-agelviz · 2017-09-11T09:26:10Z

lib/Helper/Mail/BypassListManagement.php

+ * Class BypassListManagement
+ * @package SendGrid\Helper
+ */
+class BypassListManagement implements \JsonSerializable


Same comment for Attachment.

albert-agelviz · 2017-09-11T09:26:15Z

lib/Helper/Mail/ClickTracking.php

+ * Class ClickTracking
+ * @package SendGrid
+ */
+class ClickTracking implements \JsonSerializable


Same comment for Attachment.

albert-agelviz · 2017-09-11T09:26:22Z

lib/Helper/Mail/Content.php

+ * Class Content
+ * @package SendGrid\Helper
+ */
+class Content implements \JsonSerializable


Same comment for Attachment.

albert-agelviz · 2017-09-11T09:28:08Z

lib/Helper/Mail/Email.php

+ * Class Email
+ * @package SendGrid\Helper
+ */
+class Email implements \JsonSerializable


Same comment for Attachment.

Also, this is a Value Object, why are we not validating it?, what if I pass to $email an integer? or ¯\_(ツ)_/¯ as value?, please, let's validate our Value Objects and help our base class Mail to be always in a valid state !.

albert-agelviz · 2017-09-11T09:28:27Z

lib/Helper/Mail/Footer.php

+ * Class Footer
+ * @package SendGrid\Helper
+ */
+class Footer implements \JsonSerializable


Same comment for Attachment.

albert-agelviz · 2017-09-11T09:28:35Z

lib/Helper/Mail/Ganalytics.php

+ * Class Ganalytics
+ * @package SendGrid\Helper
+ */
+class Ganalytics implements \JsonSerializable


Same comment for Attachment.

albert-agelviz · 2017-09-11T09:30:20Z

lib/Helper/Mail/Mail.php

+ * Class Mail
+ * @package SendGrid\Helper
+ */
+class Mail implements \JsonSerializable


Same comment for Attachment.

And also, Public properties?, why?, this is a very delicate class !, it represents the message that will be send !, let's be more strict and make them private, and type hint people, let's type hint more !!, it doesn't hurt.

albert-agelviz · 2017-09-11T09:30:28Z

lib/Helper/Mail/MailSettings.php

+ * Class MailSettings
+ * @package SendGrid\Helper
+ */
+class MailSettings implements \JsonSerializable


Same comment for Attachment.

albert-agelviz · 2017-09-11T09:30:34Z

lib/Helper/Mail/OpenTracking.php

+ * Class OpenTracking
+ * @package SendGrid\Helper
+ */
+class OpenTracking implements \JsonSerializable


Same comment for Attachment.

albert-agelviz · 2017-09-11T09:30:39Z

lib/Helper/Mail/Personalization.php

+ * Class Personalization
+ * @package SendGrid\Helper
+ */
+class Personalization implements \JsonSerializable


Same comment for Attachment.

albert-agelviz · 2017-09-11T09:30:45Z

lib/Helper/Mail/ReplyTo.php

+ * Class ReplyTo
+ * @package SendGrid
+ */
+class ReplyTo implements \JsonSerializable


Same comment for Attachment.

albert-agelviz · 2017-09-11T09:30:51Z

lib/Helper/Mail/SandBoxMode.php

+ * Class SandBoxMode
+ * @package SendGrid\Helper
+ */
+class SandBoxMode implements \JsonSerializable


Same comment for Attachment.

albert-agelviz · 2017-09-11T09:30:58Z

lib/Helper/Mail/SubscriptionTracking.php

+ * Class SubscriptionTracking
+ * @package SendGrid\Helper
+ */
+class SubscriptionTracking implements \JsonSerializable


Same comment for Attachment.

albert-agelviz · 2017-09-11T09:31:03Z

lib/Helper/Mail/TrackingSettings.php

+ * Class TrackingSettings
+ * @package SendGrid\Helper
+ */
+class TrackingSettings implements \JsonSerializable


Same comment for Attachment.

albert-agelviz · 2017-09-11T09:33:45Z

Made some comments for this PR @thinkingserious , also I don't see any new tests in this PR, we should be very strict to don't consider any PR without unit, functional or integrations tests (if tests are required of course 😄 )

thinkingserious · 2017-09-11T21:15:37Z

Thanks for taking the time to review @shonjord!

thinkingserious · 2017-09-12T05:19:40Z

Hi @vitya1,

I will be incorporating some of your changes to the internals of the new library and you will be credited accordingly. For now, I would greatly appreciate your feedback here.

With Best Regards,

Elmer

Adding method chaining into the Mail helpers' setters

06acae2

thinkingserious added type: community enhancement feature request not on Twilio's roadmap status: cla signed labels Sep 9, 2016

PSR-4 refactoring

3b2efa9

skytime and others added 2 commits September 15, 2016 00:37

Merge branch 'master' of https://github.com/sendgrid/sendgrid-php

a9b96df

# Conflicts: # lib/helpers/mail/Mail.php

Fix merge conflicts

5fa028c

thinkingserious requested changes Sep 15, 2016

View reviewed changes

Move mail helpers to the new place

3d92b54

thinkingserious approved these changes Sep 27, 2016

View reviewed changes

Update the dependency to php-http-client version

42f7087

Test fixing

951bffc

Merge branch 'master' of https://github.com/sendgrid/sendgrid-php

b6fc423

# Conflicts: # test/unit/SendGridTest.php

Merge remote-tracking branch 'remotes/upstream/master'

bedbe8c

# Conflicts: # lib/SendGrid.php

resolve conflicts

31e2585

thinkingserious added status: cla needed and removed status: cla signed labels Jun 19, 2017

albert-agelviz reviewed Sep 11, 2017

View reviewed changes

thinkingserious closed this Sep 12, 2017

Adding method chaining into the Mail helpers' setters #287

Adding method chaining into the Mail helpers' setters #287

Conversation

vitya1 commented Sep 5, 2016

thinkingserious commented Sep 7, 2016

vitya1 commented Sep 7, 2016

vitya1 commented Sep 13, 2016

thinkingserious commented Sep 14, 2016

vitya1 commented Sep 14, 2016

thinkingserious left a comment

Choose a reason for hiding this comment

vitya1 commented Sep 15, 2016

thinkingserious commented Sep 15, 2016

vitya1 commented Sep 15, 2016

vitya1 commented Sep 19, 2016

thinkingserious commented Sep 27, 2016

vitya1 commented Sep 27, 2016

thinkingserious commented Sep 27, 2016

vitya1 commented Sep 27, 2016 • edited Loading

thinkingserious commented Oct 5, 2016

vitya1 commented Oct 6, 2016

thinkingserious commented Oct 12, 2016

vitya1 commented Oct 13, 2016

SendGridDX commented May 23, 2017 • edited Loading

vitya1 commented May 23, 2017

thinkingserious commented May 23, 2017

thinkingserious commented Jun 7, 2017

vitya1 commented Jun 22, 2017 • edited Loading

vitya1 commented Jun 22, 2017

thinkingserious commented Jun 23, 2017

albert-agelviz Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albert-agelviz commented Sep 11, 2017

thinkingserious commented Sep 11, 2017

thinkingserious commented Sep 12, 2017 • edited Loading

vitya1 commented Sep 27, 2016 •

edited

Loading

SendGridDX commented May 23, 2017 •

edited

Loading

vitya1 commented Jun 22, 2017 •

edited

Loading

albert-agelviz Sep 11, 2017 •

edited

Loading

thinkingserious commented Sep 12, 2017 •

edited

Loading