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

allow for adding custom analyzers to mapping definition #2282

Open
wants to merge 1 commit into
base: 2.10.x
Choose a base branch
from

Conversation

gusdemayo
Copy link
Contributor

@gusdemayo gusdemayo commented Oct 8, 2021

Allow for adding analyzers to field mappings in following format

<customSearchAnalyzers>["custom_analyzer"]</customSearchAnalyzers>

return $this->config['custom_search_analyzers'];
}

return json_decode($this->config['custom_search_analyzers'], true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not a huge fan of having json_encoded data being set inside XML file, couldn't it be an XML array either ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

By the way, I think the xsd schema should be updated with this new config node probably.

/**
* {@inheritDoc}
*/
public function getCustomSearchAnalyzers()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need some unit testing here, this file is already covered so any new method should also be.

$weightedFields = array_merge($weightedFields, $this->addWeightedFields($analyzers, $canAddField, $field, $boost));
}

return $weightedFields;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need additional test cases in unit testing to deal with the fact that we can have custom_analyzers or not.

Actually the existing tests are not failing, which is good, but there is no clue about the expected result if the field is meant to have custom_analyzers

@romainruaud
Copy link
Collaborator

Hi @gusdemayo

this looks promising, but considering modifying this part is "high risk" (it can break mapping, change behavior of fulltext search, etc...), we need some more deep testing to ensure everything is safe, so we'll need some time internally to give it a try.

In the meantime, can you have a look to the requested changes I added in my review ?

Best regards

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.

4 participants