Allow patching a multiselect to empty - #2019
Closed
xIrusux wants to merge 2 commits into
Closed
Conversation
A replace patch with data: null ("clear the field") threw a TypeError
because handlePatch declared a non-nullable array return while
returning the payload data verbatim. The caller already supports null:
PatchService passes the adapter result straight into setValue(), where
null is the normal way to clear a field.
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
|
Contributor
There was a problem hiding this comment.
Pull request overview
Allows merge PATCH operations to clear multiselect fields without causing a TypeError.
Changes:
- Makes
MultiSelectAdapter::handlePatch()return nullable arrays. - Adds regression tests for null, populated, and invalid values.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/DataObject/Data/Adapter/MultiSelectAdapter.php |
Allows replace patches to return null. |
tests/Unit/DataObject/Data/Adapter/MultiSelectAdapterTest.php |
Covers multiselect PATCH behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Author
|
Superseded by the 2025.4-based PR — the fix belongs on the lowest affected release line and forward-merges up. |
Contributor
Author
|
Superseded by #2020. |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Because clearing a multiselect through a merge is a legal operation the core adapter's type signature forbade.
What the merge sent: object 333's
coloris empty; you chose to pull that side over, so the merger savedcolor: { action: 'replace', data: null }— "replace the target's value with nothing", i.e. clear the field. That's the correct payload for this merge, not a frontend bug.Where it exploded:
MultiSelectAdapter::handlePatchhandlesreplaceby returning the payload'sdataverbatim — but its return type was declaredarray, sodata: nullthrows aTypeErrorand the whole PATCH 500s (MultiSelectAdapter.php:66).Why widening to
?arrayis the right fix, not a workaround: the caller already supports null —PatchService::patchEditableDatapasses the adapter's return straight into$element->setValue($key, $value)with no null check, andsetValue(key, null)is Pimcore's normal "clear this field". The add/remove paths in the same adapter also cope with null existing values. Only thereplacepath's signature was narrower than its own contract. One character (?) restores it.Verified
MultiSelectAdapterTest, following theNumericRangeAdapterTestpattern): replace withdata: nullreturns null, replace with values returns them, non-array input returns null. 3/3 green locally viavendor/bin/codecept run Unit.🤖 Generated with Claude Code