search

sendgrid / sendgrid-php

The Official Twilio SendGrid PHP API Library
https://sendgrid.com
MIT License
1.49k stars 623 forks source link

Address Software Design issues #937

Closed adam-rocska closed 4 years ago

adam-rocska commented 4 years ago

Issue Summary

The API you guys designed is quite comfortable, and covers all sorts of edge cases, which is really cool! Honestly. Nice job.

However, it's implemented in the standard php web-cowboy way, with absolutely no interfaces in use.

This causes big trouble on our end, and is a show-stopper for integrating SendGrid, or at least doing integration via this library, because our software engineering standards :

  1. We have high & pure unit test coverage. With all dependencies of all units mocked properly.
  2. We have a thin integration layer designed for every third party integration (like SendGrid) in which we prefer to provide our own interface implementations instead of base units (such as the Mail object, or the To object.)

The current state that forces us to use concrete units instead of abstractions makes our tests extremely slow, because SendGrid units (such as SendGrid\Mail\Mail) have pretty heavy constructors, and even though we have phpunit to disableOriginalConstructor(), it still affects our test performance.

To give exact numbers : On an 8 core i9 processor one of the test methods gets executed roughly 90 times (lots of edge cases, covered using @dataProviders). A single run takes between 1 and 3 seconds roughly. Running all 90 takes roughly 3 minutes. Normally our tests run way below 50ms on these processors.

I wanted to give elaborate reasoning, and explanation why it would be important (at least for us) to slightly alter the source code of this library, to introduce interfaces for things.

I'm more than happy to provide answers to further questions, and would even offer to contribute the foundational interface extraction to the repository. With PHPStorm, as a very first step it would be perfectly enough for our scope to just run an "extract interface" for the units intended for public use, and it could also be backwards compatible.

Let me know what you think.

With respect, Adam

Technical details:

adam-rocska commented 4 years ago

We don't care anymore. We've forked the library and reworked it properly for ourselves. Cheerio.