Map calculated value fields according to their configured element type - #488
Conversation
|
🚫 Issue-link guardrail failed — this PR has been converted to draft. Every PR must reference a tracking issue in
Add the link with a keyword in the PR description (not in a comment): https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests When fixed, press Ready for review to re-run the checks. |
There was a problem hiding this comment.
🟡 Changes recommended
Boolean string values such as 'false' are currently normalized to true.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds element-type-aware indexing for calculated values.
Changes:
- Adds boolean, numeric, date, and text mappings with normalization.
- Registers the dedicated adapter.
- Adds unit coverage for mappings and normalization.
File summaries
| File | Description |
|---|---|
CalculatedValueAdapter.php |
Implements mapping and normalization. |
field-definition-adapters.yml |
Registers the new adapter. |
CalculatedValueAdapterTest.php |
Tests mappings and normalized values. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| if ($value === null || is_bool($value)) { | ||
| return $value; | ||
| } | ||
|
|
||
| return (bool) $value; |
Calculated value fields were always mapped as text/keyword regardless of their configured element type. This made boolean, numeric and date calculated values unusable for term queries, aggregations and range filters in the search index. Add a dedicated CalculatedValueAdapter that maps the field based on the element type (boolean, numeric, date) and falls back to the existing text/keyword mapping for the text-based element types (input, textarea, html). Values are normalized accordingly, since calculators may return loosely typed values.
A plain bool cast would index the literal 'false' (and values such as 'off'/'no') as true, silently reversing filters. Use FILTER_VALIDATE_BOOLEAN with FILTER_NULL_ON_FAILURE so unrecognized values become null instead.
aae95d9 to
abd7cc9
Compare
|
Fixes pimcore/platform-version#293
Problem
Calculated value fields are always indexed with the generic text/keyword mapping, regardless of their configured element type (
calculatedValueis registered onTextKeywordAdapterinfield-definition-adapters.yml).For a
CalculatedValuefield withelementType: 'boolean'this creates atextmapping with ngram/keyword/sort subfields in the search index. Term queries, aggregations and boolean filters on the field then behave like text search, and the Studio data object grid boolean filter on such a field stops working.Reproduction
CalculatedValuefield configured withelementType: 'boolean'bin/console generic-data-index:update:indexGET <index>/_mapping/field/standard_fields.<fieldname>returnstext(with ngram/keyword/sort subfields) instead ofbooleanSolution
Add a dedicated
CalculatedValueAdapterthat maps the field according to its configured element type:booleanbooleannumericfloatdatedate(strict_date_time_no_millis)input/textarea/html(default)Values are normalized per element type, since calculators are free to return loosely typed values (e.g.
'0'/'1'strings from expression results, which a boolean mapping would reject). For the text-based element types the existingTextKeywordAdapternormalization (base64srcstripping) is preserved.Covered by unit tests (
CalculatedValueAdapterTest). Verified end-to-end on Elasticsearch 8.19: after this changegeneric-data-index:update:indexcreates the new index version with abooleanmapping and reindexing succeeds.