Skip to content

Loading…

HttpClient: adapter always reachable through getter if specified on contructor #4925

Closed
wants to merge 3 commits into from

5 participants

@Slamdunk

No description provided.

@Ocramius Ocramius commented on an outdated diff
library/Zend/Http/Client.php
@@ -205,6 +205,10 @@ public function setAdapter($adapter)
*/
public function getAdapter()
{
+ if ($this->adapter == null) {
@Ocramius Zend Framework member
Ocramius added a note

if (!$this->adapter) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Ocramius
Zend Framework member

Looks good to me :shipit:

@weierophinney weierophinney added a commit that referenced this pull request
@weierophinney weierophinney Merge branch 'hotfix/4925' into develop
Forward port #4925
77f0a59
@weierophinney weierophinney added a commit that closed this pull request
@weierophinney weierophinney Merge branch 'hotfix/4925'
Close #4925
058bc70
@Stiffel

This update seems to have broken ZendOAuth in 2.2.4. When calling the send() method on the object returned form getHttpClient I get the following error.

$client = $this->getAccessToken()->getHttpClient(/Config/, /* URL */);
$client->setMethod(\ZendOAuth\OAuth::GET);
$response = $client->send();

"Call to a member function connect() on a non-object in /zendframework/zendframework/library/Zend/HTTP/Client.php on line 1357"

If I revert the changes made in this update it works fine. I spent a little time looking at this but I'm not sure whats going on. It seems like the call to getAdapter will sometimes return NULL.

~Steve

Zend Framework member

@Stiffel Please file an issue, with a reproduce case (i.e., what's in the Config you commented out up there), so we can track and try and fix it.

@fkoevoets

It seems that $this->getAdapter() on line 819 in /Zend/Http/Client.php is not called when send() is called statically from /zendoauth/library/ZendOAuth/Client.php line 201: return parent::send($request);

Therefore it can create a fatal error as mentioned by @Stiffel

@weierophinney
Zend Framework member

@fkoevoets That's not a static call; that's an extension call, and, as such, it maintains the scope of $this.

@Stiffel

@weierophinney Thanks for the reply. I've opened an issue for this #5074.

@fkoevoets

@weierophinney I am sorry then. But in my case it does not run the getAdapter() from Http/Client.php . So does it run the getAdapter() from ZendOAuth/Client.php??

I see @Stiffel opened the issue for it.

@Stiffel

@fkoevoets I see what you were saying now. The issue is that ZendOAuth/Client.php has extended Zend/Http/Client.php and thus the call to $this->getAdapter() in the send() function of Zend/Http/Client.php is actually running the function getAdapter() in the ZendOAuth/Client.php function which returns null because $this->adapter is not set.

If I update the the call to getAdapter() in Zend/Http/Client.php to refer to its own getAdapter() function with self::getAdapter(); it works correctly. (Keeping in mind @weierophinney comment above that self::getAdapter() is not a static call and is really just saying $this)

I'll add this to the ticket I made as a suggested fix.

@weierophinney
Zend Framework member

If I update the the call to getAdapter() in Zend/Http/Client.php to refer to its own getAdapter() function with self::getAdapter(); it works correctly.

Don't you mean in ZendOAuth\Client here?

Also self:: is a static call; parent:: is not. However, if called from within an instance method, $this will be populated.

@weierophinney weierophinney added a commit to zendframework/zend-http that referenced this pull request
@weierophinney weierophinney Merge pull request zendframework/zf2#4925 from Slamdunk/hotfix/client…
…-adapter

HttpClient: adapter always reachable through getter if specified on contructor
5baedb2
@weierophinney weierophinney added a commit to zendframework/zend-http that referenced this pull request
@weierophinney weierophinney Merge branch 'hotfix/4925' 593b71b
@weierophinney weierophinney added a commit to zendframework/zend-http that referenced this pull request
@weierophinney weierophinney Merge branch 'hotfix/4925' into develop fa8e439
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Showing with 17 additions and 6 deletions.
  1. +7 −6 library/Zend/Http/Client.php
  2. +10 −0 tests/ZendTest/Http/ClientTest.php
View
13 library/Zend/Http/Client.php
@@ -205,6 +205,10 @@ public function setAdapter($adapter)
*/
public function getAdapter()
{
+ if (! $this->adapter) {
+ $this->setAdapter($this->config['adapter']);
+ }
+
return $this->adapter;
}
@@ -812,10 +816,7 @@ public function send(Request $request = null)
$this->redirectCounter = 0;
$response = null;
- // Make sure the adapter is loaded
- if ($this->adapter == null) {
- $this->setAdapter($this->config['adapter']);
- }
+ $adapter = $this->getAdapter();
// Send the first request. If redirected, continue.
do {
@@ -868,7 +869,7 @@ public function send(Request $request = null)
}
// check that adapter supports streaming before using it
- if (is_resource($body) && !($this->adapter instanceof Client\Adapter\StreamInterface)) {
+ if (is_resource($body) && !($adapter instanceof Client\Adapter\StreamInterface)) {
throw new Client\Exception\RuntimeException('Adapter does not support streaming');
}
@@ -896,7 +897,7 @@ public function send(Request $request = null)
rewind($stream);
}
// cleanup the adapter
- $this->adapter->setOutputStream(null);
+ $adapter->setOutputStream(null);
$response = Response\Stream::fromStream($response, $stream);
$response->setStreamName($this->streamName);
if (!is_string($this->config['outputstream'])) {
View
10 tests/ZendTest/Http/ClientTest.php
@@ -339,4 +339,14 @@ public function testIfClientDoesNotForwardAuthenticationToForeignHost()
// because example.org is not a subdomain unter sub.example.org
$this->assertTrue(strpos($client->getLastRawRequest(), $encoded) === false);
}
+
+ public function testAdapterAlwaysReachableIfSpecified()
+ {
+ $testAdapter = new Test();
+ $client = new Client('http://www.example.org/', array(
+ 'adapter' => $testAdapter,
+ ));
+
+ $this->assertSame($testAdapter, $client->getAdapter());
+ }
}
Something went wrong with that request. Please try again.