Refactoring mapping field building and merging with config files. #1055
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I also refactored the Unit tests for Mapping since it does not rely anymore on Dynamic Fields Provider : easier to test now. But the Indices Config part remains untested about DynamicFieldProviders. let me know |
afoucret
reviewed
Sep 4, 2018
| /** | ||
| * {@inheritDoc} | ||
| */ | ||
| public function getConfig() |
There was a problem hiding this comment.
Think it is a mistake to create a new getter for the config.
I would prefer a method mergeConfig(array $config) that would return a new instance of the field with updated config.
It is also possible to add a test on the mergeConfig method in FieldTest.
| } | ||
|
|
||
| foreach ($typeConfigData['mapping']['staticFields'] as $fieldName => $fieldConfig) { | ||
| if (isset($fields[$fieldName])) { |
There was a problem hiding this comment.
Use mergeConfig method instead of this one :
$field = $this->mappingFieldFactory->create(['name' => $fieldName] + $fieldConfig);
if (isset($fields[$fieldName]) {
$field = $fields[$fieldName]->mergeConfig($fieldConfig);
}
$fields[$fieldName] = $field;
romainruaud
force-pushed
the
feature_mapping-fields-refactoring
branch
2 times, most recently
from
7c5192e to
33994f7
Compare
romainruaud
force-pushed
the
feature_mapping-fields-refactoring
branch
from
d421ed5 to
68d2029
Compare
|
PR updated and rebased |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Several things here :
Mapping class refactored to allow only an array of fields in constructor.
Fields are built by the Indices Config prior to creating the mapping.
Fields that exist both in a "dynamic provider" and from the elasticsuite_indices.xml file are now properly merged.
I made the choice to give precedence to the file : only some fieldConfig is written to the file, meaning it will only overwrite some values of the dynamically computed Field.
For SKU, this allow to have the "defaultSearchAnalyzer" defined in the file, and to rely on the Back-Office for Search Weight of the attribute.
Fix #920