Skip to content
This repository has been archived by the owner on Nov 26, 2017. It is now read-only.

Handling in HttpTransport when the requested uri does not exist #1431

Merged
merged 2 commits into from
Oct 25, 2012

Conversation

elinw
Copy link
Contributor

@elinw elinw commented Aug 5, 2012

The transports are currently not handling it well when the domain for a requested uri completely does not exist. At this point they are somewhat inconsistent in what they do (e.g. stream silences the message and then throws an exception similarly to what I propose here for socket). Rather than use a generic exception message this uses php and curl errors to give more detailed information about what went wrong.

@elinw
Copy link
Contributor Author

elinw commented Aug 5, 2012

These came up when testing bad urls. The challenge was that fsockopen() sends a warning when the uri is not there ... the sending of the warning was making it very difficult to handle problem so this was the solution I finally came to after going around in circles for a while.

@piotr-cz
Copy link
Contributor

piotr-cz commented Aug 7, 2012

Some time ago I've send a PR #1022 that was addressing this issue across all transports ( fix for Curl has been merged yesterday in #1438).

I think it's important to keep same convention across all transport and throw an exception in the same place (probably request function).

@elinw
Copy link
Contributor Author

elinw commented Aug 8, 2012

Well .. I agree consistent is good and as you see I also came to the conclusion that silencing was the only option for fsocketopen(). The no conent condition is different than the bad url condition. I also think it's worth considering that there may only be headers and no body (or body and no headers) to cover all scenarios.

@elinw elinw closed this Aug 9, 2012
@elinw elinw reopened this Aug 10, 2012
@elinw
Copy link
Contributor Author

elinw commented Aug 16, 2012

There was a reason for doing it twice but I can't remember what it was (although I will test again with your version and see if it copes with all of my cases). I guess that it really doesn't matter that there are two different exceptions happening, one where the host is not foundand the other where it can't be opened for another reason.

@piotr-cz
Copy link
Contributor

@elinw
More descriptive exceptions are always welcome.
But there are some cases (authentication to external REST service) when there are multiple requests exchanged and every saved request counts.

Besides in my opinion in your previous code both assertions for fsockopen had same result and passing first would mean passing second as well.

Thanks for taking time at it.

@elinw
Copy link
Contributor Author

elinw commented Aug 17, 2012

Yes I was trying to remember why I did that and I think I just was avoiding touching the existing code for no good reason and really more focused on the empty $content problem.

@elinw
Copy link
Contributor Author

elinw commented Oct 10, 2012

Wow that was a not easy exercise in rebasing

// Split the response into headers and body.
$response = explode("\r\n\r\n", $content, 2);

// Get the response headers as an array.
$headers = explode("\r\n", $response[0]);

if (empty($response[1]))
{
throw new UnexpectedValueException('No [1] offset in response.');
Copy link
Contributor

Choose a reason for hiding this comment

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

If we got headers and an empty body, I'm not sure this is necessarily an exception.

Copy link
Contributor

Choose a reason for hiding this comment

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

Went and looked at the spec, a HEAD request by definition won't return a body:

The HEAD method is identical to GET except that the server MUST NOT return a message-body in the response.

So we really shouldn't return an exception here.

pasamio added a commit that referenced this pull request Oct 25, 2012
Handling in HttpTransport when the requested uri does not exist
@pasamio pasamio merged commit e921add into joomla:staging Oct 25, 2012
@elinw elinw deleted the socket branch January 6, 2013 13:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants