[BUGFIX] Report unprocessable files instead of aborting or truncating them - #437
Open
chrissonntag wants to merge 2 commits into
Open
[BUGFIX] Report unprocessable files instead of aborting or truncating them#437chrissonntag wants to merge 2 commits into
chrissonntag wants to merge 2 commits into
Conversation
A processor that throws takes the whole run down with it: the remaining files are never looked at, and the message names neither the file nor the processor that failed. A single unreadable file in a large code base is enough to make Fractor unusable until it is found by hand. - Wrap each processor call in a try/catch and collect the failure as a SystemError naming the file, the processor and the reason - Skip the failed file before it receives a diff, so a document that was already half-processed can never reach the disk - Report the errors in the console and JSON output formatters, and skip the success message when a run produced errors - Return ExitCode::FAILURE when at least one file could not be processed, so an unprocessable file does not pass silently in CI The value object, the accessor and the return code follow Rector, whose SystemError this mirrors.
Both XML processors called DOMDocument::loadXML() without checking its result. An unparsable file left an empty document behind, which behaves differently depending on the processor and is destructive in one case: - An .xml file was serialized from that empty document and written back, truncating it to a bare XML declaration. The report announced the file as "reformatted", so the loss was silent. - An .xlf file reached the version detection instead, where an assertion aborted the whole run with "XLIFF document has no root element" and no file name. Both processors now obtain their document from a factory that reports what libxml found and never returns an unloaded document: - Collect libxml diagnostics internally instead of letting them surface as raw PHP warnings, and restore the previous state afterwards - Reject only a document that failed to load: warnings such as an invalid xml:space value still leave a usable document and are not an error - Reject an empty string up front, where loadXML() raises a ValueError rather than reporting a parse error Fixture tests cover both processors and assert that an unparsable file is reported and stays byte for byte the same.
Contributor
Author
|
The rector tests have already failed before the changes. Most likely due to new rules that came with an update. I suggest to ignore those for now and address them later on. |
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
1 participant
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 two related failure modes reported in #435 and #436.
Unparsable XML (
be6ead2) — both processors calledloadXML()without checking the result. For.xmlthe empty document was written back, silently truncating the file to a bare XML declaration while reporting it as "reformatted". For.xlfan assertion aborted the whole run without naming the file. Both now get their document from a factory that reports what libxml found. Warnings such as the invalidxml:spacevalue from #436 still leave a usable document and are not treated as errors.Failure isolation (
0fbd6dd) — a throwing processor took the entire run with it. Each processor call is now wrapped, the failure is collected as aSystemErrornaming the file and processor, and the run continues. Modelled on Rector'sSystemError/getSystemErrors()/ExitCode::FAILURE, including returning a non-zero exit code so an unprocessable file can't pass silently in CI.Fixture tests for both processors assert the file is reported and stays byte for byte identical. Verified the first commit stands on its own (283/283) so the history stays bisectable.
Closes #436
Refs #435 — the crash there is a bug in
simonschaufi/pretty-xml(simonschaufi/pretty-xml#16). This change makes it a named, skipped file rather than a fatal; the actual formatter fix needs that release plus a constraint bump here.Note for #430: this touches
FractorRunnerandProcessResulttoo. If this lands first, that branch needs a rebase — the hunks don't overlap.