Skip to content
This repository has been archived by the owner on Jun 27, 2018. It is now read-only.

Added support for using custom HTTP clients #12

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jeremeamia
Copy link

I have added support for using custom HTTP clients in a backward-compatible way. The way this works, is that I made an HTTP Client interface, with two implementations: CurlClient and StreamClient. The code for these implementations was refactored out of the original HTTPMessage::send() method.

You can use the new HTTPMessage::setHttpClient() method to inject any HTTP client that conforms to the aforementioned interface. If none is injected, the HTTPMessage chooses one for you, using a similar strategy found in the original class.

I also improved the code for the fopen-based client, since it was not handling the response data properly like cURL code was doing.

Additional Changes

  • Project-level configuration files typically found in open source PHP projects, including:
    • .editorconfig with a basic PSR-compliant configuration
    • .gitignore for excluding composer files that should not be committed
    • phpunit.xml.dist for configuring PHPUnit. To override this file in your own development environment, you can copy it to phpunit.xml and make changes.
  • A PHPUnit-based test suite that covers the HTTPMessage, CurlClient, and StreamClient classes.
    • Testing code that uses functions with side-effects like curl_exec and fopen is not easy. I opted to use PHP's built-in server (with an object-oriented wrapper) to respond with canned results that I can test against. This setup worked fine in my Mac-based PHP environment, but I'm not sure how well it works in non-bash environments. Can be changed in the future, but wanted to be thorough on the tests.

Test Output

> phpunit --coverage-text
PHPUnit 5.6.2 by Sebastian Bergmann and contributors.

.............                                                     13 / 13 (100%)

Time: 1.19 seconds, Memory: 6.00MB

OK (13 tests, 28 assertions)


Code Coverage Report:      
  2016-11-11 03:03:21      
                           
 Summary:                  
  Classes:  9.30% (4/43)   
  Methods:  1.89% (6/318)  
  Lines:    1.37% (63/4596)

\IMSGlobal\LTI::HTTPMessage
  Methods: 100.00% ( 4/ 4)   Lines: 100.00% ( 11/ 11)
\IMSGlobal\LTI\HTTP::CurlClient
  Methods: 100.00% ( 1/ 1)   Lines: 100.00% ( 35/ 35)
\IMSGlobal\LTI\HTTP::StreamClient
  Methods: 100.00% ( 1/ 1)   Lines: 100.00% ( 17/ 17)

if ($message->ok) {
$chResp = str_replace("\r\n", "\n", $chResp);
$chRespSplit = explode("\n\n", $chResp, 2);
if ((count($chRespSplit) > 1) && (substr($chRespSplit[1], 0, 5) === 'HTTP/')) {
Copy link
Author

Choose a reason for hiding this comment

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

Why was this behavior originally included?

@xow
Copy link
Contributor

xow commented Nov 11, 2016

Thanks very much for working on this. It will help Moodle to better control over the LTI requests and give it the same checks as we have for other HTTP requests. If I get spare time I'll see if I will take a closer look

@jeremeamia
Copy link
Author

@spvickers Let me know what your thoughts are here and if you'd like to talk more about it.

…way.

Also added a test suite that covers the HTTPMessage and client classes.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants