Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Throw an exception before invalid response is parsed by getResponse() #1022

Closed
wants to merge 6 commits into from

4 participants

Piotr Sam Moffatt Louis Landry elinw
Piotr

to test: JHttpFactory::getHttp()->head('http://blah')

Parsing invalid response (false, "") throws PHP errors as getResponse() assumes $content is consists headers and content.

Piotr

In case of JHttpTransportStream,
PHP shows a Warning:
Warning: fopen($invaliduri) failed to open stream: HTTP request failed! in ~libraries\joomla\http\transport\stream.php on line 125

But I'm not sure if it's the right thing to supress the errors

Edit:
As I'm thinking about it, fopen should supress any PHP warnings.
Typical scenario may be when the response (after processing) is going to be output as JSON. Any PHP warnings are then appended to the response body, deforming the JSON output.

The check for no HTTP response that I added is supposed to handle it anyway

see: http://php.net/manual/en/function.fopen.php
If the open fails, an error of level E_WARNING is generated. You may use @ to suppress this warning.

Piotr

There is 1 error in Unit test (http://developer.joomla.org/pulls/pulls/1022.html) but seems like it's triggered by something strange outside this PR

Does this PR have any chance to be merged? There are few other issues in JHttp package but I decided to wait so this one can be still merged

Piotr

JHttpTransportCurl fixed by #1438

Sam Moffatt

You'll need to update to master because of the curl changes and would you mind putting in some code to check $php_errormsg for the stream handler so that we can get some insight at least there why it failed?

elinw elinw commented on the diff
libraries/joomla/http/transport/socket.php
@@ -137,6 +137,12 @@ public function request($method, JUri $uri, $data = null, array $headers = null,
$content .= fgets($connection, 4096);
}
+ // Received empty response (invalid uri, connection reached timeout);
+ if ($content === "")
+ {
+ throw new RuntimeException('HTTP request failed');
+ }
+
return $this->getResponse($content);
}
elinw
elinw added a note

What you are doing here is different than what I did in my pull request which relate to the warning from fsockopen() at line 256 similar to what you did with fopen() in stream.

I've tested all transports against invalid url and var_dump'ed the result:

  • for socket it was: ""
  • for curl and stream: false

So I've checked for empty response in the request method to be able to throw an exception in about the same place as in other transports.

Probably emphasizing an convention may not be optimal here as every transport is different.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Louis Landry

@piotr-cz would you mind rebasing this so we can potentially getting it merged? I'm gonna want to give it a test drive for sure, but I'd love to have consistency here. Can you think of a way we might be able to test it?

Note: I'm closing this until it is mergeable again, not because I'm rejecting the work. Please re-open the request when you get it ready to go.

Louis Landry LouisLandry closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 22, 2012
  1. Piotr

    Throw an exception before invalid response is parsed by getResponse()

    piotr-cz authored
    to test: JHttpFactory::getHttp()->head('http://blah')
    
    Parsing invalid response (false, "") throws PHP errors
  2. Piotr

    Part of pull request #1002

    piotr-cz authored
  3. Piotr
Commits on Mar 23, 2012
  1. Piotr
Commits on Apr 3, 2012
  1. Piotr
  2. Piotr

    fixed checkstyle error

    piotr-cz authored
This page is out of date. Refresh to see the latest.
7 libraries/joomla/http/transport/curl.php
View
@@ -55,6 +55,7 @@ public function __construct(JRegistry $options)
* @return JHttpResponse
*
* @since 11.3
+ * @throws RuntimeException
*/
public function request($method, JUri $uri, $data = null, array $headers = null, $timeout = null, $userAgent = null)
{
@@ -134,6 +135,12 @@ public function request($method, JUri $uri, $data = null, array $headers = null,
$content = curl_exec($ch);
curl_close($ch);
+ // Received empty response (invalid uri, connection reached timeout);
+ if ($content === false)
+ {
+ throw new RuntimeException('HTTP request failed');
+ }
+
return $this->getResponse($content);
}
6 libraries/joomla/http/transport/socket.php
View
@@ -137,6 +137,12 @@ public function request($method, JUri $uri, $data = null, array $headers = null,
$content .= fgets($connection, 4096);
}
+ // Received empty response (invalid uri, connection reached timeout);
+ if ($content === "")
+ {
+ throw new RuntimeException('HTTP request failed');
+ }
+
return $this->getResponse($content);
}
elinw
elinw added a note

What you are doing here is different than what I did in my pull request which relate to the warning from fsockopen() at line 256 similar to what you did with fopen() in stream.

I've tested all transports against invalid url and var_dump'ed the result:

  • for socket it was: ""
  • for curl and stream: false

So I've checked for empty response in the request method to be able to throw an exception in about the same place as in other transports.

Probably emphasizing an convention may not be optimal here as every transport is different.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
9 libraries/joomla/http/transport/stream.php
View
@@ -62,6 +62,7 @@ public function __construct(JRegistry $options)
* @return JHttpResponse
*
* @since 11.3
+ * @throws RuntimeException
*/
public function request($method, JUri $uri, $data = null, array $headers = null, $timeout = null, $userAgent = null)
{
@@ -122,7 +123,13 @@ public function request($method, JUri $uri, $data = null, array $headers = null,
$context = stream_context_create(array('http' => $options));
// Open the stream for reading.
- $stream = fopen((string) $uri, 'r', false, $context);
+ $stream = @fopen((string) $uri, 'r', false, $context);
+
+ // When fopen($uri) returns false (invalid uri, connection reached timeout)
+ if ($stream === false)
+ {
+ throw new RuntimeException('HTTP request failed');
+ }
// Get the metadata for the stream, including response headers.
$metadata = stream_get_meta_data($stream);
Something went wrong with that request. Please try again.