fix: only save custom fields the imported row carried [3.x] - #193
Merged
Conversation
ImporterBuilder::saveValues() delegated to saveCustomFields(), which iterates every custom field on the entity and writes null for any absent from its payload. That is right for a form — it renders all fields, so absent means the user cleared it — and destructive for an import, where a CSV legitimately maps a subset of columns and absent means "not in this file". An update import that mapped one custom field silently wiped every other custom field on the record. The blast radius was masked because saveValues() returns early on an empty payload, and callers that strip custom field keys before Filament's fillRecord() never populate the payload at all. saveValues() now writes only the codes the row actually carried, leaving the rest untouched. HasCustomFields::saveCustomFieldValue() was also missing the $tenant parameter its implementation has accepted since multi-tenancy landed; the contract now matches, so implementers can't drop tenant scoping by following the interface.
There was a problem hiding this comment.
Pull request overview
This PR fixes destructive behavior in the import pipeline where updating a record with a subset of mapped custom-field columns could unintentionally wipe all other custom-field values, and aligns the HasCustomFields contract with the multi-tenancy-aware implementation.
Changes:
- Update
ImporterBuilder::saveValues()to persist only custom field codes actually present in the imported row payload. - Fix the
HasCustomFields::saveCustomFieldValue()contract to accept an optional tenant model parameter. - Add feature tests covering: saving carried fields, leaving uncarried fields untouched, and no-op behavior when no custom-field columns were carried.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/Feature/Imports/ImporterBuilderSaveValuesTest.php | Adds regression tests to prevent imports from wiping uncarried custom fields. |
| src/Models/Contracts/HasCustomFields.php | Updates interface signature to include optional tenant parameter for tenant-scoped value saving. |
| src/Filament/Integration/Builders/ImporterBuilder.php | Changes save logic to write only row-carried custom field codes instead of nulling absent ones. |
Comment on lines
+126
to
+130
| ->each(fn (CustomField $field) => $this->model->saveCustomFieldValue( | ||
| $field, | ||
| $customFieldsData[$field->code], | ||
| $tenant, | ||
| )); |
The distinction saveValues() draws is between a column the row carried and one it did not, not between a value that is filled and one that is blank. A mapped column left blank in the CSV reaches storage as an explicit null and must still clear the stored value; only an absent key is left alone.
Collection::each() breaks out of the loop when its callback returns false, and an arrow function implicitly returns whatever it evaluates. saveCustomFieldValue() returns null today so nothing breaks, but the arrow function couples this loop's completion to that return value: if the method ever returned false, the remaining custom fields would be skipped silently, which is exactly the failure this method exists to prevent. Matches the block closure UsesCustomFields::saveCustomFields() already uses.
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.
Found while business-reviewing #192 end-to-end against a real consumer app.
The bug
ImporterBuilder::saveValues()delegated tosaveCustomFields():That semantic is correct for a form — it renders every field, so a field absent from the submitted state means the user cleared it. It is destructive for an import, where a CSV legitimately maps a subset of columns and absent means "not in this file".
Result: an update import that mapped one custom field silently wiped every other custom field on the record.
Why it hasn't bitten yet
saveValues()returns early whenImportDataStorage::pull()is empty. Consumers that stripcustom_fields_*keys from$this->databefore Filament'sImporter::fillRecord()runs never populate the storage at all, sosaveValues()no-ops and nothing is written — or wiped. In the app I reviewed, that describes all 11 importers: custom field values were never persisted, which also hid this.Fix that consumer-side ordering and the wipe becomes live immediately. So this is latent today and load-bearing the moment anyone gets the ordering right.
Fix
saveValues()now writes only the codes the row actually carried:Also fixes
HasCustomFields::saveCustomFieldValue(), which was missing the?Model $tenant = nullparameter itsUsesCustomFieldsimplementation has accepted since multi-tenancy landed. PHPStan caught it as soon as the interface was called with three arguments. Any implementer following the contract would silently drop tenant scoping.Verification
tests/Feature/Imports/ImporterBuilderSaveValuesTest.php— three cases: carried fields are saved, uncarried fields are untouched, empty payload is a no-op. Confirmed the "untouched" case fails without the fix (nullinstead of'Alex Rivera'), so it isn't vacuous.pest --parallel— 764 passed, 0 failedphpstan analyse— no errorspint --dirty,rector --dry-run— cleanRelationship to #192
Independent, both in the import path, both should land in the same release. #192 stops optional custom fields from failing validation on a mapped-but-empty column; this stops a subset-column import from wiping data once values actually reach the save path.