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

Analyze removed support for request parameters #1370

Merged
merged 1 commit into from
Sep 5, 2017

Conversation

p365labs
Copy link
Collaborator

@p365labs p365labs commented Sep 2, 2017

Analyze endpoint removed support for request parameters, we should use request body instead. I updated the Analyze endpoint in order to accept not only text, but an array of values, so even the explain functionality will work.

@ruflin
Copy link
Owner

ruflin commented Sep 4, 2017

Guest what ... ^

* @param array $args OPTIONAL Additional arguments
*
* @return array Server response
*
* @link https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-analyze.html
*/
public function analyze($text, $args = [])
public function analyze($body, $args = [])
{
$endpoint = new Analyze();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should return an error in case body is not a map to make it easier for app devs to detect the problem early.

@p365labs p365labs force-pushed the es6_remove_explain_query_param branch 3 times, most recently from 60c0a34 to f7845ee Compare September 4, 2017 19:25
@p365labs
Copy link
Collaborator Author

p365labs commented Sep 4, 2017

sorry I make some mistakes between comments and review ... ;)

  • I updated the docblock as $body should be and array
  • I declared $body as array forcing the strict type
    what do u think ? should we really raise an error in case of $body not a map ?

@p365labs p365labs force-pushed the es6_remove_explain_query_param branch from f7845ee to 7e748b3 Compare September 4, 2017 19:44
@ruflin ruflin merged commit 41e8163 into ruflin:master Sep 5, 2017
@ruflin
Copy link
Owner

ruflin commented Sep 5, 2017

I like the strict typing of array. This will also show developers and error very quickly which hopefully pushes them to read the changelog :-)

@p365labs
Copy link
Collaborator Author

p365labs commented Sep 5, 2017

I like strict type hinting too... maybe when this major upgrade has ended we could plan a major refactor in order to make Elastica more respectful of it, considering we are now upgraded to php7.
Just an idea :)

@p365labs p365labs deleted the es6_remove_explain_query_param branch September 5, 2017 07:48
@ruflin
Copy link
Owner

ruflin commented Sep 5, 2017

I like the idea as it will make devs detect problems much earlier. But it will definitively break all older versions (which is probably fine looking at http://php.net/supported-versions.php).

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