fix: clearing one custom field must not delete the others - #862
Merged
blaipr merged 1 commit intoAug 23, 2026
Merged
Conversation
The save path deleted a custom field's row when its posted value was empty, which is right — saving the blank instead would leave the old ciphertext behind. But the delete it made was `delete([$itemId], $moduleId)`, and that is keyed on the item and the module with no definition at all: it removes *every* custom field value on the item. So emptying one field on any item that has more than one wiped the rest of them in the same save, including encrypted values the person saving could not see and had not touched. Nothing reported it. The save succeeded and the other fields simply came back blank, which looks like the fields were never filled in rather than like data loss. deleteBatch() is keyed that way because it exists for the other job — deleting the item takes its fields with it — and the single-field case had been given the same method. This adds deleteForDefinition() through the repository, both ports and the service, and leaves deleteBatch() to the case it was written for. Also narrows updateCustomFieldsForItem()'s $itemId from `int|array` to `int`. Every caller passes a single id, and the array form was never workable here: it is written straight into the model's itemId, so an array would have been stored as one. The wider type came from the batch delete, where it does make sense. Checked by putting the old call back: the test that pins which row is deleted fails, and so does the one asserting the fields either side of a cleared one survive.
blaipr
deleted the
fix/clearing-one-custom-field-must-not-delete-the-others
branch
August 23, 2026 22:38
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.
The save path deleted a custom field's row when its posted value was empty, which is
right — saving the blank instead would leave the old ciphertext behind. But the delete
it made was
delete([$itemId], $moduleId), and that is keyed on the item and the modulewith no definition at all: it removes every custom field value on the item.
So emptying one field on any item that has more than one wiped the rest of them in the
same save, including encrypted values the person saving could not see and had not
touched. Nothing reported it. The save succeeded and the other fields simply came back
blank, which looks like the fields were never filled in rather than like data loss.
deleteBatch() is keyed that way because it exists for the other job — deleting the item
takes its fields with it — and the single-field case had been given the same method.
This adds deleteForDefinition() through the repository, both ports and the service, and
leaves deleteBatch() to the case it was written for.
Also narrows updateCustomFieldsForItem()'s $itemId from
int|arraytoint. Everycaller passes a single id, and the array form was never workable here: it is written
straight into the model's itemId, so an array would have been stored as one. The wider
type came from the batch delete, where it does make sense.
Checked by putting the old call back: the test that pins which row is deleted fails, and
so does the one asserting the fields either side of a cleared one survive.