-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add to EP mapping properties instead of overwriting them #3
base: master
Are you sure you want to change the base?
Add to EP mapping properties instead of overwriting them #3
Conversation
The properties key of the mappings key in the mapping array already has values provided by the ElasticPress mapping. In the old situation, that entire key was overwritten, instead of adding the custom mappings to the properties.
@mvdhoek1 @SimonvanWijhe This PR was left open on the previous home of the repository. I've ported it over and provided reference links to track back the conversation. Can this still be reviewed, please? |
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.
As far as I can tell this looks good. Simon is more familiar with ElasticSearch, he will be returning to work this Monday.
When you're ready to merge don't forget the version bumps in src/OpenPub/Foundation/Plugin.php and openpub-base.php + changelog |
] | ||
$mapping_version = $this->getElasticPressMappingVersion(); | ||
|
||
if ($mapping_version == 5) { |
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.
Een extra =
if ($mappingVersion === 5) {
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.
Graag variabelen in camelCase voor consistentie met de rest van het project.
The properties key of the mappings key in the mapping array already has values provided by the ElasticPress mapping. In the old situation, that entire key was overwritten, instead of adding the custom mappings to the properties.
Original PR from Bitbucket (for reference): https://bitbucket.org/openwebconcept/plugin-openpub-base/pull-requests/20
Fixes issue with custom mapping that has been reported before (for reference, link to original repository reported issue): https://bitbucket.org/openwebconcept/plugin-openpub-base/issues/4/elasticsearch-custom-mapping