From 536759eac41f1c6800740ae74a0349f14c534a04 Mon Sep 17 00:00:00 2001 From: IanM Date: Sun, 11 Oct 2026 14:00:42 +0100 Subject: [PATCH] fix(upgrade): check for other extensions in flarum.extensions 1.x code checked for another extension with app.initializers.has(name). An initializer is not always named after its extension, and 2.0 renamed three of core's, so fof/username-request's check for 'flarum/nicknames', for one, is always false in 2.0. The misc frontend step was meant to rename those three, but never ran: it looked for this.currentTag after an earlier change had replaced it. - app.initializers.has('name') becomes 'id' in flarum.extensions where the ID is certain: a bundled extension's, or one composer.json names. Other checks get a @TODO. - A replacement that changes the code as text no longer loses its change when a later one in the same step returns the syntax tree it was given. --- src/steps/upgrade/twopointoh/base.ts | 6 + .../twopointoh/frontend/admin-extender.ts | 6 +- src/steps/upgrade/twopointoh/frontend/misc.ts | 156 +++++++++++++++- src/utils/composer.ts | 10 ++ test/steps/upgrade/initializer-checks.test.ts | 167 ++++++++++++++++++ 5 files changed, 336 insertions(+), 9 deletions(-) create mode 100644 test/steps/upgrade/initializer-checks.test.ts diff --git a/src/steps/upgrade/twopointoh/base.ts b/src/steps/upgrade/twopointoh/base.ts index 7e788542..49edd0fc 100644 --- a/src/steps/upgrade/twopointoh/base.ts +++ b/src/steps/upgrade/twopointoh/base.ts @@ -304,6 +304,12 @@ export abstract class BaseUpgradeStep implements Step { // eslint-disable-next-line no-await-in-loop code = await this.updateCode(file, code, result.updated); + // Later replacements read the parsed code: parse it again after a text + // change, or one that returns a syntax tree would undo this change. + if (typeof result.updated === 'string') { + advanced = this.advancedContent(file, code); + } + if (result.imports?.length || (result.updated && typeof result.updated !== 'string' && 'program' in result.updated)) { try { advanced = parseCode(code); diff --git a/src/steps/upgrade/twopointoh/frontend/admin-extender.ts b/src/steps/upgrade/twopointoh/frontend/admin-extender.ts index 152e367a..5e34a25c 100644 --- a/src/steps/upgrade/twopointoh/frontend/admin-extender.ts +++ b/src/steps/upgrade/twopointoh/frontend/admin-extender.ts @@ -7,6 +7,7 @@ import * as t from '@babel/types'; import { parse } from 'recast/parsers/babel-ts'; import { BaseUpgradeStep, GitCommit, Replacement } from '../base'; import { formatCode } from '../../../../utils/ast'; +import { extensionId } from '../../../../utils/composer'; const TODO = '@TODO: register this with Extend.Admin in admin/extend instead'; const DOCS = 'https://docs.flarum.org/2.x/extend/admin'; @@ -462,10 +463,7 @@ export default class AdminExtender extends BaseUpgradeStep { /** The extension's ID, as core makes it from the package name. */ protected extensionId(): string | null { try { - const name: string = JSON.parse(fs.readFileSync(path.join(this.root, 'composer.json'), 'utf8')).name; - const [vendor, pkg] = name.includes('/') ? name.split('/', 2) : [name, name]; - - return `${vendor}-${pkg.replace(/flarum-ext-/g, '').replace(/flarum-/g, '')}`; + return extensionId(JSON.parse(fs.readFileSync(path.join(this.root, 'composer.json'), 'utf8')).name); } catch { return null; } diff --git a/src/steps/upgrade/twopointoh/frontend/misc.ts b/src/steps/upgrade/twopointoh/frontend/misc.ts index ec40a450..8e322283 100644 --- a/src/steps/upgrade/twopointoh/frontend/misc.ts +++ b/src/steps/upgrade/twopointoh/frontend/misc.ts @@ -2,7 +2,48 @@ import { BaseUpgradeStep, GitCommit, ImportChange, Replacement } from '../base'; import chalk from 'chalk'; import * as t from '@babel/types'; import traverse from '@babel/traverse'; +import fs from 'fs'; +import path from 'path'; +import { parse } from 'recast/parsers/babel-ts'; import { getFunctionName } from '../../../../utils/ast'; +import { extensionId } from '../../../../utils/composer'; + +/** The bundled extensions' IDs, which are their initializers' names in 2.0. */ +const BUNDLED = new Set([ + 'flarum-akismet', + 'flarum-approval', + 'flarum-audit', + 'flarum-bbcode', + 'flarum-deck', + 'flarum-embed', + 'flarum-emoji', + 'flarum-extension-manager', + 'flarum-flags', + 'flarum-gdpr', + 'flarum-lang-english', + 'flarum-likes', + 'flarum-lock', + 'flarum-markdown', + 'flarum-mentions', + 'flarum-messages', + 'flarum-nicknames', + 'flarum-pusher', + 'flarum-realtime', + 'flarum-statistics', + 'flarum-sticky', + 'flarum-subscriptions', + 'flarum-suspend', + 'flarum-tags', +]); + +/** The 1.x names of the bundled initializers 2.0 renamed (flarum/framework#4003). */ +const RENAMED: Record = { + lock: 'flarum-lock', + subscriptions: 'flarum-subscriptions', + 'flarum/nicknames': 'flarum-nicknames', +}; + +const CHECK_TODO = "@TODO: check whether the extension is enabled with '' in flarum.extensions instead"; export default class MiscFrontendChanges extends BaseUpgradeStep { type = 'Miscellaneous frontend changes.'; @@ -387,19 +428,124 @@ export default class MiscFrontendChanges extends BaseUpgradeStep { }; } + /** + * `app.initializers.has(name)` becomes `'id' in flarum.extensions`, where the + * upgrade knows the ID for certain: a bundled extension's, or one the + * extension's composer.json names. An initializer is not always named after + * its extension, so a check it can't be sure of gets a @TODO instead. + */ private updateInitializers(): Replacement { return (_file, code) => { - if (!code.includes('this.currentTag')) return null; + if (!/initializers\s*\.\s*has\s*\(/.test(code)) return null; + + const edits: Array<{ start: number; end: number; text: string }> = []; + const flagged = new Set(); + + traverse(parse(code) as t.File, { + CallExpression: (p) => { + const id = this.checkedExtension(p.node); + + if (id === undefined) return; + + if (id) { + edits.push({ start: p.node.start!, end: p.node.end!, text: `('${id}' in flarum.extensions)` }); + + return; + } + + // Above the statement, where a comment can't land in JSX or a string. + const statement = p.getStatementParent(); + + if (!statement || statement.node.leadingComments?.some((comment) => comment.value.includes(CHECK_TODO))) return; + + const lineStart = code.lastIndexOf('\n', statement.node.start! - 1) + 1; + + if (flagged.has(lineStart)) return; + + flagged.add(lineStart); + + const indent = /^[\t ]*/.exec(code.slice(lineStart))![0]; + const docs = 'https://docs.flarum.org/2.x/extend/extending-extensions'; + + edits.push({ + start: lineStart, + end: lineStart, + text: `${indent}// ${CHECK_TODO} (${docs}). An initializer is not always named after its extension, and some were renamed in 2.0.\n`, + }); + }, + }); + + if (edits.length === 0) return null; return { - updated: code - .replace(/initializers\.has\(["']lock["']\)/g, "initializers.has('flarum-lock')") - .replace(/initializers\.has\(["']subscriptions["']\)/g, "initializers.has('flarum-subscriptions')") - .replace(/initializers\.has\(["']flarum\/nicknames["']\)/g, "initializers.add('flarum-nicknames')"), + updated: edits + .sort((a, b) => b.start - a.start) + .reduce((result, edit) => result.slice(0, edit.start) + edit.text + result.slice(edit.end), code), }; }; } + /** + * For `app.initializers.has('name')`: the ID of the extension it checks for, + * or null when the upgrade can't be sure of it. Undefined for anything else. + */ + private checkedExtension(call: t.CallExpression): string | null | undefined { + const callee = call.callee; + + if (!t.isMemberExpression(callee) || callee.computed || !t.isIdentifier(callee.property, { name: 'has' })) return undefined; + + const initializers = callee.object; + + if (!t.isMemberExpression(initializers) || initializers.computed || !t.isIdentifier(initializers.property, { name: 'initializers' })) { + return undefined; + } + + const owner = initializers.object; + + if (!t.isIdentifier(owner, { name: 'app' }) && !(t.isMemberExpression(owner) && t.isIdentifier(owner.property, { name: 'app' }))) { + return undefined; + } + + const [name] = call.arguments; + + if (call.arguments.length !== 1 || !t.isStringLiteral(name)) return undefined; + + return RENAMED[name.value] ?? (BUNDLED.has(name.value) ? name.value : this.namedExtensions().get(name.value) ?? null); + } + + private named: Map | null = null; + + /** + * The extensions the composer.json names, by package name and by ID. + */ + private namedExtensions(): Map { + if (this.named) return this.named; + + this.named = new Map(); + + let composer: Record = {}; + + try { + composer = JSON.parse(fs.readFileSync(path.join(this.root, 'composer.json'), 'utf8')); + } catch { + return this.named; + } + + const packages = [ + ...Object.keys(composer.require ?? {}), + ...Object.keys(composer['require-dev'] ?? {}), + ...Object.keys(composer.suggest ?? {}), + ...(composer.extra?.['flarum-extension']?.['optional-dependencies'] ?? []), + ].filter((name: unknown): name is string => typeof name === 'string' && name.includes('/')); + + for (const name of packages) { + this.named.set(name, extensionId(name)); + this.named.set(extensionId(name), extensionId(name)); + } + + return this.named; + } + private updateUploadImageButton(): Replacement { return (file, code) => { if (!code.includes('UploadImageButton')) return null; diff --git a/src/utils/composer.ts b/src/utils/composer.ts index 4eea539a..2b922eda 100644 --- a/src/utils/composer.ts +++ b/src/utils/composer.ts @@ -70,3 +70,13 @@ export type ComposerJsonSchema = { export function getComposerJson(fs: Store, paths: Paths): ComposerJsonSchema { return create(fs).readJSON(paths.package('composer.json')) as ComposerJsonSchema; } + +/** + * An extension's ID, from its package name, as core makes it: `vendor-package`, + * without a `flarum-ext-` or `flarum-` in the package's part. + */ +export function extensionId(packageName: string): string { + const [vendor, pkg] = packageName.includes('/') ? packageName.split('/', 2) : [packageName, packageName]; + + return `${vendor}-${pkg.replace(/flarum-ext-/g, '').replace(/flarum-/g, '')}`; +} diff --git a/test/steps/upgrade/initializer-checks.test.ts b/test/steps/upgrade/initializer-checks.test.ts new file mode 100644 index 00000000..129c10b4 --- /dev/null +++ b/test/steps/upgrade/initializer-checks.test.ts @@ -0,0 +1,167 @@ +import fs from 'fs'; +import os from 'os'; +import path from 'path'; +import MiscFrontendChanges from '../../../src/steps/upgrade/twopointoh/frontend/misc'; +import { formatCode } from '../../../src/utils/ast'; + +/** + * In 1.x, code checked whether another extension was enabled with + * app.initializers.has(name). Initializer names are not always the extension's + * ID, and some of core's changed in 2.0, so the upgrade checks the extension's + * ID in flarum.extensions instead, where it knows the ID for certain. + */ + +function extension(composer: Record): string { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'initializer-checks-')); + + fs.writeFileSync(path.join(root, 'composer.json'), JSON.stringify({ name: 'acme/flarum-interstellar', ...composer })); + + return root; +} + +async function upgrade(code: string, composer: Record = {}, file = 'js/src/forum/index.ts'): Promise { + const step = new MiscFrontendChanges({} as any, extension(composer), 1, 1, false); + const result = await step.applyReplacements(file, code, step.advancedContent(file, code)); + + return result.updated as string; +} + +const format = (code: string) => formatCode(code); + +const todo = + "// @TODO: check whether the extension is enabled with '' in flarum.extensions instead (https://docs.flarum.org/2.x/extend/extending-extensions). An initializer is not always named after its extension, and some were renamed in 2.0."; + +describe('checking whether another extension is enabled', () => { + it("uses a bundled extension's ID", async () => { + // As fof/sitemap's 1.x forum has it. + expect( + await upgrade(`import app from 'flarum/forum/app'; + +if (app.initializers.has('flarum-tags')) { + console.log('tags'); +} +`) + ).toBe( + await format(`import app from 'flarum/forum/app'; + +if ('flarum-tags' in flarum.extensions) { + console.log('tags'); +} +`) + ); + }); + + it('uses the IDs of the bundled extensions whose initializers 2.0 renamed', async () => { + // fof/username-request's 1.x forum checks 'flarum/nicknames', which 2.0 + // renamed 'flarum-nicknames', so the check is false in 2.0. + expect( + await upgrade(`const nicknames = app.initializers.has('flarum/nicknames'); +const lock = app.initializers.has('lock'); +const subscriptions = app.initializers.has("subscriptions"); +`) + ).toBe( + await format(`const nicknames = 'flarum-nicknames' in flarum.extensions; +const lock = 'flarum-lock' in flarum.extensions; +const subscriptions = 'flarum-subscriptions' in flarum.extensions; +`) + ); + }); + + it('uses the ID of an extension the composer.json names, by its ID or its package name', async () => { + // As ianm/follow-users' and fof/drafts' 1.x forums have it. + const composer = { + require: { 'flarum/core': '^1.8', 'fof/follow-tags': '*' }, + 'require-dev': { 'clarkwinkelmann/flarum-ext-author-change': '*' }, + suggest: { 'fof/user-directory': 'Lists users' }, + extra: { 'flarum-extension': { 'optional-dependencies': ['fof/byobu'] } }, + }; + + expect( + await upgrade( + `const tags = app.initializers.has('fof/follow-tags'); +const directory = app.initializers.has('fof-user-directory'); +const byobu = app.initializers.has('fof-byobu'); +const author = app.initializers.has('clarkwinkelmann-author-change'); +`, + composer + ) + ).toBe( + await format(`const tags = 'fof-follow-tags' in flarum.extensions; +const directory = 'fof-user-directory' in flarum.extensions; +const byobu = 'fof-byobu' in flarum.extensions; +const author = 'clarkwinkelmann-author-change' in flarum.extensions; +`) + ); + }); + + it('keeps what the check meant, whatever it is part of', async () => { + expect( + await upgrade(`const shown = !app.initializers.has('flarum-tags') && app.initializers.has('flarum-sticky') ? 1 : 2; +`) + ).toBe( + await format(`const shown = !('flarum-tags' in flarum.extensions) && 'flarum-sticky' in flarum.extensions ? 1 : 2; +`) + ); + }); + + it('leaves a check on an extension whose ID it cannot know, with a @TODO above its statement', async () => { + // As datlechin/flarum-scroll-buttons' 1.x forum has it: neither extension + // is in its composer.json. The @TODO goes above the statement, not inside + // the JSX, where it would show. + const code = `export default function view() { + return ( +
+ {app.initializers.has('acpl/mobile-tab') && app.initializers.has('itnt-uitab') ? : null} +
+ ); +} +`; + + expect(await upgrade(code, {}, 'js/src/forum/view.tsx')).toBe( + await format(`export default function view() { + ${todo} + return ( +
+ {app.initializers.has('acpl/mobile-tab') && app.initializers.has('itnt-uitab') ? : null} +
+ ); +} +`) + ); + }); + + it("keeps its changes, and the misc step's other ones, when a later change rewrites the syntax tree", async () => { + // As fof/impersonate's 1.x ImpersonateModal has it: the modal becomes a + // FormModal by rewriting the syntax tree, after the text changes ran. + const code = await upgrade(`import Modal from 'flarum/common/components/Modal'; + +export default class ImpersonateModal extends Modal { + oninit(vnode) { + super.oninit(vnode); + + this.reasonEnabled = app.initializers.has('flarum-tags'); + this.tag = this.currentTag; + } + + content() { + return ; + } +} +`); + + expect(code).toContain('extends FormModal'); + expect(code).toContain("this.reasonEnabled = 'flarum-tags' in flarum.extensions;"); + expect(code).toContain('this.tag = app.currentTag;'); + }); + + it('changes nothing the second time', async () => { + const once = await upgrade(`if (app.initializers.has('flarum-tags') || app.initializers.has('acme-other')) { + console.log('tags'); +} +`); + + expect(once).toContain("'flarum-tags' in flarum.extensions"); + expect(once.match(/@TODO/g)).toHaveLength(1); + expect(await upgrade(once)).toBe(once); + }); +});