fix: apply presence rule to custom field validation on import [3.x] - #192
Merged
Merged
Conversation
ValidationService::getValidationRules() returned value-shape rules only — no required, no nullable. Filament form fields hide that gap because Field::getRequiredValidationRule() always prepends one. The importer calls Laravel's validator directly, so it shipped a rule set that was wrong in both directions. Filament's ImportColumn::castStateItem() turns every blank cell into null before Importer::validateData() runs, so a mapped-but-empty column reaches the validator as a present null. Without nullable, date/numeric/boolean/ file/regex all reject it: 11 of 24 field types failed the row for an optional field the user simply left blank. Conversely, required custom fields were never enforced at all — 12 types silently accepted an empty mapped cell, the rest only rejected it by accident via the type rule, with a misleading message. getValidationRules() now returns the presence rule followed by the value rules, so any caller handing a value straight to a validator is correct by default. The Filament form adapters, which supply presence themselves conditionally on field visibility, move to the new getValueValidationRules(). Form behaviour is unchanged. Covered by an invariant test that iterates every registered field type, so newly added types — including app-registered ones — are checked in both directions automatically.
There was a problem hiding this comment.
Pull request overview
This PR fixes custom field import validation so that presence (required vs nullable) is correctly enforced when validating mapped import columns, aligning import behavior with how Filament form fields handle required/optional fields.
Changes:
- Extend
ValidationServiceto make presence validation explicit and reusable viagetPresenceRule(), with a safe default ingetValidationRules(). - Introduce
getValueValidationRules()for callers (Filament form adapters) that already apply presence separately. - Add a comprehensive import-focused test that iterates over the registered custom field type registry to prevent regressions for newly added types.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/Feature/Imports/ImportColumnPresenceValidationTest.php | Adds coverage to ensure optional/required presence behaves correctly across all registered field types during imports. |
| src/Services/ValidationService.php | Makes presence (required/nullable) first-class and separates full rules vs value-only rules. |
| src/Filament/Integration/Concerns/Forms/ConfiguresValidation.php | Switches Filament form integration to use value-only rules since presence is applied via ->required(). |
| src/Filament/Integration/Base/AbstractFormComponent.php | Updates form component validation rule retrieval to use value-only rules and documents the presence-handling rationale. |
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.
Fixes optional custom fields failing import validation when the column is mapped but the cell is empty (Journey: 868kkcw4h).
Root cause
ValidationService::getValidationRules()returns value-shape rules only — norequired, nonullable. Filament form fields hide that gap becauseField::getRequiredValidationRule()always prepends one. The importer calls Laravel's validator directly, so it shipped a rule set that was wrong in both directions.Per row, Filament runs
Importer::castData()beforeImporter::validateData(), andImportColumn::castStateItem()turns every blank cell intonull. So a mapped-but-empty column reaches the validator as a presentnull, which Laravel considers validatable. Withoutnullable,date/numeric/boolean/file/regexall reject it.Reproduced
Against all 24 registered field types, optional field + mapped column + empty cell — 11 types rejected the row:
numberradioselecttoggle-buttonscurrencydatedate-timecheckboxtogglecolor-pickerfile-uploadThe same run with
validation_rules = ['required' => true]exposed the inverse bug: required custom fields were never enforced on import.text,textarea,rich-editor,markdown-editor,email,phone,link,multi-select,checkbox-list,tags-input,recordall accepted an empty mapped cell. The remaining types only rejected it by accident, via the type rule, with a misleading message.Fix
Make presence a first-class part of the
ValidationServicecontract, with the safe default:getValidationRules()— presence rule + value rules. The correct set for any caller handing a value straight to a Laravel validator (imports, APIs, jobs).getValueValidationRules()— value rules only, for callers that supply presence themselves.getPresenceRule()— single source of truth forrequiredvsnullable.ImportColumnConfiguratorneeds no change: it already callsgetValidationRules(), which is now complete. The two Filament form adapters move togetValueValidationRules()because they apply->required()separately and conditionally on field visibility — that conditionality is exactly why presence must not be baked into the form path.Patching the importer call site alone would have fixed this ticket in three lines and left the same trap for the next consumer. This makes the obvious call correct and requires the special case to opt out.
Future-proofing
The one-line fix isn't what stops this recurring.
tests/Feature/Imports/ImportColumnPresenceValidationTest.phpiteratesCustomFieldsType::toCollection()and asserts, per registered type:Because it enumerates the registry rather than a hardcoded list, any newly added field type — including app-registered ones — is covered in both directions automatically.
Behaviour change
Required custom fields are now actually enforced on import. Rows that previously imported with a required custom field left blank will now fail with a clear "field is required" message instead of silently landing incomplete.
Verification
pest --parallel— 778 passed, 0 failed (3 todos)phpstan analyse— no errorspint --dirty— passedrector --dry-run— cleanDateConstraintField.phpandFieldForm.php; not gated in CI)Known gaps, deliberately out of scope
Importer::getValidationRules()skips unmapped columns entirely, so a required custom field the user never mapped is never validated. Filament's answer isImportColumn::requiredMapping(). Left out because it would block existing import workflows at the mapping step — worth a separate, deliberate PR.file-uploadfields are unimportable with a filled cell.FileUploadFieldTypedeclaresdefaultValidationRules(['file']), and a CSV cell is always a string, so a filled cell fails with "must be a file" regardless of this change.nullableonly rescues the empty case.getDatabaseValidationRules()already special-casesFieldDataType::FILEfor the same reason; the import path should too.