fix(imports): report every invalid cell instead of aborting the row or silently guessing - #206
Merged
Merged
Conversation
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 bug
Import casts were doing validation's job.
Filament runs
remapData → castData → validateData → fillRecord.Importer::castData()is a bare per-column loop with no error aggregation, so a cast has exactly two ways to signal a bad cell:ImportColumnConfiguratorused both. Choices and lookups threw. Toggle, checkbox, date, date-time, currency and number returned a guess.Neither is correct, and at that layer there is no third option.
What that looked like
A row with a valid select and garbage in a toggle, a currency field and a date:
Zero errors. Row succeeds. Three fields wrong.
And when a choice column did fail, it threw
RowImportFailedException, whichImportCsvcatches on a separate branch that can only ever carry one message. Every other bad cell in that row stayed hidden until the next upload.Dates were worse, because the input was valid
The same column flipped day-first and month-first based on whether the year had two digits or four. Not garbage input. Ordinary dates, silently transposed, invisible whenever both numbers are 12 or under.
The fix
A cast is now total and silent. Every input maps to some output and it never throws. A value it cannot honestly produce becomes an
UnresolvedValue, and abail-prefixedRejectsUnresolvedValuerule reports it.Because rejection now happens in
validateData(), Laravel's single validation pass names every bad column at once, merged with the host application's own column errors:One upload, every problem.
bailsits first so an unresolvable cell reports its own reason once, rather than also tripping the type rule behind it with a message describing the cast's fallback instead of the user's mistake.Dates and numbers are declared, not guessed
ImportDateFormat(iso,european,american) andImportNumberFormat(point,comma), both configurable undercustom-fields.imports.The date default is
iso. A column declared ISO means theY-m-dfamily and nothing else. Day-first, month-first and textual month forms belong to the convention that disambiguates them, so choosing one is a deliberate act rather than a silent guess. Every convention is a superset of ISO, so pickingeuropeanoramericanonly widens what is read, never changes how aY-m-dcell is interpreted.Parsing uses
createFromFormatwith an explicit format list and noCarbon::parsefallback. Overflow is rejected viaDateTimeImmutable::getLastErrors():Worth recording for anyone who hits this later: the
!format prefix does not prevent overflow (!Y-m-dstill turns2024-02-31into 2 March), andCarbon::hasFormat()does not detect it either (returnstruefor2024-02-31).getLastErrors()is the only reliable gate.Host field types cannot break the contract
configureViaFieldType()wraps every registeredimportTransformer, so a throw from a field type this package does not own becomes anUnresolvedValueinstead of killing the row. The wrapper also prefixes the field name when the transformer's message lacks it, because a field type does not know which field it is configuring andImportCsvflattens the error bag to message text, dropping attribute names.CurrencyFieldTypeis why this is a wrapper and not a convention. It shipped a transformer that ranpreg_replace('/[^0-9.-]/','')and turnedQ/Ainto0.0, and nothing caught it.Behavior change
Rows carrying values the importer previously swallowed now fail. That is the intent. The failure names the field, preserves the row in
failed_import_rows, and is fixed by correcting the cell. A recoverable error replaces silent wrong data.What does not change:
point, matching how this package has always behaved.blank -> nullguard.The one visible difference for choices is the exception class reaching
ImportCsv:ValidationExceptionrather thanRowImportFailedException. Both are caught there. Code catchingRowImportFailedExceptionaround its own import loop should widen it.Second behavior change: the date default
This package previously read dates through a day-first regex with a
Carbon::parsefallback, so15/01/2024parsed and3/4/2024meant 3 April. Under theisodefault neither is accepted, and the cell is reported rather than guessed at.If your users are not on
Y-m-d, set the convention explicitly:or
CUSTOM_FIELDS_IMPORT_DATE_FORMAT=european.That is the upgrade step for this release. It is one line, and it replaces an ambiguity that silently transposed day and month on every two-digit-year cell.
Tests
ImportValidationContractTestruns a realImporterover real rows through a fixture importer wired the way the docs tell people to wire one, and asserts onfailed_import_rowsand stored values rather than on cast internals. It covers multi-error reporting, mixed custom-field and ordinary column errors, message quality for the single-error case, each silent-swallow class, a clean row storing every value, blank optional cells still importing, partial failure leaving good rows imported, and the failed row being preserved for re-upload.ImportContractConformanceTestiterates every registered field type and asserts the contract holds: never throws, never silently coerces, always names the field, blank still becomes null. That is what stops the next field type reintroducing this, which is how the currency transformer got here.Two assertions in
ImportArchitectureTestchanged, both of which encoded the bug:'invalid-date'becomingnull, and an invalid option throwing. Every other assertion in that file passes untouched, including'15/01/2024' -> '2024-01-15'and'January 15, 2024' -> '2024-01-15', which is the evidence that existing behavior is preserved.Full suite: 906 passed. PHPStan clean. Pint and Rector clean. Type coverage is unchanged at 99.4%, which is the pre-existing value on
3.x.