#465 use guzzle for http transport #618
Conversation
|
Out of memory issues on travis, it passed here: https://travis-ci.org/milan/Elastica/builds/25789973. Not sure if this can be rerun without another push, but give me a shout if there is anything you'd want to change anyway. |
|
I restarted the build. Looks good, but will have a deeper look into it during the weekend. |
| * @param bool $persistent False if not persistent connection | ||
| * @return resource Connection resource | ||
| */ | ||
| protected function getGuzzleClient($baseUrl, $persistent = true) |
There was a problem hiding this comment.
We use the naming convention with _ for protected functions
|
What you added here is really nice. Can you also update the changes.txt file? There is on thing that worries me. A lot of code in the http transport and the guzzle transport are the same. Shouldn't we either have a common base class or that Guzzle extends http and overwrites the functions needed? At the moment the problem is, if we fix something in one transport and forget also to copy it to the other, it will be broken. |
|
I was thinking the same thing, but couldn't decide on an approach, I think at a high level they are the same, so would probably lean towards a base class with a concrete exec() function made up abstract methods that get extended by Http/Guzzle implementing just those abstract functions? Could we create a new issue regarding the best approach to refactor this and I'll try and create a pull request with some refactored code once there is a set direction. |
#465 use guzzle for http transport
|
Ok, I merged this one in. Lets discuss this refactoring directly on a new issue. My problem is that Http sounds like the perfect name for the base class. How should we call it? HttpBase? |
Allow guzzle to be used as an optional transport