Fix falsy value check dropping 0/false in field collection sub-fields - #2026
Open
jcPimcore wants to merge 3 commits into
Open
Fix falsy value check dropping 0/false in field collection sub-fields#2026jcPimcore wants to merge 3 commits into
jcPimcore wants to merge 3 commits into
Conversation
FieldCollectionsAdapter::processCollectionRaw() used !$elementValue to decide whether a field-collection sub-field had a value, which is true for 0, false, "0" and "" in PHP. This silently dropped legitimate falsy values (e.g. a numeric field's default value of 0) before they ever reached the field's setter adapter. Refs PEES-1279 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Covers processCollectionRaw() keeping 0, false and "" values instead of dropping them, while still skipping a genuinely absent field. Verified this test fails against the pre-fix code and passes against the fix. Refs PEES-1279 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Present null values must reach the setter adapter so fields can be explicitly cleared.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes field-collection handling of submitted falsy values.
Changes:
- Preserves
0,false, and empty strings. - Adds regression tests for falsy and missing values.
File summaries
| File | Description |
|---|---|
tests/Unit/DataObject/Data/Adapter/FieldCollectionsAdapterTest.php |
Adds regression coverage for falsy and missing values. |
src/DataObject/Data/Adapter/FieldCollectionsAdapter.php |
Preserves falsy values, but still treats an explicit null like an omitted key. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
jcPimcore
force-pushed
the
fix/PEES-1279-field-collection-default-value
branch
from
September 3, 2026 14:06
aec33b0 to
e8ff1c1
Compare
processCollectionRaw() still conflated an omitted key with a key explicitly submitted as null, since both produced $elementValue === null. The setter-adapter contract allows null to clear a field on non-patch saves, so a field-collection sub-field could still never be cleared. Check key presence with array_key_exists(), matching the existing pattern in ObjectBricksAdapter::getCollectionData(), and pass a present null through to the adapter. Adds regression tests for both the explicit-null-passthrough case and a collection item with no 'data' key at all. Ran the full tests/Unit/DataObject/ suite locally (43 tests, 93 assertions) with no regressions. Addresses Copilot review feedback on PR #2026. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
Good catch — fixed in ebfc3e8:
🤖 Generated with Claude Code |
|
This file contains hidden or 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
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.
Summary
FieldCollectionsAdapter::processCollectionRaw()used!$elementValueto decide whether a field-collection sub-field had a submitted value.truefor0,false,"","0"in PHP, so any such legitimate value submitted for a field-collection sub-field (including a pre-filled numeric default of0) was silently dropped before it ever reached the field's own setter adapter.$elementValue === null, so a genuinely absent field is still skipped, but falsy-but-real values are kept.Test plan
tests/Unit/DataObject/Data/Adapter/FieldCollectionsAdapterTest.php, callingprocessCollectionRaw()via reflection with a realFieldcollection\Definitionregistered inRuntimeCache.Refs PEES-1279
Resolves https://github.com/pimcore/service-operations/issues/953
🤖 Generated with Claude Code