Issues

ZF-8107: Zend_Http_Clinet getAdapter() is not needed.

Description

I mentioned #ZF-8027 http://framework.zend.com/issues/browse/ZF-8027

trunk(r18637)'s "getAdapter()" is not needed , and remove, please. (Over API may cause bug..)

Comments

Hi, Kazusuke Sasezaki.

I think getAdapter would bring us some benefit. Do you have any reason why the function should be removed?

I will be happy if you would tell us anything until next minor release.

Because once the function would be released, we must wait next major release for removing funtions.

Hi, Satoru!

I have 3 reasons at least.

  1. I think Zend_Http_Client_Adapter_* is "internal class"(like as Java's). Is not connect() under "encapsulation" ?

$client = new Zend_Http_Client($url);
$client->getAdapter()->read(); //Fatal Error!!
  1. Possibile case.

//-- common or bootstrap file
$client = new Zend_Http_Client();
$client->setAdapter('Zend_Http_Client_Adapter_Socket');
$client->setConfig($config = array('timeout' => 10)); // (Offen,via Zend_Config)


// -- base file
// There is a possibility that someone types code like this. (for special site...
$client->getAdapter()->setConfig(array('timeout' => 1));

// -- execute file
try { 
    $client->request($url);
} catch (Exception $e) {
    // logger...
    log("Error!Error!", $url, $date, $useragnet, $config, $e->getMessage());
    // If logging is limited without enough length, the exception's message will 
    // not be recorded...
}   

//When the error happens,
//A lot of people will see Config File...(and will grep "client->setConfig")
  1. There is no reason more than debugging for getAdapter(). I don't know benefit except debugging. For example, "getConfig()" , "setUserAgent" and other method will also have great benefits to. But current Zend_Http_Client has not these method.

And Unfortunatoly, getter's doc-comment is same as mutator.

As I mentioned in ZF-8027 getAdapter() is useful when debugging. In my case something was setting the adapter to something unexpected, but without a getAdapter() there was no way to even determine the class/type of the adapter.

getAdapter() is also probably useful when using some of the test adapters. For example if you need to dynamically modify the next "response" that the adapter will give.

Hi, Michael. When debugging. Is not there a way to use other extended-class?


//For example,
class Debug_Http_Client extends Zend_Http_Client
{
    public function getAdapter()
    {
        return $this->adapter;
    }   
}   

//$client = new Zend_Http_Client($url);
$client = new Debug_Http_Client($url);
$client->setAdapter(new Zend_Http_Client_Adapter_Test);
$client->getAdapter()->setResponse('test');

Anyway, current getAdapter() is not correct implemented code against its doc-comment.


    //suggestion code
    public function getAdapter()
    {   
        if (!$this->adapter instanceof Zend_Http_Client_Adapter_Interface)
        {   
            $this->setAdapter($this->config['adapter']);
        }
        
        return $this->adapter;
    }

//and, Modifying in 'request()' would be needed.

And, this changeset was Committed without testcode. http://framework.zend.com/code/changelog/…

I'm not completely sure, but I don't think the getAdapter() should be in the business of creating an adapter if it doesn't already exist. (One reason: if this is the case, then there's no way to tell whether the adapter is already set.) I don't think it's necessary to check the type of the adapter since setAdapter() already does this. (i.e. the return type of getAdapter() will never be a lie, because there's no way to set the adapter to anything other than something that implements Zend_Http_Client_Adapter_Interface.)

However, I do think there is a problem arising from the patch in that if getAdapter() exists, all direct reads of $this->adapter (for example, in Zend_Http_Client's request() method) should be converted into reads of getAdapter() instead.

Regarding reason (1) above, I think the problem that calling methods on the adapter directly can lead to nonsensical results is more a consequence of the somewhat strange way the client uses the adapter rather than a problem that arises because getAdapter() exists.

I don't really understand (2).

Regarding (3), I think there probably should be a getConfig() method as well.

Oh, and regarding why I can't extend Zend_Http_Client myself, and add a getAdapter() method to that: in my case the client is being built by a call to a factory object that I don't have any control over.

Michael Stillwell, thank you very much for your comment.

By the enough reason in above comment, I satisfied myself. So, I want to close this issue.

Closing per request.