Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added transport to support egeloen/http-adapter #727

Merged
merged 2 commits into from
Nov 30, 2014

Conversation

agallou
Copy link
Contributor

@agallou agallou commented Nov 23, 2014

Supporting egeloen/http-adapter will
allow to make http calls using multiple libraries (including Guzzle3) :

  • Buzz
  • Cake
  • cURL
  • FileGetContents
  • Fopen
  • Guzzle
  • GuzzleHttp
  • Httpful
  • React
  • Socket
  • Zend1
  • Zend2

Here is an example of how to use it :

  $guzzle3 = new \Guzzle\Http\Client();

  $httpAdapter = new \Ivory\HttpAdapter\GuzzleHttpAdapter();
  $httpAdapterTransport = new \Elastica\Transport\HttpAdapter(null, $httpAdapter);

  $elastica = new \Elastica\Client(array(
      'connections' => array(
          array(
          '   host' => 'localhost',
              'port' => 9200,
              'transport' => $httpAdapterTransport,
          )
      )
  ));

  var_dump($elastica->getIndex('livre')->getType('livre')->getDocument('9782212136777')->getData());

Supporting [egeloen/http-adapter](https://github.com/egeloen/ivory-http-adapter) will
allow to make http calls using  multiple libraries (including Guzzle3) :

* Buzz
* Cake
* cURL
* FileGetContents
* Fopen
* Guzzle
* GuzzleHttp
* Httpful
* React
* Socket
* Zend1
* Zend2

Here is an example of how to use it :

```php
  $guzzle3 = new \Guzzle\Http\Client();

  $httpAdapter = new \Ivory\HttpAdapter\GuzzleHttpAdapter();
  $httpAdapterTransport = new \Elastica\Transport\HttpAdapter(null, $httpAdapter);

  $elastica = new \Elastica\Client(array(
      'connections' => array(
          array(
          '   host' => 'localhost',
              'port' => 9200,
              'transport' => $httpAdapterTransport,
          )
      )
  ));

  var_dump($elastica->getIndex('livre')->getType('livre')->getDocument('9782212136777')->getData());
```
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.5%) when pulling 0683bf1 on agallou:http_adapter into c10b047 on ruflin:master.

*
* @return string
*/
protected function getUri(ElasticaRequest $request, Connection $connection)
Copy link
Owner

Choose a reason for hiding this comment

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

Please use _getUri for the protected methods.

@ruflin
Copy link
Owner

ruflin commented Nov 24, 2014

Does this actually mean we don't need the Guzzle Adapter anymore?

Can you please make the protected methods starting with _ as this is the library convention?

@agallou
Copy link
Contributor Author

agallou commented Nov 24, 2014

I've renamed the methods according the library convention.

The only thing that is not in this adapter is getting the proxy configuration from the connection and use it into the httpadapter that is used (guzzle3, guzzle, buzz, curl...). That's something that could be configured when instanciating the adapter.

Yes, the guzzle adapter is not needed anymore (besides in the current guzzle adapter, guzzle could not been injected properly). But its a bit more complicated to use it (as you can see if my example there is some injection to do), but more decoupled.

If you want to get rid of the current code of the Guzzle transport and avoid breaking BC, we could keep the Guzzle class but extend the HttpAdapter and create the guzzle instance in the class constructor (and setting the proxy on the guzzle instance). But This would require to put egeloen/http-adapter as requirement if someone wants to use Guzzle with Elastica. How do you want to preceed with this ?

@ruflin
Copy link
Owner

ruflin commented Nov 30, 2014

@agallou I suggest we merge it as it is and do optimisation like removing Guzzle at a later stage.

About the configuration: There will soon be some changes probably. Have a look at the last comments. #723

@ruflin ruflin merged commit 9f95232 into ruflin:master Nov 30, 2014
@ruflin
Copy link
Owner

ruflin commented Nov 30, 2014

Ok, I merged it to master and also updated the changes.txt file.

@nurikabe
Copy link

Nice, though note that this relies on v0.6 of egeloen/http-adapter

@ruflin
Copy link
Owner

ruflin commented Apr 14, 2015

@nurikabe Does this mean it doesn't work with the never versions?

@agallou
Copy link
Contributor Author

agallou commented Apr 14, 2015

@ruflin I haven't tested it on newer versions, but PSR-7 is still in review (https://github.com/php-fig/fig-standards/blob/master/proposed/http-message-meta.md).
When it will be accepted, I'll make a PR to made the changes if needed (I suppose that egeloen/http-adapter will adjust to use the standard interface).

@nurikabe
Copy link

Looks like another couple of weeks for PSR-7. Hopefully it will go through this time.

I didn't dig very deep, but the initial problem I noticed was that Ivory\HttpAdapter\Message\Stream\StringStream has been replaced with https://github.com/phly/http

@ruflin
Copy link
Owner

ruflin commented Apr 17, 2015

Ok, lets wait for PSR-7 then.

@nurikabe
Copy link

PSR-7 approved last week on May 5th.

@ruflin
Copy link
Owner

ruflin commented May 13, 2015

@nurikabe Thanks for the update.
@agallou Time for a Pull request? :-)

@agallou
Copy link
Contributor Author

agallou commented May 13, 2015

@ruflin tomorow is a public holiday, so I'll have a PR by the end of tomorow

@ruflin
Copy link
Owner

ruflin commented May 13, 2015

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants