Requests: Add "requests.failed" hook

[pprkut]

Pull Request Type

• I have checked there is no other PR open for the same change.

This is a:

• Bug fix
• New feature
• Code quality improvement

Context

This allows to inspect or alter the exception that is thrown to
the user in case of transport errors, or in case of response
parsing problems.

Detailed Description

We use the hooks to gather analytics about the remote requests we make in out platform, but failed requests slipped through the cracks since there is currently no way to track those. You can get some analytics from when you make the request, but you won't know from just that whether it failed. We'd also like to know why it failed, the error message that was returned, etc.

We've been using the hook in our platform for a couple weeks now and it worked fine for us :)
(Although we've applied this change to 1.8.x)

Quality assurance

• This change does NOT contain a breaking change (fix or feature that would cause existing functionality to change).
• I have commented my code, particularly in hard-to-understand areas.
• I have added unit tests to accompany this PR.
• The (new/existing) tests cover this PR 100%.
• I have (manually) tested this code to the best of my abilities.
• My code follows the style guidelines of this project.

Documentation

For new features:

• I have added a code example showing how to use this feature to the examples directory.
• I have added documentation about this feature to the docs directory.
  If the documentation is in a new markdown file, I have added a link to this new file to the Docs folder README.md file.

[jrfnl]

Thank you @pprkut . I'm not adverse to this change, but would like to see it covered by unit tests.

[pprkut]

Sure thing, just a bit confused by the test organization 😅
If someone could give me a pointer of where the test(s) should be added, I would much appreciate it.

[jrfnl]

Sure thing, just a bit confused by the test organization 😅 If someone could give me a pointer of where the test(s) should be added, I would much appreciate it.

I think the most appropriate place for the tests would be in the RequestsTest class as the hook is being added to the Requests class.

If needs be, we can always move it later (we're working on improving the test suite).

If you need some inspiration on how to test the hook, you may want to have a look at the following tests:

Requests/tests/Transport/BaseTestCase.php

Lines 924 to 957 in 28879ed

	public function testProgressCallback() {
	$mock = $this->getMockBuilder(stdClass::class)->setMethods(array('progress'))->getMock();
	$mock->expects($this->atLeastOnce())->method('progress');
	$hooks = new Hooks();
	$hooks->register('request.progress', array($mock, 'progress'));
	$options = array(
	'hooks' => $hooks,
	);
	$options = $this->getOptions($options);
	Requests::get(httpbin('/get'), array(), $options);
	}
	public function testAfterRequestCallback() {
	$mock = $this->getMockBuilder(stdClass::class)
	->setMethods(array('after_request'))
	->getMock();
	$mock->expects($this->atLeastOnce())
	->method('after_request')
	->with(
	$this->isType('string'),
	$this->logicalAnd($this->isType('array'), $this->logicalNot($this->isEmpty()))
	);
	$hooks = new Hooks();
	$hooks->register('curl.after_request', array($mock, 'after_request'));
	$hooks->register('fsockopen.after_request', array($mock, 'after_request'));
	$options = array(
	'hooks' => $hooks,
	);
	$options = $this->getOptions($options);
	Requests::get(httpbin('/get'), array(), $options);
	}

[pprkut]

Thank you @jrfnl! I will take a look!

[jrfnl]

@pprkut Just checking in to see how it's going...

[pprkut]

@jrfnl I still have it on my list, but haven't found the time yet :-/
It's still gonna take me some time. My goal would be to finish this up before the end of the year

[pprkut]

@jrfnl I think I got all the different test cases. Let me know what you think :)

I'm also fine squashing the commits, etc. Just focused on getting the tests in for now

[schlessera]

@pprkut,

Thanks for the work on this PR so far!

I have been thinking about this some more, and I actually fail to see what the use case is where you would need this action. I'm not opposed to adding actions, but they need to add something to warrant their added maintenance cost. Here, I'm not sure this is the case. Maybe you can help me figure this out.

The way I see it right now, you could get everything you need by adding a try/catch block around the Requests::request() call, instead of doing the call right away and then listening on a requests/failed hook for a possible exception.

Do you have a use case in mind where just adding a try/catch block around Requests::request() would not be enough or not be feasible?

[pprkut]

@schlessera Sure, I'll try to outline our usecase :)

We use the hooks to collect analytics about all HTTP requests that our system makes. The end result then looks like this:

The hooks we use are:

• requests.before_request
• curl.after_request
• requests.before_redirect
• requests.after_request

The way it works is that requests.before_request initiates the analytics data, but we need the response data to finalize it. curl.after_request adds IP information to the analytics, requests.before_redirect finalizes a request with a 301 response (for example), and requests.after_request finalizes all other successful requests.

However, if a request fails, because of wrong options or curl errors, we have the analytics data initiated, but no other hook is triggered so we never get an indicator that the request is "completed" (and that means the anayltics are never stored and won't show on that dashboard). We could do that with exception handling on our side of the code, but then we'd have to either do that everywhere we make an HTTP request with Requests and call the analytics library in all those places to indicate the request is completed and failed, or implement a wrapper around Requests that does that exception handling. But that just seems weird and clunky since most of the functionality is already handled via hooks.

Adding the requests.failed hook looked like the cleanest way to implement this, and would also allow other people to do something similar. That's why I went in that direction.

[SMillerDev]

Rebased, and I think the "Status: needs tests" label can go now since it has tests.

[SMillerDev]

Rebased after doing some work on our fork accidentally dropped some commits.

[jrfnl]

@pprkut PR #833 has just been merged, which should allow for simplifying the tests in this PR. Would you like to update the PR ? Or do you expect us to get your PR to a mergable state ?

[pprkut]

@jrfnl I've been following along the other PR 🙂
I'm on vacation right now until the weekend, but I'll see what I can do to get this PR updated 👍

[jrfnl]

@pprkut Excellent! Take your time and I look forward to seeing the next iteration. In the mean time: enjoy your break!

[pprkut]

@jrfnl Should be updated 🙂