Allow patching a multiselect to empty - #2020
Open
xIrusux wants to merge 2 commits into
Open
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 multiselect PATCH operations to clear fields by accepting null replacement values.
Changes:
- Widens the patch handler return type to
?array. - Adds regression tests for null, populated, and invalid multiselect 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 |
Supports null replacement values. |
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.
lukmzig
approved these changes
Aug 31, 2026
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.
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