Fix for PHPStan issues #1439

ezimuel · 2025-03-22T09:46:39Z

This PR fixes the PHPStan issues reported in #1425, #1424, #1423 and #1346. It also improved the PHPStan rule level to 5.

AJenbo · 2025-03-22T17:36:17Z

src/Endpoints/AsyncSearch.php

+	 *     human?: bool, // Return human readable values for statistics. (DEFAULT: true)
+	 *     error_trace?: bool, // Include the stack trace of returned errors. (DEFAULT: false)
+	 *     source?: string, // The URL-encoded request definition. Useful for libraries that do not accept a request body for non-POST requests.
+	 *     filter_path?: string, // A comma-separated list of filters used to reduce the response.


Note the code will actually also accept an array of sting elements: string|array<string>

@AJenbo these array fields are specificed by the Elasticsearch rest-api-specification, reported here. A list for this specification is represented as a comma separated list in the querystring, that is equivalent as a PHP string. That's why I used only string here.

I understand that, but the implementation in this repo will take a list of strings and turn it in to a comma separated string:

elasticsearch-php/src/Traits/EndpointTrait.php

Line 71 in 603d87e

// Convert to comma-separated list if array

Not documenting this effectively turns this functionality in to dead code and prevents us from taking advantage of the convenience.

@AJenbo unfortunately the convertValue() function that is able to translate an array into a comma separated value is used only for the query string parameter (e.g. filter_path). I need to adjust the usage of convertValue also for other parameters in the URL, e.g. here. Anyway, you right we should use this feature with string|array<string>. Thanks for you help!

AJenbo · 2025-03-22T17:57:09Z

src/Traits/MessageResponseTrait.php

@@ -29,6 +29,9 @@ public function getProtocolVersion(): string
        return $this->response->getProtocolVersion();
    }

