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

Fix reading bool index setting & deprecate ElasticsearchException #1251

Merged
merged 5 commits into from
Feb 20, 2017
Merged

Fix reading bool index setting & deprecate ElasticsearchException #1251

merged 5 commits into from
Feb 20, 2017

Conversation

Tobion
Copy link
Collaborator

@Tobion Tobion commented Feb 16, 2017

  • Use the explicit getter Response::getStatus instead of the curl transfer info
  • Fix reading bool index setting, e.g. when it uses 'off' instead of '0' (where just casting to bool doesn't work)
  • Deprecate ElasticsearchException which is irrelevant since elasticsearch now exposes the errors as a structured array instead of a single string. So the whole purpose of the class is gone.
  • Add test to show Index Exists will return false if an index exists but is marked as read-only. #457 is solved by now

@@ -133,13 +126,7 @@ public function getFullError()
*/
public function getErrorMessage()
{
$error = $this->getError();

if (!is_string($error)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

getError always returns string. So this is useless

Copy link
Owner

Choose a reason for hiding this comment

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

Not sure if we introduced this to make master "partially" compatible with 2.x. Happy to remove it now and lets readd it in case things break ...

*
* @var bool Error
*/
protected $_error = false;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Property that doesn't even have a getter and is not really used

@@ -185,13 +194,12 @@ public function setBlocksWrite($state = true)
*/
public function getBlocksMetadata()
{
// TODO will have to be replace by block.metadata.write once https://github.com/elasticsearch/elasticsearch/pull/9203 has been fixed
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This has been merged and makes it possible to use the correct read_only setting again which was changed in #738

* @param int $code Error code
* @param array $error Error object from elasticsearch
* @param int $code Error code
* @param string $error Error message from elasticsearch
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -103,6 +103,25 @@ public function get($setting = '')
}

/**
* Returns a setting interpreted as a bool.
Copy link
Owner

Choose a reason for hiding this comment

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

Nice addition. I hope that ES will also clean this up in the future and only have 1 option of returning bools.

@@ -133,13 +126,7 @@ public function getFullError()
*/
public function getErrorMessage()
{
$error = $this->getError();

if (!is_string($error)) {
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure if we introduced this to make master "partially" compatible with 2.x. Happy to remove it now and lets readd it in case things break ...

@ruflin ruflin merged commit cad28fd into ruflin:master Feb 20, 2017
@Tobion Tobion deleted the fix-error-handling branch February 20, 2017 13:49
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