feat: enhanced type exception handling #762

misantron · 2018-10-27T13:50:55Z

Closes #754
Closes #987

Checklist

I have made a material change to the repo (functionality, testing, spelling, grammar)
I have read the [Contribution Guide] and my PR follows them.
I 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 in line documentation to the code I modified

Short description of what this PR does:

New Assert class for strict type validation (100% code coverage with tests)
Refactoring using params type assertion

SendGridDX · 2018-10-27T13:51:12Z

All committers have signed the CLA.

lib/helper/Assert.php

peter279k

The const values I think we don't need to add the \.

They should be removed.

…ocker

misantron · 2018-10-28T07:19:42Z

Made some optional improvement of TravisCI deployment:

Add Composer cache to prevent everytime same dependencies installation
Add Prism mock server run from Docker container instead of download and run from .sh script (https://docs.stoplight.io/testing/continuous-integration/travis)

peter279k · 2018-10-28T07:53:51Z

@misantron, I don't think the .travis.yml enhancement is related to this PR.

Please split two PRs.

Thanks.

misantron · 2018-10-28T08:14:56Z

@peter279k These builds throw an error because of Prism server:
https://travis-ci.org/sendgrid/sendgrid-php/builds/447213062
https://travis-ci.org/sendgrid/sendgrid-php/builds/447216145
This changes is an attempt to solve the problem.

peter279k · 2018-10-28T08:39:04Z

All right. And you can create new PR to fix that. It's out of range for this PR.
You can write comment in this PR to mention this issue and refer another PR.

misantron · 2018-10-28T09:16:14Z

Ok. Should I rollback the changes?

misantron · 2018-10-28T18:00:37Z

#766 @peter279k PR for TravisCI is ready

thinkingserious · 2018-10-31T17:51:24Z

Hello @misantron,

Thanks again for the PR!

It's HACKTOBERFEST! We want to show our appreciation by sending you some special Hacktoberfest swag. If you have not already, could you please fill out this form so we can send it to you? Thanks!

Team SendGrid DX

…patch # Conflicts: # lib/mail/Asm.php # lib/mail/EmailAddress.php # lib/mail/Mail.php # test/unit/EmailAddressHelperTest.php

# Conflicts: # composer.json # lib/mail/Asm.php # lib/mail/Attachment.php # lib/mail/BatchId.php # lib/mail/BccSettings.php # lib/mail/BypassListManagement.php # lib/mail/Category.php # lib/mail/ClickTracking.php # lib/mail/CustomArg.php # lib/mail/EmailAddress.php # lib/mail/Footer.php # lib/mail/Ganalytics.php # lib/mail/GroupId.php # lib/mail/GroupsToDisplay.php # lib/mail/Header.php # lib/mail/IpPoolName.php # lib/mail/Mail.php # lib/mail/MailSettings.php # lib/mail/OpenTracking.php # lib/mail/Personalization.php # lib/mail/SandBoxMode.php # lib/mail/Section.php # lib/mail/SendAt.php # lib/mail/SpamCheck.php # lib/mail/SubscriptionTracking.php # lib/mail/Substitution.php # lib/mail/TemplateId.php # test/BaseTestClass.php # test/unit/EmailAddressTest.php # test/unit/MailGetContentsTest.php

…to issue754-patch

kampalex · 2020-07-04T10:54:00Z

This is not a official review, just some thoughts about contribution yet:
Good work!

In file lib/mail/Asm.php, I suggest to replace 'Twilio' in the TypeException message because it's unnecessary:
...must be an instance of Twilio SendGrid\Mail\...
to
...must be an instance of SendGrid\Mail\...

The Assert::email method doesn't contain the invalid 'string value' in the TypeException message (as requested in #987). Currently, it only contains the property name.

The EmailAddress::isValidEmailAddress logic can link to the Assert::email method like this:

try {
  Assert::email($emailAddress, 'emailAddress');
  return true;
} catch (TypeException $exception) {
  return false;
}

But as seen in all current usages (for now, only Mail::setFrom is left), this method might be unnecessary because every 'else' scenario leads to TypeException. Despite it's a public static method, the intent usage is 'this library only'.

The Mail::setFrom now allows an invalid string; I suggest to change it to:

  if ($email instanceof From) {
    $this->from = $email;
  } else {
    Assert::email($email, 'email', 'Value "$%s" is not an email address.');
    $this->from = new From($email, $name);
  }

After digging a bit more, I'm a bit worried because this PR has also breaking changes:
Some methods now expecting explicit class instances in favor of Assert::isInstanceOf which can lead to different exception/fatal error.
In PHP 5.6: PHP Catchable fatal error
In PHP 7.x: Fatal error: Uncaught TypeError: Argument 1 passed to ...

That's it for now!

lib/mail/Personalization.php

lib/mail/Substitution.php

childish-sambino · 2020-07-06T21:25:47Z

test/unit/GroupsToDisplayTest.php

+
+        $data = range(1, 25);
+        $groups = new GroupsToDisplay($data);
+        $groups->addGroupToDisplay(1);


nit: it's possible the last line is not run (which is where the expected exception should come from) since the exception could be thrown during construction.

This case test addGroupToDisplay()
we add exeactly 25 groups on object construction - ok
than add one more group - got an exception
anything wrong?

If the max allowed was incorrectly defined as 24, then this test would still pass but the last line would not get executed because the constructor would be the one throwing the exception.

lib/mail/Mail.php

test/phpunit.xml.dist

lib/helper/Assert.php

misantron · 2020-07-07T19:59:34Z

build on php 7.4 failed because of Prism server error

misantron added 5 commits October 27, 2018 14:34

Add Assert class for data type validation

450cdfc

Additional asserts

f2876d5

Add whitelist section to PHPUnit config

aa9c296

Add Assert class unit tests, some tests fixes

03d90cd

Add assertion method descriptions

34c22bd

thinkingserious added the status: code review request requesting a community code review or review from Twilio label Oct 27, 2018

misantron added 2 commits October 27, 2018 16:59

Category checking fix

5ba0f8a

GroupsToDisplay fixes and additional tests

11c7725

misantron mentioned this pull request Oct 27, 2018

feat: automatic code style checking sendgrid/php-http-client#109

Merged

6 tasks

peter279k reviewed Oct 27, 2018

View reviewed changes

lib/helper/Assert.php Outdated Show resolved Hide resolved

peter279k reviewed Oct 27, 2018

View reviewed changes

Remove global namespace from constants definition

a20aaf4

misantron closed this Oct 27, 2018

misantron reopened this Oct 27, 2018

Update TravisCI configuration: Composer cache and Prism server from D…

610afcd

…ocker

Rollback TravisCI configuration changes

e3fb459

thinkingserious added hacktoberfest difficulty: hard fix is hard in difficulty type: twilio enhancement feature request on Twilio's roadmap labels Oct 31, 2018

misantron added 2 commits October 21, 2019 22:16

Merge remote-tracking branch 'remotes/upstream/master' into issue754-…

4963a10

…patch # Conflicts: # lib/mail/Asm.php # lib/mail/EmailAddress.php # lib/mail/Mail.php # test/unit/EmailAddressHelperTest.php

Fix test classes naming

46396a7

Fix EmailAddress tests

ccfee46

kampalex mentioned this pull request Jul 2, 2020

Set from email incorrect error message #987

Closed

childish-sambino and others added 6 commits July 2, 2020 14:44

Update composer.json

3fb4aa6

Update composer.json

ea88cea

Fix code and tests after master merge

eafbee1

Merge branch 'issue754-patch' of github.com:misantron/sendgrid-php in…

208fcba

…to issue754-patch

Fix content params assertion

b41937e

misantron and others added 7 commits July 5, 2020 23:40

Replace abstract with final

fbbcbc1

Refactoring assertion messages - display provided value in error message

2a129fc

Fix instance of assertion

b8db656

Rollback breaking changes

3633f5f

Fix typos

510104f

Merge branch 'master' into issue754-patch

493f58d

Update composer.json

d7d2f7f

childish-sambino reviewed Jul 6, 2020

View reviewed changes

Fixes

4cc68cf

Merge branch 'master' into issue754-patch

94f10e7

childish-sambino changed the title ~~Type Exception Handling~~ feat: enhanced type exception handling Jul 8, 2020

childish-sambino approved these changes Jul 8, 2020

View reviewed changes

childish-sambino added 2 commits July 9, 2020 14:50

Merge branch 'master' into issue754-patch

8295a9c

Update Mail.php

3fe2e8e

childish-sambino merged commit e9e0db6 into sendgrid:master Jul 9, 2020

childish-sambino mentioned this pull request Aug 28, 2020

Simplified TypeException error #738

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enhanced type exception handling #762

feat: enhanced type exception handling #762

misantron commented Oct 27, 2018 •

edited by childish-sambino

Loading

SendGridDX commented Oct 27, 2018 •

edited

Loading

peter279k left a comment

misantron commented Oct 28, 2018

peter279k commented Oct 28, 2018 •

edited

Loading

misantron commented Oct 28, 2018

peter279k commented Oct 28, 2018

misantron commented Oct 28, 2018

misantron commented Oct 28, 2018

thinkingserious commented Oct 31, 2018

kampalex commented Jul 4, 2020

childish-sambino Jul 6, 2020

misantron Jul 7, 2020

childish-sambino Jul 8, 2020

misantron commented Jul 7, 2020

feat: enhanced type exception handling #762

feat: enhanced type exception handling #762

Conversation

misantron commented Oct 27, 2018 • edited by childish-sambino Loading

Checklist

Short description of what this PR does:

SendGridDX commented Oct 27, 2018 • edited Loading

peter279k left a comment

Choose a reason for hiding this comment

misantron commented Oct 28, 2018

peter279k commented Oct 28, 2018 • edited Loading

misantron commented Oct 28, 2018

peter279k commented Oct 28, 2018

misantron commented Oct 28, 2018

misantron commented Oct 28, 2018

thinkingserious commented Oct 31, 2018

kampalex commented Jul 4, 2020

childish-sambino Jul 6, 2020

Choose a reason for hiding this comment

misantron Jul 7, 2020

Choose a reason for hiding this comment

childish-sambino Jul 8, 2020

Choose a reason for hiding this comment

misantron commented Jul 7, 2020

misantron commented Oct 27, 2018 •

edited by childish-sambino

Loading

SendGridDX commented Oct 27, 2018 •

edited

Loading

peter279k commented Oct 28, 2018 •

edited

Loading