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

[PROPOSAL] Un-deprecate the builder pattern #257

Open
dblock opened this issue Jan 22, 2025 · 14 comments
Open

[PROPOSAL] Un-deprecate the builder pattern #257

dblock opened this issue Jan 22, 2025 · 14 comments
Labels
enhancement New feature or request

Comments

@dblock
Copy link
Member

dblock commented Jan 22, 2025

What/Why

What are you proposing?

With the PSR work we deprecated the builder pattern.

https://github.com/opensearch-project/opensearch-php/blob/main/src/OpenSearch/ClientBuilder.php#L48

Do we have to? From the user perspective it seems easier to use and automatically detects the available Psr library. So why deprecate it? What are the downsides of the builder pattern? From the upsides I see less code to write to accomplish the same thing.

What users have asked for this feature?

Users that like to write less code.

What problems are you trying to solve?

Give users a way to use the client simply.

Any remaining open questions?

Would love to hear from @kimpepper on this topic before we release 2.4.0 via #244.

@kimpepper
Copy link
Collaborator

The current builder is there to support the legacy ringphp connection approach. If we un-deprecate it, we would not be able to remove ringphp in 3.x.

We could create separate builders for Guzzle and Symfony HTTP client which would reduce the amount of boilerplate.

@dblock
Copy link
Member Author

dblock commented Jan 23, 2025

Ok, I think I am proposing the latter. We can definitely do this later.

@kimpepper
Copy link
Collaborator

Created #271 to simplify creating the http clients.

@kimpepper
Copy link
Collaborator

I think using the auto-discovery approach is still the simplest:

$transport = (new \OpenSearch\TransportFactory())->create();
$endpointFactory = new \OpenSearch\EndpointFactory();
$client = new \OpenSearch\Client($transport, $endpointFactory, []);

@dblock dblock added enhancement New feature or request and removed untriaged labels Jan 30, 2025
@dblock
Copy link
Member Author

dblock commented Jan 30, 2025

I think #271 is mostly solving this, and we just need docs? @kimpepper ?

@kimpepper
Copy link
Collaborator

Yeah some doc updates are still needed.

@kimpepper
Copy link
Collaborator

I added #287 which takes us back to almost exactly the same API as the deprecated ClientBuilder::build() approach.

@dblock
Copy link
Member Author

dblock commented Feb 6, 2025

@kimpepper So should we be recommending these factories as the default construction or what we have in https://github.com/opensearch-project/opensearch-php/blob/main/USER_GUIDE.md? Want to do a pass on that doc and we can close this issue after that's done?

@kimpepper
Copy link
Collaborator

I created #291 to update the user guide.

@derikb
Copy link

derikb commented Feb 7, 2025

Was updating to 2.4 and saw the deprecation notice about ClientBuilder... and I'm super confused about how to replace it. The PR for updating the docs provides some minimal advice but doesn't at all address how the auth should work now for AWS credentials.

@dblock
Copy link
Member Author

dblock commented Feb 7, 2025

@derikb Take a look at dblock/opensearch-php-client-demo#1, there's working code for guzzle and symfony. I had also updated https://github.com/opensearch-project/opensearch-php/blob/main/guides/auth.md.

I think we need to do a better job here. The UPGRADING guide needs to talk about this path, auth.md needs to remove deprecated code examples, and we need to support SigV4 on top of #287 in 2.5.0 (cc: @kimpepper). @derikb maybe once you sort it out for your application you can help at least with the UPGRADING.md part?

@derikb
Copy link

derikb commented Feb 7, 2025

Thanks @dblock I'll take a look and see what I can get working.

@derikb
Copy link

derikb commented Feb 7, 2025

oh and fwiw I totally missed the link to the auth guide way down there at the bottom of the user guide.

@dblock
Copy link
Member Author

dblock commented Feb 7, 2025

oh and fwiw I totally missed the link to the auth guide way down there at the bottom of the user guide.

Maybe surface it in the README better?

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

No branches or pull requests

3 participants