Skip to content

Decouple Builder from Elastic\Elasticsearch\Client #69

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mfonda
Copy link

@mfonda mfonda commented May 2, 2025

This PR decouples Builder from Elastic\Elasticsearch\Client. Builder::search is replaced with Builder::params, returning an array that can then be passed directly to Elastic\Elasticsearch\Client::search.

The rationale behind this change is that I'd like to build a query in a method that does not have access to my instance of Elastic\Elasticsearch\Client. The result of this method will ultimately get passed to where I do have my $client. I'd like to avoid having to pass around $client everywhere I use the query builder.

I see building a query and executing it as two separate concerns.

Please feel free to reject this change. I understand this is a major backwards compatibility break and would require a new major version. Probably not a change there will be any interest in, but nonetheless, I figured I'd open up a PR just in case.

@bram-pkg
Copy link
Collaborator

bram-pkg commented May 5, 2025

This is a good initiative. Do you think it would be good to keep support for ES client though? It is a nice convenience for developers, allowing them to chain on the search call onto their query, rather than needing to wrap the query in another call

@mfonda
Copy link
Author

mfonda commented May 6, 2025

@bram-pkg in my opinion, it's better to get rid of the dependency on the ES client. It certainly is a nice convenience to be able to chain on a search call, but the alternative isn't that much more inconvenient--just a single extra line to perform the actual search. In exchange, you lose a dependency and gain more flexibility in where you can use the builder.

If starting from scratch, I'd go that route, but whether to make the change now is a tough question. It's a big BC break that would require changes to all code using Builder. Although it's a straightforward change to make, that's still a big burden to place on users.

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.

2 participants