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
[NodeSearchBundle] Elastic search version 6 support #1875
Conversation
treeleaf
commented
Mar 20, 2018
•
edited
edited
Q | A |
---|---|
Bug fix? | no |
New feature? | yes |
BC breaks? | no |
Deprecations? | no |
Fixed tickets | #1799 |
d279525
to
4d896d9
Compare
$properties->children()->scalarNode('type')->beforeNormalization()->ifNotInArray($types)->thenInvalid('type must be one of: ' . implode(', ', $types)); | ||
|
||
if (ElasticSearchUtil::useVersion6()) { | ||
$properties->children()->booleanNode('fielddata'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was reading the https://www.elastic.co/guide/en/elasticsearch/reference/current/fielddata.html article and like they say, it's not a good idea to use this fielddata element because it will cause a lot of memory issues. We should use https://www.elastic.co/guide/en/elasticsearch/reference/current/doc-values.html.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, using doc_values seems a good alternative, however, I don't think we should prohibit it. We are not using fielddata, but allowing that it is used as a mapping value. I'll add the doc_values as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, super
*/ | ||
public static function useVersion6() | ||
{ | ||
return (PHP_VERSION[0] == 7 && !class_exists('\Elastica\Tool\CrossIndex')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PHP_MAJOR_VERSION
|
||
## Dynamic configuration | ||
We have added dynamic support to detect which version your project is using. | ||
Dependancies for using the configuration of version 6 is that your project uses PHP 7, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo, can you check the document again for typo's ? 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the doc. and removed the typo :)