From 666519d96518047972033fc28e6378e7fca5e79f Mon Sep 17 00:00:00 2001 From: IanM Date: Sat, 1 Aug 2026 08:13:45 +0100 Subject: [PATCH] fix(upgrade): report which files a step could not upgrade, and recover A single unparseable file anywhere in an extension ended the whole upgrade run with a raw Babel stack trace and a dump of the file's contents, naming neither the file nor anything the author could act on. The step's partial writes were left on disk, so the uncommitted-changes guard then refused to let the command be run again. Each step now collects every file it could not handle, rolls back its own writes, and aborts with the file names and the reasons. Aborting keeps a step all-or-nothing: a half-applied transform leaves an extension in a state that is hard to reason about. Since the rollback leaves the tree clean and completed steps are already committed, re-running resumes from the step that stopped. Also removes a leftover debug console.log that printed whole files on any parse failure, and fixes a docs link left pointing at localhost. --- .../upgrade/twopointoh/backend/phpunit.ts | 2 +- src/steps/upgrade/twopointoh/base.ts | 73 +++++++++++++++---- src/steps/upgrade/twopointoh/step-failures.ts | 54 ++++++++++++++ src/utils/ast.ts | 11 +-- test/steps/upgrade/step-failures.test.ts | 68 +++++++++++++++++ 5 files changed, 185 insertions(+), 23 deletions(-) create mode 100644 src/steps/upgrade/twopointoh/step-failures.ts create mode 100644 test/steps/upgrade/step-failures.test.ts diff --git a/src/steps/upgrade/twopointoh/backend/phpunit.ts b/src/steps/upgrade/twopointoh/backend/phpunit.ts index ce99214..ff030a8 100644 --- a/src/steps/upgrade/twopointoh/backend/phpunit.ts +++ b/src/steps/upgrade/twopointoh/backend/phpunit.ts @@ -39,7 +39,7 @@ export default class PhpUnit extends BaseUpgradeStep { 'https://github.com/sebastianbergmann/phpunit/blob/11.3.0/DEPRECATIONS.md', ]; - const dbLink = 'http://localhost:3000/extend/testing#model-factories'; + const dbLink = 'https://docs.flarum.org/2.x/extend/testing#model-factories'; return `Flarum 2.0 uses PHPUnit 11. The tool has applied the most significant changes, but you might still run into other deprecations. Please refer to the following links for more information: diff --git a/src/steps/upgrade/twopointoh/base.ts b/src/steps/upgrade/twopointoh/base.ts index 464b129..ee16e2d 100644 --- a/src/steps/upgrade/twopointoh/base.ts +++ b/src/steps/upgrade/twopointoh/base.ts @@ -15,6 +15,7 @@ import s from 'string'; import simpleGit from 'simple-git'; import { cloneNode } from '@babel/types'; import { PhpProvider } from '../../../providers/php-provider'; +import { StepFailures } from './step-failures'; export type ReplacementResult = { imports?: ImportChange[]; @@ -127,6 +128,8 @@ export abstract class BaseUpgradeStep implements Step { dots = (dots + 1) % 4; }; + const failures = new StepFailures(); + const targets = this.targets(); for (const target of targets) { @@ -145,9 +148,16 @@ export abstract class BaseUpgradeStep implements Step { } const code = fsEditor.read(file); - const advanced = this.advancedContent(file, code); - this.before(file, code, advanced); + // Same as the transform pass below: a file this step can't read + // shouldn't stop it from reporting on the others. + try { + const advanced = this.advancedContent(file, code); + + this.before(file, code, advanced); + } catch (error) { + failures.record(s(file.replace(paths.package(), '')).stripLeft('/').stripLeft('\\').toString(), error); + } progress(); } @@ -163,12 +173,22 @@ export abstract class BaseUpgradeStep implements Step { } const code = fsEditor.read(file); - const advanced = this.advancedContent(file, code); - const relativeTarget = s(file.replace(paths.package(), '')).stripLeft('/').stripLeft('\\').toString(); - // eslint-disable-next-line no-await-in-loop - const result = await this.applyReplacements(relativeTarget, code, advanced); + let result; + + // One file the step can't handle shouldn't decide the fate of the + // rest: note it and carry on, so the author is told about every + // problem this step found rather than the first one. + try { + const advanced = this.advancedContent(file, code); + + // eslint-disable-next-line no-await-in-loop + result = await this.applyReplacements(relativeTarget, code, advanced); + } catch (error) { + failures.record(relativeTarget, error); + continue; + } if (result.newPath && result.newPath !== file) { fsEditor.move(file, result.newPath!); @@ -213,7 +233,30 @@ export abstract class BaseUpgradeStep implements Step { // } } - const changesMade = await simpleGit(paths.requestedDir() ?? paths.cwd()) + const dir = paths.requestedDir() ?? paths.cwd(); + + if (failures.any()) { + // Leave the step all-or-nothing. Its partial writes are already on disk, + // and the run refuses to start with a dirty tree, so keeping them would + // block the re-run this step is asking the author to do. + await simpleGit(dir).checkout(['--', '.']); + + this.command.log('\u001B[A => ' + chalk.bgRed.bold(' FAILED ')); + this.command.log(''); + this.command.log(failures.report()); + this.command.log(''); + + const files = failures.count() === 1 ? 'file' : 'files'; + + this.command.error( + `${failures.count()} ${files} could not be upgraded by this step, so it made no changes.\n` + + ' Fix the problems above and run the command again — steps that already\n' + + ' completed are skipped, so it will resume from here.', + { code: 'FL_ERR' } + ); + } + + const changesMade = await simpleGit(dir) .diffSummary() .then((summary) => summary.files.length > 0); @@ -265,8 +308,7 @@ export abstract class BaseUpgradeStep implements Step { try { advanced = parseCode(code); } catch (error) { - this.command.warn(`Failed to parse code for ${file}, code: \n${code}`); - throw error; + throw new Error(`The transformed code could not be parsed back: ${(error as Error).message}`); } result.imports?.forEach((imp) => { @@ -332,13 +374,17 @@ export abstract class BaseUpgradeStep implements Step { try { return JSON.parse(code); - } catch { - this.command.error(`Failed to parse JSON file ${file}`); + } catch (error) { + throw new Error(`Not valid JSON: ${(error as Error).message}`); } } if (['js', 'ts', 'jsx', 'tsx'].includes(lang || '')) { - return parseCode(code); + try { + return parseCode(code); + } catch (error) { + throw new Error(`Could not parse: ${(error as Error).message}`); + } } return null; @@ -365,8 +411,7 @@ export abstract class BaseUpgradeStep implements Step { try { return generateCode(updated as t.File, false); } catch (error) { - this.command.warn(`Failed to generate code for ${file}, code: \n${code}`); - throw error; + throw new Error(`Could not generate code from the transformed syntax tree: ${(error as Error).message}`); } } diff --git a/src/steps/upgrade/twopointoh/step-failures.ts b/src/steps/upgrade/twopointoh/step-failures.ts new file mode 100644 index 0000000..93ec5ed --- /dev/null +++ b/src/steps/upgrade/twopointoh/step-failures.ts @@ -0,0 +1,54 @@ +import chalk from 'chalk'; + +type Failure = { + file: string; + reason: string; +}; + +/** + * The files a single upgrade step could not transform. + * + * A step runs over every matching file in an extension, and any one of them + * can defeat it — a parser that doesn't recognise some syntax, a transformer + * meeting a shape it wasn't written for. Collecting those instead of throwing + * at the first means the author is told about all of them at once, and fixes + * them in one pass rather than rediscovering them one re-run at a time. + */ +export class StepFailures { + private failures: Failure[] = []; + + record(file: string, error: unknown): void { + this.failures.push({ file, reason: StepFailures.reasonFor(error) }); + } + + any(): boolean { + return this.failures.length > 0; + } + + count(): number { + return this.failures.length; + } + + /** + * The failures, formatted for the author: which file, and why. + */ + report(): string { + return this.failures.map(({ file, reason }) => ` ${chalk.bold(file)}\n ${reason}`).join('\n\n'); + } + + /** + * The message alone. A stack trace is about our internals, not about the + * code being upgraded, so it isn't what the author needs to see. + */ + private static reasonFor(error: unknown): string { + if (error instanceof Error) { + return error.message; + } + + if (typeof error === 'string') { + return error; + } + + return JSON.stringify(error); + } +} diff --git a/src/utils/ast.ts b/src/utils/ast.ts index 98cf618..12271d4 100644 --- a/src/utils/ast.ts +++ b/src/utils/ast.ts @@ -8,14 +8,9 @@ import prettierConfig from '@flarum/prettier-config/prettierrc.json'; import * as recast from 'recast'; export function parseCode(code: string): t.File { - try { - return recast.parse(code, { - parser: require('recast/parsers/babel-ts'), - }); - } catch (error) { - console.log(code); - throw error; - } + return recast.parse(code, { + parser: require('recast/parsers/babel-ts'), + }); } export async function generateCode(ast: t.File, extenders = false): Promise { diff --git a/test/steps/upgrade/step-failures.test.ts b/test/steps/upgrade/step-failures.test.ts new file mode 100644 index 0000000..fcd626c --- /dev/null +++ b/test/steps/upgrade/step-failures.test.ts @@ -0,0 +1,68 @@ +import { StepFailures } from '../../../src/steps/upgrade/twopointoh/step-failures'; + +describe('StepFailures', () => { + it('has nothing to report when no file failed', () => { + const failures = new StepFailures(); + + expect(failures.any()).toBe(false); + expect(failures.count()).toBe(0); + }); + + it('records the file and the reason it failed', () => { + const failures = new StepFailures(); + + failures.record('src/Api/ListThingController.php', new Error('Unexpected token at line 42')); + + expect(failures.any()).toBe(true); + expect(failures.count()).toBe(1); + + const report = failures.report(); + + expect(report).toContain('src/Api/ListThingController.php'); + expect(report).toContain('Unexpected token at line 42'); + }); + + it('keeps going after the first failure so one run surfaces them all', () => { + // The point of collecting rather than throwing on the first: an author + // fixes everything the step found in one pass, instead of rediscovering + // problems one re-run at a time. + const failures = new StepFailures(); + + failures.record('a.php', new Error('first problem')); + failures.record('b.php', new Error('second problem')); + failures.record('c.php', new Error('third problem')); + + expect(failures.count()).toBe(3); + + const report = failures.report(); + + expect(report).toContain('a.php'); + expect(report).toContain('b.php'); + expect(report).toContain('c.php'); + expect(report).toContain('third problem'); + }); + + it('reports a non-Error throw without losing what it was', () => { + const failures = new StepFailures(); + + // Transformers can reject with a string, or with a PHP subsystem payload. + failures.record('odd.php', 'a plain string failure'); + + expect(failures.report()).toContain('a plain string failure'); + }); + + it('reports the message rather than the whole stack', () => { + // Stacks belong behind a debug flag; the author needs the file and the + // reason, not our internals. + const error = new Error('the useful part'); + error.stack = 'Error: the useful part\n at Object. (/cli/src/internal.ts:1:1)'; + + const failures = new StepFailures(); + failures.record('x.php', error); + + const report = failures.report(); + + expect(report).toContain('the useful part'); + expect(report).not.toContain('/cli/src/internal.ts'); + }); +});