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

v2.0.0-beta1 Feedback #193

Closed
polyfractal opened this issue Mar 2, 2015 · 12 comments
Closed

v2.0.0-beta1 Feedback #193

polyfractal opened this issue Mar 2, 2015 · 12 comments

Comments

@polyfractal
Copy link
Contributor

Have you used v2.0.0-beta1? Love it? Hate it? Questions how to use it? This is the ticket for you!

@simplechris
Copy link
Contributor

Seems good so far. I remember you mentioning at elastic{on} that you were supporting PHP 5.4, so I've started testing in a 5.4 environment. I believe I've found and issue with async mode when using 5.4, which i've opened an issue for: #201

@polyfractal
Copy link
Contributor Author

Barring anymore glaring issues, I'm hoping to release 2.0 as a stable branch soon. Looks like the major bugs have been hammered out, the rest we can work out as patch / minor versions.

/cc @simplechris @Nickology

@Nickology
Copy link

Looking good on my end 👍

@simplechris
Copy link
Contributor

yeah, sounds good to me. 👍

@simplechris
Copy link
Contributor

So, I just saw that Guzzle has a new release - With the recent acceptance of PSR-7 and the community seems to be rushing to become compatible, Guzzle 6.0.0 was launched yesterday: https://github.com/guzzle/guzzle/releases/tag/6.0.0. It's a large refactor, as you would expect of a major release, but interestingly Guzzle no longer has a dependency on RingPHP, and it has opted to use it's own promises library.

This might have some knock-on for this package, but I don't think it should hold up the 2.0.0 release, but it might mean we have to start working on a 2.1 or 3.0 release sooner than expected :/

I'll try and work on some benchmarks to see what this could mean for the elasticsearch-php package.

EDIT:

Guzzle no longer has a dependency on RingPHP. Due to the use of a middleware system based on PSR-7, using RingPHP and it's middleware system as well adds more complexity than the benefits it provides. All HTTP handlers that were present in RingPHP have been modified to work directly with PSR-7 messages and placed in the GuzzleHttp\Handler namespace. This significantly reduces complexity in Guzzle, removes a dependency, and improves performance. RingPHP will be maintained for Guzzle 5 support, but will no longer be a part of Guzzle 6

it seems like guzzlehttp/ringphp will continue to be maintained, so the release of a new guzzle package probably wont affect the elasticsearch-php package 👍

@polyfractal
Copy link
Contributor Author

negativeman-55f

Agreed that it shouldn't hold up the 2.0 release, if nothing else because I'd rather maintain a library with a slightly out-of-date dependency than the current 1.x version with it's massively out-of-date Guzzle :)

it seems like guzzlehttp/ringphp will continue to be maintained, so the release of a new guzzle package probably wont affect the elasticsearch-php package. 👍

Yeah...but it's only a matter of time before RingPHP falls into neglect if Guzzle isn't using it anymore. Which means the burden of maintenance would likely fall to us (not necessarily a bad thing though).

I'm sad that Guzzle has moved away from RingPHP. ES-PHP doesn't really need many of the conveniences of Guzzle; that's why the original 1.x branch used the Guzzle/HTTP dep (until that was deprecated), and why 2.x only relies on RingPHP and not Guzzle proper.

I guess we'll see based on benchmarks and usability. Many of the changes look like they reduce the complexity in Guzzle and likely help performance when using it's complete feature set...I'm just not sure how much that carries over to our simpler usage of RingPHP.

I'm not convinced about the perf claims regarding the new immutable middleware though: the changes avoid large stacks, but the immutable middleware is doing a lot of memory allocation to support all the closures and return values. PHP is notoriously poor at optimization, so I doubt these are handled very gracefully. But maybe shallower stacks and fewer function calls outweigh the extra allocations, I dunno.

Looking forward
I originally chose Guzzle because it is well supported and I wanted to avoid NIH syndrome. But honestly, the rapid turnover of bwc breaks and features we don't use has been painful. I'm tempted to either help maintain RingPHP, or just roll a simple http middleware like the old CurlMultiConnection and just be done with it.

Argh :(

@fractalf
Copy link

I have trouble getting it to work at all. I'm no expert, but neigher a novice.

The documentation on https://www.elastic.co/guide/en/elasticsearch/client/php-api/2.0/_quickstart.html
says to use ~2.0 (which fails install with composer) so I use ~2.0@beta which installs just fine, but then when doing

require 'vendor/autoload.php';
$client = ClientBuilder::create()->build();

I get

Fatal error: Class 'ClientBuilder' not found in ....php on line 33

Any tips appreciated!

EDIT
Damn those namespaces! ;)
$client = Elasticsearch\ClientBuilder::create()->build(); works. Might want to update the docs

@simplechris
Copy link
Contributor

@fractalf ClientBuilder refers to the Elasticsearch\ClientBuilder class -src/Elasticsearch/ClientBuilder.php

As the class is in the Elasticsearch namespace, you will need to add a use statement, or reference the class via it's fully qualified name:

<?php

require 'vendor/autoload.php';

use Elasticsearch\ClientBuilder;

$client = ClientBuilder::create()->build();

or

<?php

require 'vendor/autoload.php';

$client = \Elasticsearch\ClientBuilder::create()->build();

👍

@fractalf
Copy link

Thanks @simplechris 👍
I just realised 5 minuttes after posting. Good to have here though incase any other temporary missplace their brain for a couple of minuttes (like I did)

@polyfractal
Copy link
Contributor Author

Beaten by @simplechris :)

I'll add the namespace to the docs though, as I imagine that'll be a common tripping point if people are just c/p to get up and running quickly.

@bakura10
Copy link

I originally chose Guzzle because it is well supported and I wanted to avoid NIH syndrome. But honestly, the rapid turnover of bwc breaks and features we don't use has been painful. I'm tempted to either help maintain RingPHP, or just roll a simple http middleware like the old CurlMultiConnection and just be done with it.

Actually, when Guzzle 4 was released, I've heard that it should be the Guzzle version "for at least a few years". Then with all the PSR-7 rush, a lot of BC has been done.

Also, Guzzle is the heart of AWS SDK, and as AWS SDK has been tagged v3 a few days ago, relying on Guzzle 6, I'm pretty sure that Guzzle is going to be more stable now and won't encounter as many BC breaks and major versions.

The additions of both AWS SDK and PSR-7 is what that have surely caused this whole mess ;).

@polyfractal
Copy link
Contributor Author

Only like 6 months delayed (urrrrrgh), but 2.0 has been tagged and released. Yay emot-toot

At some point in the future, we can start to look at Guzzle 6. But it might be nice to stick with RingPHP, if nothing else because it helps eliminate dependency problems with projects that use Guzzle itself.

I think it would be worthwhile to make the responses PSR-7 compatible however, and shouldn't affect operation too much. I'm going to close this issue and open one specifically about PSR-7

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

No branches or pull requests

5 participants