opensearch-project / opensearch-php

Official PHP Client for OpenSearch
Other
109 stars 58 forks source link

Replace callable with Endpoint factory #237

Closed kimpepper closed 1 week ago

kimpepper commented 3 weeks ago

Description

As part of #233 we want to modernise the API in terms of getting endpoints.

This PR replaces the callable for getting endpoints with a strongly typed EndpointFactory.

In addition, the key for the endpoints is the fully qualified class name. This allows us to use the ::class constant (e.g. \OpenSearch\Endpoints\Bulk::class) which provides an improved DX, autocompletion, phpstan type checks etc.

The use of generics in the method docblock allows IDEs and phpstan to correctly assign the return type

    /**
     * Gets an endpoint.
     *
     * @phpstan-template T of AbstractEndpoint
     * @phpstan-param class-string<T> $class
     * @phpstan-return T
     */
    public function getEndpoint(string $class): AbstractEndpoint;

Issues Resolved

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check here.

dblock commented 2 weeks ago

Same as the other PR, let's look at the changes that are just the non-generated code? I merged https://github.com/opensearch-project/opensearch-php/pull/235 so you can rebase, but still make sure you're not modifying any generated code, only the templates used to generate it. If the project doesn't build/test anymore without those, at least make two separate commits (one of the change, the other for re-generating specs) so we can see the actual code changes in the diff.

kimpepper commented 2 weeks ago

@dblock done :heavy_check_mark:

I'm not sure how the automated generation cron job works. Does it work on PRs too? Do you need to merge first?

It would be great if you could generate a specific commit hash of the spec rather that only the latest.

shyim commented 2 weeks ago

I think we need to support both callback and your new factory. Otherwise it's an break

kimpepper commented 2 weeks ago

OK we should we trigger a deprecation error then, and specify we will drop support of callable in the next major ie. 3.0.0?

shyim commented 2 weeks ago

yes sounds good

kimpepper commented 2 weeks ago

Added BC support for the deprecated $endpoints property.

dblock commented 1 week ago

I came here to say that I am so grateful to @kimpepper for picking the PSR work up and for @stof foe the CRs and @shyim for your support.

dblock commented 1 week ago

@stof looking at you to comment when you’re satisfied with the PR, @kimpepper iterate to green

stof commented 1 week ago

To make unit tests pass, you will need to regenerate the client and namespaces in this PR instead of changing only the templates. And you also need to remove the CI jobs running on PHP 7 due to the new PHP requirement.

kimpepper commented 1 week ago

@stof

To make unit tests pass, you will need to regenerate the client and namespaces in this PR instead of changing only the templates. And you also need to remove the CI jobs running on PHP 7 due to the new PHP requirement.

I think we should merge #238 first to avoid a load of unrelated changes.

PHP 7 was recently removed in #239

dblock commented 1 week ago

I think we should merge #238 first to avoid a load of unrelated changes.

Just did. That was a clean update from spec. Rebase.

kimpepper commented 1 week ago

OK pushed latest generated code and all tests are green :heavy_check_mark: