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

Issue Type: Task Created: 2009-10-20T04:57:32.000+0000 Last Updated: 2009-10-23T07:08:41.000+0000 Status: Resolved Fix version(s): Reporter: Kazusuke Sasezaki (sasezaki) Assignee: Dolf Schimmel (Freeaqingme) (freak) Tags: - Zend_Http_Client

Related issues: - ZF-8027



I mentioned #ZF-8027

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


Posted by Satoru Yoshida (satoruyoshida) on 2009-10-20T18:34:37.000+0000

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.

Posted by Kazusuke Sasezaki (sasezaki) on 2009-10-21T07:37:00.000+0000

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!!
  2. Possibile case.

     //-- common or bootstrap file
     $client = new Zend_Http_Client();
     $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 { 
     } 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")
  3. 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.

Posted by Michael Stillwell (mjs) on 2009-10-22T02:25:55.000+0000

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.

Posted by Kazusuke Sasezaki (sasezaki) on 2009-10-22T07:20:33.000+0000

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

<pre class="highlight">
//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);

Posted by Kazusuke Sasezaki (sasezaki) on 2009-10-22T07:31:53.000+0000

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

<pre class="highlight">
    //suggestion code
    public function getAdapter()
        if (!$this->adapter instanceof Zend_Http_Client_Adapter_Interface)
        return $this->adapter;

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

And, this changeset was Committed without testcode.…

Posted by Michael Stillwell (mjs) on 2009-10-23T02:59:42.000+0000

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.

Posted by Michael Stillwell (mjs) on 2009-10-23T03:12:11.000+0000

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.

Posted by Kazusuke Sasezaki (sasezaki) on 2009-10-23T06:19:33.000+0000

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.

Posted by Dolf Schimmel (Freeaqingme) (freak) on 2009-10-23T07:08:38.000+0000

Closing per request.

Have you found an issue?

See the Overview section for more details.


© 2006-2018 by Zend, a Rogue Wave Company. Made with by awesome contributors.

This website is built using zend-expressive and it runs on PHP 7.