+    /**
+     * @return MessageInterface


Nit: This PHPDoc is redundant since it's already expressed by the native return type

I agree that this is redundant but if I remove it I get the following error from PHPStan.

$ composer run-script phpstan > phpstan analyse --no-progress --memory-limit 256M Note: Using configuration file /.../Git/elasticsearch-php/phpstan.neon. ------ ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- Line Traits/MessageResponseTrait.php (in context of class Elastic\Elasticsearch\Response\Elasticsearch) ------ ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- :34 Method Elastic\Elasticsearch\Response\Elasticsearch::withProtocolVersion() should return static(Elastic\Elasticsearch\Response\Elasticsearch) but returns Psr\Http\Message\ResponseInterface. 🪪 return.type ------ -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

I'm trying to understand where is the issue here, any idea? Thanks.

This looks to be because Elasticsearch is actually violating the ResponseInterface interface by returning an instance of the underlying ResponseInterface rather then the object it's being called on.

https://github.com/php-fig/http-message/blob/402d35bcb92c70c026d1a6a9883f06b2ead23d71/src/MessageInterface.php#L39

By adding the PHPDoc to the trait it's overwriting the one from the ResponseInterface which is why it silences the error.

I have opened a PR to fix this properly: #1440

Thanks @AJenbo nice catch! I'll ask to wait the #1440 until we merge this PR: Moreover, can you rebase the PR to 8.17 branch? Thanks.

Done, I also set it to target the 8.17 branch so you can make it part of this PR.

AJenbo · 2025-03-22T18:12:29Z

src/Traits/ClientEndpointsTrait.php

+	 *     scroll?: int|string, // Specify how long a consistent view of the index should be maintained for scrolled search
+	 *     search_type?: string, // Search operation type
+	 *     search_timeout?: int|string, // Explicit timeout for each search request. Defaults to no timeout.
+	 *     max_docs?: float, // Maximum number of documents to process (default: all documents)


Consider if int|float should be the type for number as some of these do not appear to make sens as fractions. If it can accept a string representation then you could set it as numeric as that is short for int|float|numeric-string. If float was really the only intended input the source would probably have been double instead.

For the Elasticsearch specification reported here, a number is alias for double (moreover it's deprecated). I agree that makes no sense for max_docs that should be always an integer. I will double check with the Elasticsearch team. Thanks!

Ok, in that case it sounds like the issue isn't related to this PR but more the upstream documentation 👍

AJenbo · 2025-03-22T18:19:29Z

src/Traits/ClientEndpointsTrait.php

+	 *     wait_for_completion?: bool, // Should the request should block until the reindex is complete.
+	 *     requests_per_second?: float, // The throttle to set on this request in sub-requests per second. -1 means no throttle.
+	 *     scroll?: int|string, // Control how long to keep the search context alive
+	 *     slices?: float|number|string, // The number of slices this task should be divided into. Defaults to 1, meaning the task isn't sliced into subtasks. Can be set to `auto`.


This one still has the invalid number type as part of it's signature.

It's also valid to have the literal string in the PHPDoc type so the ideal type would be int|'auto'

AJenbo · 2025-03-22T18:21:23Z

src/Traits/ClientEndpointsTrait.php

+	 *     error_trace?: bool, // Include the stack trace of returned errors. (DEFAULT: false)
+	 *     source?: string, // The URL-encoded request definition. Useful for libraries that do not accept a request body for non-POST requests.
+	 *     filter_path?: string, // A comma-separated list of filters used to reduce the response.
+	 *     body: string|array, // (REQUIRED) The document. If body is a string must be a valid JSON.


To get closer to passing PHPStan level 6 you might want to hint this as either string|array<string, mixed> or string|array<mixed> (if lists are allowed)

AJenbo · 2025-03-22T18:35:47Z

src/Endpoints/Snapshot.php

+	 *     index_details?: bool, // Whether to include details of each index in the snapshot, if those details are available. Defaults to false.
+	 *     include_repository?: bool, // Whether to include the repository name in the snapshot info. Defaults to true.
+	 *     sort?: string, // Allows setting a sort order for the result. Defaults to start_time
+	 *     size?: integer, // Maximum number of snapshots to return. Defaults to 0 which means return all that match without limit.


Since you changed boolean to bool you might also want to change integer to int

AJenbo

Nice work. This change should improve things a lot.

I went over all the changes ad made comments where I think things can still be improved a bit in ways that would make it easier to work with the library and not get false positives.

ezimuel · 2025-03-23T10:39:57Z

Thanks @AJenbo for all your feedback and contribution. I'm finalizing this PR with all your feedback.

ezimuel · 2025-03-24T07:22:23Z

I'm closing this PR since it has been sent to main by mistake. I need to revert some changes and send it to 8.17.

Fix for PHPStan issues

5a03628

ezimuel requested a review from a team as a code owner March 22, 2025 09:46

This was referenced Mar 22, 2025

Invalid type given as required for multiple methods #1425

Open

Invalid PHPDocs - optional array keys #1424

Open

Invalid phpdoc (list instead of string) #1423

Open

Psalm 5.14.0 and higher triggers issues with defined array-shapes #1346

Open

AJenbo reviewed Mar 22, 2025

View reviewed changes

ezimuel closed this Mar 24, 2025

ezimuel mentioned this pull request Mar 25, 2025

Fix and improvements for PHPStan (rule level 5) #1442

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for PHPStan issues #1439

Fix for PHPStan issues #1439

ezimuel commented Mar 22, 2025

AJenbo Mar 22, 2025

ezimuel Mar 22, 2025

AJenbo Mar 22, 2025

ezimuel Mar 23, 2025

AJenbo Mar 22, 2025

ezimuel Mar 22, 2025

AJenbo Mar 22, 2025

ezimuel Mar 23, 2025

AJenbo Mar 23, 2025

AJenbo Mar 22, 2025 •

edited

Loading

ezimuel Mar 22, 2025

AJenbo Mar 22, 2025

AJenbo Mar 22, 2025 •

edited

Loading

AJenbo Mar 22, 2025

AJenbo Mar 22, 2025

AJenbo left a comment

ezimuel commented Mar 23, 2025

ezimuel commented Mar 24, 2025

Fix for PHPStan issues #1439

Fix for PHPStan issues #1439

Conversation

ezimuel commented Mar 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AJenbo Mar 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AJenbo Mar 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AJenbo left a comment

Choose a reason for hiding this comment

ezimuel commented Mar 23, 2025

ezimuel commented Mar 24, 2025

AJenbo Mar 22, 2025 •

edited

Loading

AJenbo Mar 22, 2025 •

edited

Loading