Repository navigation
Conversation
|
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 |
|
@thinkingserious I did |
|
Here is big psr-4 refactoring in additional to the main commit |
|
Hi @vitya1, Could you please resolve the conflicts so that I may test and merge? Thanks! |
# Conflicts: # lib/helpers/mail/Mail.php
|
Hi @thinkingserious. Conflicts have fixed! |
thinkingserious
left a comment
There was a problem hiding this comment.
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.
|
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)? |
|
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 |
|
Ok, let I move |
|
Done |
|
Can you please update the dependency to php-http-client to v3.4? Check out this release: sendgrid/php-http-client#6 Thanks! |
|
@thinkingserious |
|
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: Thanks! |
|
@thinkingserious |
|
It looks like we have some conflicts now. Could you please update the branch? Thanks! |
# Conflicts: # test/unit/SendGridTest.php
|
@thinkingserious, |
|
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
|
@thinkingserious, it's actually nice. |
|
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. |
|
@thinkingserious |
|
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 |
|
Hi @vitya1, Is there anything I can do to help? |
|
Hi @thinkingserious, |
|
I can try to cherry pick commits to another branch, but I'm not sure it will be helpful. |
|
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 |
| * @package SendGrid\Helper | ||
| */ | ||
| class Attachment implements \JsonSerializable | ||
| { |
There was a problem hiding this comment.
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)| * Class BccSettings | ||
| * @package SendGrid\Helper | ||
| */ | ||
| class BccSettings implements \JsonSerializable |
| * Class BypassListManagement | ||
| * @package SendGrid\Helper | ||
| */ | ||
| class BypassListManagement implements \JsonSerializable |
| * Class ClickTracking | ||
| * @package SendGrid | ||
| */ | ||
| class ClickTracking implements \JsonSerializable |
| * Class Content | ||
| * @package SendGrid\Helper | ||
| */ | ||
| class Content implements \JsonSerializable |
| * Class Email | ||
| * @package SendGrid\Helper | ||
| */ | ||
| class Email implements \JsonSerializable |
There was a problem hiding this comment.
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 !.
| * Class Footer | ||
| * @package SendGrid\Helper | ||
| */ | ||
| class Footer implements \JsonSerializable |
| * Class Ganalytics | ||
| * @package SendGrid\Helper | ||
| */ | ||
| class Ganalytics implements \JsonSerializable |
| * Class Mail | ||
| * @package SendGrid\Helper | ||
| */ | ||
| class Mail implements \JsonSerializable |
There was a problem hiding this comment.
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.
| * Class MailSettings | ||
| * @package SendGrid\Helper | ||
| */ | ||
| class MailSettings implements \JsonSerializable |
| * Class OpenTracking | ||
| * @package SendGrid\Helper | ||
| */ | ||
| class OpenTracking implements \JsonSerializable |
| * Class Personalization | ||
| * @package SendGrid\Helper | ||
| */ | ||
| class Personalization implements \JsonSerializable |
| * Class ReplyTo | ||
| * @package SendGrid | ||
| */ | ||
| class ReplyTo implements \JsonSerializable |
| * Class SandBoxMode | ||
| * @package SendGrid\Helper | ||
| */ | ||
| class SandBoxMode implements \JsonSerializable |
| * Class SubscriptionTracking | ||
| * @package SendGrid\Helper | ||
| */ | ||
| class SubscriptionTracking implements \JsonSerializable |
| * Class TrackingSettings | ||
| * @package SendGrid\Helper | ||
| */ | ||
| class TrackingSettings implements \JsonSerializable |
|
Made some comments for this PR @thinkingserious , also I don't see any new |
|
Thanks for taking the time to review @shonjord! |
No description provided.