fix(upgrade): report which files a step could not upgrade, and recover

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:

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<FlarumProviders> {
       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<FlarumProviders> {
           }
 
           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<FlarumProviders> {
           }
 
           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<FlarumProviders> {
       // }
     }
 
-    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<FlarumProviders> {
         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<FlarumProviders> {
 
       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<FlarumProviders> {
       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}`);
       }
     }
 

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);
+  }
+}

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<string> {

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.<anonymous> (/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');
+  });
+});