From 3fdaad9b77893b65986a6c8b7470973d4d317517 Mon Sep 17 00:00:00 2001 From: Rejman Nascimento Date: Wed, 29 Jul 2026 20:49:22 -0300 Subject: [PATCH 1/7] fix(namespace): prevent namespace renames from matching identifier prefixes Use a boundary check when replacing old namespaces so the updater only targets complete FQCNs. This avoids corrupting existing names that merely start with the old namespace, and the tests lock in the regression case. --- .../update/MultiFileReferenceUpdater.ts | 3 +- src/domain/namespace/PhpPatterns.ts | 3 ++ src/test/PhpPatterns.test.ts | 54 ++++++++++++++++++- 3 files changed, 58 insertions(+), 2 deletions(-) diff --git a/src/app/services/update/MultiFileReferenceUpdater.ts b/src/app/services/update/MultiFileReferenceUpdater.ts index 59e2d83..2d53f4e 100644 --- a/src/app/services/update/MultiFileReferenceUpdater.ts +++ b/src/app/services/update/MultiFileReferenceUpdater.ts @@ -1,4 +1,5 @@ import { ImportRemover } from '@app/services/remove/ImportRemover'; +import { NOT_FOLLOWED_BY_IDENTIFIER_CHAR } from '@domain/namespace/PhpPatterns'; import { UseStatementCreator } from '@domain/namespace/UseStatementCreator'; import { UseStatementInjector } from '@domain/namespace/UseStatementInjector'; import { UseStatementLocator } from '@domain/namespace/UseStatementLocator'; @@ -41,7 +42,7 @@ export class MultiFileReferenceUpdater { const useImport = this.useStatementCreator.single({ fullNamespace: useNewNamespace }); const ignoreFile = newUri.fsPath; const fileStream = workspace.fs; - const namespaceRegex = new RegExp(this.escapeRegex(useOldNamespace), 'g'); + const namespaceRegex = new RegExp(`${this.escapeRegex(useOldNamespace)}${NOT_FOLLOWED_BY_IDENTIFIER_CHAR}`, 'g'); const classNameRegex = className !== newClassName ? new RegExp(`\\b${className}\\b`, 'g') diff --git a/src/domain/namespace/PhpPatterns.ts b/src/domain/namespace/PhpPatterns.ts index bf58b29..99b08de 100644 --- a/src/domain/namespace/PhpPatterns.ts +++ b/src/domain/namespace/PhpPatterns.ts @@ -20,3 +20,6 @@ export const PHP_CLASS_DECLARATION_REGEX = new RegExp( `^\\s*${DECLARATION_MODIFIER_PATTERN}(?:${NAMED_TYPE_PATTERN})\\s+(\\w+)`, 'm' ); + +// Prevents a match from being treated as a prefix of a longer identifier (e.g. "Foo" inside "FooAbstract"). +export const NOT_FOLLOWED_BY_IDENTIFIER_CHAR = '(?![A-Za-z0-9_])'; diff --git a/src/test/PhpPatterns.test.ts b/src/test/PhpPatterns.test.ts index f923f55..18c5dee 100644 --- a/src/test/PhpPatterns.test.ts +++ b/src/test/PhpPatterns.test.ts @@ -1,6 +1,6 @@ import * as assert from 'assert'; -import { PHP_CLASS_DECLARATION_REGEX } from '../domain/namespace/PhpPatterns'; +import { NOT_FOLLOWED_BY_IDENTIFIER_CHAR, PHP_CLASS_DECLARATION_REGEX } from '../domain/namespace/PhpPatterns'; suite('PHP_CLASS_DECLARATION_REGEX', () => { /** @@ -79,3 +79,55 @@ suite('PHP_CLASS_DECLARATION_REGEX', () => { }); }); }); + +suite('NOT_FOLLOWED_BY_IDENTIFIER_CHAR', () => { + /** + * Bug – renaming "DetalhePagamentoDTO" to "DetalhePagamentoDTOAbstract" was + * corrupting an unrelated "use ...DetalhePagamentoDTOAbstract;" statement + * already present in the same file, turning it into + * "...DetalhePagamentoDTOAbstractAbstract" because the namespace replace + * regex matched the old FQCN as a mere prefix of the longer one. + */ + suite('Guard against matching a FQCN that is a prefix of a longer one', () => { + function buildNamespaceRegex(oldNamespace: string): RegExp { + const escaped = oldNamespace.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + return new RegExp(`${escaped}${NOT_FOLLOWED_BY_IDENTIFIER_CHAR}`, 'g'); + } + + test('replaces the old namespace when it is not followed by extra identifier characters', () => { + const regex = buildNamespaceRegex('SharedBundle\\DetalhePagamentoDTO'); + const content = 'use SharedBundle\\DetalhePagamentoDTO;'; + + const result = content.replace(regex, 'SharedBundle\\DetalhePagamentoDTOAbstract'); + + assert.strictEqual(result, 'use SharedBundle\\DetalhePagamentoDTOAbstract;'); + }); + + test('does not touch an unrelated FQCN that has the old namespace as a prefix', () => { + const regex = buildNamespaceRegex('SharedBundle\\DetalhePagamentoDTO'); + const content = [ + 'use SharedBundle\\DetalhePagamentoDTO;', + 'use SharedBundle\\DetalhePagamentoDTOAbstract;', + ].join('\n'); + + const result = content.replace(regex, 'SharedBundle\\DetalhePagamentoDTOAbstract'); + + assert.strictEqual( + result, + [ + 'use SharedBundle\\DetalhePagamentoDTOAbstract;', + 'use SharedBundle\\DetalhePagamentoDTOAbstract;', + ].join('\n'), + ); + }); + + test('without the guard, the old regex would double the suffix (regression check)', () => { + const unguardedRegex = new RegExp('SharedBundle\\\\DetalhePagamentoDTO', 'g'); + const content = 'use SharedBundle\\DetalhePagamentoDTOAbstract;'; + + const result = content.replace(unguardedRegex, 'SharedBundle\\DetalhePagamentoDTOAbstract'); + + assert.strictEqual(result, 'use SharedBundle\\DetalhePagamentoDTOAbstractAbstract;'); + }); + }); +}); From 36f086914750f8eccb6517c12ef569d3af6f2ee9 Mon Sep 17 00:00:00 2001 From: Rejman Nascimento Date: Wed, 29 Jul 2026 21:25:19 -0300 Subject: [PATCH 2/7] fix(namespace-refactor): prevent prefix matches in php namespace refactors Use namespace-aware regex boundaries when renaming classes and scanning imports so sub-namespace paths are not mistaken for the class itself. This avoids corrupting aliased imports that share a name prefix with the renamed class. --- src/app/services/update/ClassNameUpdater.ts | 7 +- .../update/MultiFileReferenceUpdater.ts | 5 +- src/domain/namespace/PhpPatterns.ts | 8 ++ src/domain/namespace/UnusedImportDetector.ts | 4 +- src/test/ClassNameUpdater.test.ts | 38 ++++++++++ src/test/MultiFileReferenceUpdater.test.ts | 63 +++++++++++++++ src/test/PhpPatterns.test.ts | 76 ++++++++++++++++++- src/test/UnusedImportDetector.test.ts | 20 +++++ 8 files changed, 215 insertions(+), 6 deletions(-) diff --git a/src/app/services/update/ClassNameUpdater.ts b/src/app/services/update/ClassNameUpdater.ts index 9a89761..87bf571 100644 --- a/src/app/services/update/ClassNameUpdater.ts +++ b/src/app/services/update/ClassNameUpdater.ts @@ -1,4 +1,4 @@ -import { PHP_CLASS_DECLARATION_REGEX } from '@domain/namespace/PhpPatterns'; +import { NOT_FOLLOWED_BY_NAMESPACE_CHAR, NOT_PRECEDED_BY_NAMESPACE_CHAR, PHP_CLASS_DECLARATION_REGEX } from '@domain/namespace/PhpPatterns'; import { WorkspacePathResolver } from '@domain/workspace/WorkspacePathResolver'; import { FileEditApplier } from '@infra/vscode/FileEditApplier'; import { TextDocumentOpener } from '@infra/vscode/TextDocumentOpener'; @@ -32,7 +32,10 @@ export class ClassNameUpdater { return; } - const newText = text.replace(new RegExp(`\\b${currentName}\\b`, 'g'), expectedName); + const newText = text.replace( + new RegExp(`${NOT_PRECEDED_BY_NAMESPACE_CHAR}${currentName}${NOT_FOLLOWED_BY_NAMESPACE_CHAR}`, 'g'), + expectedName, + ); const edit = new WorkspaceEdit(); edit.replace( diff --git a/src/app/services/update/MultiFileReferenceUpdater.ts b/src/app/services/update/MultiFileReferenceUpdater.ts index f24ab9d..725156f 100644 --- a/src/app/services/update/MultiFileReferenceUpdater.ts +++ b/src/app/services/update/MultiFileReferenceUpdater.ts @@ -1,4 +1,5 @@ import { ImportRemover } from '@app/services/remove/ImportRemover'; +import { NOT_FOLLOWED_BY_NAMESPACE_CHAR, NOT_PRECEDED_BY_NAMESPACE_CHAR } from '@domain/namespace/PhpPatterns'; import { UseStatementCreator } from '@domain/namespace/UseStatementCreator'; import { UseStatementInjector } from '@domain/namespace/UseStatementInjector'; import { UseStatementLocator } from '@domain/namespace/UseStatementLocator'; @@ -47,10 +48,10 @@ export class MultiFileReferenceUpdater { const newClassName = this.workspacePathResolver.extractClassNameFromPath(newUri.fsPath); const useImport = this.useStatementCreator.single({ fullNamespace: useNewNamespace }); const ignoreFile = newUri.fsPath; - const namespaceRegex = new RegExp(this.escapeRegex(useOldNamespace), 'g'); + const namespaceRegex = new RegExp(`${this.escapeRegex(useOldNamespace)}${NOT_FOLLOWED_BY_NAMESPACE_CHAR}`, 'g'); const classNameRegex = className !== newClassName - ? new RegExp(`\\b${className}\\b`, 'g') + ? new RegExp(`${NOT_PRECEDED_BY_NAMESPACE_CHAR}${className}${NOT_FOLLOWED_BY_NAMESPACE_CHAR}`, 'g') : null; // Files that import/use the old namespace. diff --git a/src/domain/namespace/PhpPatterns.ts b/src/domain/namespace/PhpPatterns.ts index bf58b29..8b5ed01 100644 --- a/src/domain/namespace/PhpPatterns.ts +++ b/src/domain/namespace/PhpPatterns.ts @@ -20,3 +20,11 @@ export const PHP_CLASS_DECLARATION_REGEX = new RegExp( `^\\s*${DECLARATION_MODIFIER_PATTERN}(?:${NAMED_TYPE_PATTERN})\\s+(\\w+)`, 'm' ); + +// A namespace/identifier boundary: neither an identifier character (letter, +// digit, underscore) nor a namespace separator (\) can sit on this side of a +// match, otherwise the match is actually a prefix or suffix of a longer FQCN +// rather than the identifier itself — e.g. matching "Foo" inside "FooBar" or +// inside "Foo\Bar\Baz" (a sub-namespace that merely starts with "Foo"). +export const NOT_PRECEDED_BY_NAMESPACE_CHAR = '(? { - const regex = new RegExp(`\\b${className}\\b`, 'g'); + const regex = new RegExp(`${NOT_PRECEDED_BY_NAMESPACE_CHAR}${className}${NOT_FOLLOWED_BY_NAMESPACE_CHAR}`, 'g'); if (regex.test(contentDocument) && !classesUsed.includes(className)) { classesUsed.push(className); } diff --git a/src/test/ClassNameUpdater.test.ts b/src/test/ClassNameUpdater.test.ts index fd59cca..21d824a 100644 --- a/src/test/ClassNameUpdater.test.ts +++ b/src/test/ClassNameUpdater.test.ts @@ -116,4 +116,42 @@ suite('ClassNameUpdater', () => { const text = await waitForText(uri, t => t.includes('OutroTest.class')); assert.ok(text.includes('class OutroTest.class'), `expected today's known-bad output, got:\n${text}`); }); + + /** + * A class can share its name with a sibling namespace (e.g. a + * RenamedClass.php file next to a RenamedClass/ directory holding + * Foo/Type.php). When RenamedClass.php is renamed to + * RenamedClassTest.php, its own aliased imports from that sibling + * namespace must not be corrupted just because they start with the old name. + */ + test('does not corrupt its own aliased imports from a sub-namespace sharing its name', async () => { + const content = [ + ' t.includes('class RenamedClassTest')); + assert.ok( + text.includes('use App\\Controller\\RenamedClass\\Foo\\Type as FooType;'), + `aliased sub-namespace import should be left untouched, got:\n${text}`, + ); + assert.ok( + text.includes('use App\\Controller\\RenamedClass\\Bar\\Type as BarType;'), + `aliased sub-namespace import should be left untouched, got:\n${text}`, + ); + }); }); diff --git a/src/test/MultiFileReferenceUpdater.test.ts b/src/test/MultiFileReferenceUpdater.test.ts index b16b8af..cdad664 100644 --- a/src/test/MultiFileReferenceUpdater.test.ts +++ b/src/test/MultiFileReferenceUpdater.test.ts @@ -239,4 +239,67 @@ suite('MultiFileReferenceUpdater', () => { ); assert.ok(!/\bprivate Order \$order\b/.test(text), `old bare class name should be gone, got:\n${text}`); }); + + /** + * A class can share its name with a sibling namespace (e.g. a + * RevisaoCadastral.php file next to a RevisaoCadastral/ directory holding + * DadosPessoais/FormType.php and Endereco/FormType.php). Renaming the class + * to RevisaoCadastralTeste must not corrupt aliased imports that merely + * start with the old FQCN but actually point into that sibling namespace. + */ + test('renaming a class does not corrupt aliased imports from a sub-namespace sharing its name', async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'php-namespace-refactor-')); + + const oldUri = vscode.Uri.file(path.join(dir, 'RevisaoCadastral.php')); + const newUri = vscode.Uri.file(path.join(dir, 'RevisaoCadastralTeste.php')); + + const consumerContent = [ + ' { /** @@ -79,3 +83,73 @@ suite('PHP_CLASS_DECLARATION_REGEX', () => { }); }); }); + +suite('NOT_PRECEDED_BY_NAMESPACE_CHAR / NOT_FOLLOWED_BY_NAMESPACE_CHAR', () => { + /** + * Bug – a class "RevisaoCadastral" can coexist with a sibling namespace of + * the same name (e.g. a RevisaoCadastral.php file next to a + * RevisaoCadastral/ directory). Renaming the class to "RevisaoCadastralTeste" + * must not corrupt aliased imports from that sibling namespace, such as + * "use ...\RevisaoCadastral\DadosPessoais\FormType as DadosPessoaisFormType;", + * because "RevisaoCadastral" there is a namespace segment, not the class. + */ + function buildGuardedRegex(identifier: string): RegExp { + return new RegExp(`${NOT_PRECEDED_BY_NAMESPACE_CHAR}${identifier}${NOT_FOLLOWED_BY_NAMESPACE_CHAR}`, 'g'); + } + + suite('Guard against matching an identifier that is a namespace-path prefix of a longer one', () => { + test('replaces the identifier when it stands alone as a full FQCN', () => { + const regex = buildGuardedRegex('App\\\\Controller\\\\Atendimento\\\\RevisaoCadastral'); + const content = 'use App\\Controller\\Atendimento\\RevisaoCadastral;'; + + const result = content.replace(regex, 'App\\Controller\\Atendimento\\RevisaoCadastralTeste'); + + assert.strictEqual(result, 'use App\\Controller\\Atendimento\\RevisaoCadastralTeste;'); + }); + + test('does not touch aliased imports from a deeper sub-namespace sharing the same prefix', () => { + const regex = buildGuardedRegex('RevisaoCadastral'); + const content = [ + 'use App\\Controller\\Atendimento\\RevisaoCadastral\\DadosPessoais\\FormType as DadosPessoaisFormType;', + 'use App\\Controller\\Atendimento\\RevisaoCadastral\\Endereco\\FormType as EnderecoFormType;', + ].join('\n'); + + const result = content.replace(regex, 'RevisaoCadastralTeste'); + + assert.strictEqual(result, content, `sub-namespace imports should be left untouched, got:\n${result}`); + }); + + test('still replaces the bare identifier when it is not part of a namespace path', () => { + const regex = buildGuardedRegex('RevisaoCadastral'); + const content = 'private RevisaoCadastral $revisaoCadastral;\n\nnew RevisaoCadastral();'; + + const result = content.replace(regex, 'RevisaoCadastralTeste'); + + assert.strictEqual( + result, + 'private RevisaoCadastralTeste $revisaoCadastral;\n\nnew RevisaoCadastralTeste();', + ); + }); + + test('does not match a suffix identifier that merely starts with the same characters', () => { + const regex = buildGuardedRegex('RevisaoCadastral'); + const content = 'use App\\Domain\\RevisaoCadastralAbstract;'; + + const result = content.replace(regex, 'RevisaoCadastralTeste'); + + assert.strictEqual(result, content); + }); + + test('without the guard, the old regex would corrupt the sub-namespace imports (regression check)', () => { + const unguardedRegex = new RegExp('RevisaoCadastral', 'g'); + const content = 'use App\\Controller\\Atendimento\\RevisaoCadastral\\DadosPessoais\\FormType as DadosPessoaisFormType;'; + + const result = content.replace(unguardedRegex, 'RevisaoCadastralTeste'); + + assert.strictEqual( + result, + 'use App\\Controller\\Atendimento\\RevisaoCadastralTeste\\DadosPessoais\\FormType as DadosPessoaisFormType;', + ); + }); + }); +}); diff --git a/src/test/UnusedImportDetector.test.ts b/src/test/UnusedImportDetector.test.ts index 5566923..25f8872 100644 --- a/src/test/UnusedImportDetector.test.ts +++ b/src/test/UnusedImportDetector.test.ts @@ -110,5 +110,25 @@ suite('UnusedImportDetector', () => { const result = detector.execute({ contentDocument: content, classes: ['UserService'] }); assert.strictEqual(result.length, 1); }); + + /** + * A class can share its name with a sibling namespace (e.g. a + * RevisaoCadastral.php file next to a RevisaoCadastral/ directory). An + * aliased import from that sibling namespace, such as + * "use ...\RevisaoCadastral\DadosPessoais\FormType as DadosPessoaisFormType;", + * must not make "RevisaoCadastral" look used — it's a namespace segment + * there, not a reference to the class. + */ + test('does not treat a class name as used merely because it prefixes a sub-namespace path', () => { + const content = [ + 'namespace App\\Controller\\PreCadastro\\Atendimento;', + 'use App\\Controller\\PreCadastro\\Atendimento\\RevisaoCadastral\\DadosPessoais\\FormType as DadosPessoaisFormType;', + 'use App\\Controller\\PreCadastro\\Atendimento\\RevisaoCadastral\\Endereco\\FormType as EnderecoFormType;', + 'class Foo { function bar(DadosPessoaisFormType $f) {} }', + ].join('\n'); + + const result = detector.execute({ contentDocument: content, classes: ['RevisaoCadastral'] }); + assert.deepStrictEqual(result, []); + }); }); }); From 5e4b25c0e5d40885b67fbe74572ed5fe652dbe76 Mon Sep 17 00:00:00 2001 From: Rejman Nascimento Date: Wed, 29 Jul 2026 22:32:46 -0300 Subject: [PATCH 3/7] feat(property-renaming): rename constructor-typed properties on class move Add an opt-in rename pass after class/file moves so constructor-typed properties and their usages stay aligned with the renamed class. Include a separate flag for cases where the existing property name does not match the class-name convention. --- package.json | 10 ++ src/app/operations/FileMoveOperation.ts | 6 + src/app/operations/PropertyRenameOperation.ts | 160 +++++++++++++++++ .../property/ClassTypedPropertyLocator.ts | 126 +++++++++++++ src/domain/property/ConstructorSpanFinder.ts | 61 +++++++ src/domain/property/PropertyNameResolver.ts | 8 + .../property/PropertyRenameConfigKeys.ts | 3 + src/domain/workspace/ConfigurationLocator.ts | 1 + src/test/ClassTypedPropertyLocator.test.ts | 102 +++++++++++ src/test/PropertyRenameOperation.test.ts | 165 ++++++++++++++++++ 10 files changed, 642 insertions(+) create mode 100644 src/app/operations/PropertyRenameOperation.ts create mode 100644 src/domain/property/ClassTypedPropertyLocator.ts create mode 100644 src/domain/property/ConstructorSpanFinder.ts create mode 100644 src/domain/property/PropertyNameResolver.ts create mode 100644 src/domain/property/PropertyRenameConfigKeys.ts create mode 100644 src/test/ClassTypedPropertyLocator.test.ts create mode 100644 src/test/PropertyRenameOperation.test.ts diff --git a/package.json b/package.json index 2858e79..d127d9f 100644 --- a/package.json +++ b/package.json @@ -109,6 +109,16 @@ "type": "boolean", "default": true, "description": "Apply refactor edits to files without opening them, keeping only files that were already open in the editor. Disable to have every edited file opened in the editor as before." + }, + "phpNamespaceRefactor.renameProperties": { + "type": "boolean", + "default": false, + "description": "Rename constructor-typed properties (promoted or not, readonly or not) to match the class name when renaming a class." + }, + "phpNamespaceRefactor.renameProperties.renameMismatchedNames": { + "type": "boolean", + "default": false, + "description": "Also rename properties whose current name doesn't match the class name (e.g. $service for a Teste type). Only applies when renameProperties is enabled." } } } diff --git a/src/app/operations/FileMoveOperation.ts b/src/app/operations/FileMoveOperation.ts index 11562aa..caa2681 100644 --- a/src/app/operations/FileMoveOperation.ts +++ b/src/app/operations/FileMoveOperation.ts @@ -7,6 +7,7 @@ import { FeatureFlagManager } from '@domain/workspace/FeatureFlagManager'; import { inject, injectable } from 'tsyringe'; import type { FileMove } from './FileMove'; +import { PropertyRenameOperation } from './PropertyRenameOperation'; @injectable() export class FileMoveOperation { @@ -16,6 +17,7 @@ export class FileMoveOperation { @inject(MissingClassImporter) private missingClassImporter: MissingClassImporter, @inject(ImportRemover) private importRemover: ImportRemover, @inject(FeatureFlagManager) private featureFlagManager: FeatureFlagManager, + @inject(PropertyRenameOperation) private propertyRenameOperation: PropertyRenameOperation, ) {} public async execute(files: ReadonlyArray): Promise { @@ -29,6 +31,10 @@ export class FileMoveOperation { try { await this.namespaceBatchUpdater.execute({ newUri, oldUri }); + if (this.featureFlagManager.isActive({ key: ConfigKeys.RENAME_PROPERTIES, defaultValue: false })) { + await this.propertyRenameOperation.execute({ oldUri, newUri }); + } + if (this.featureFlagManager.isActive({ key: ConfigKeys.AUTO_IMPORT_NAMESPACE })) { await this.missingClassImporter.execute({ oldUri, newUri }); } diff --git a/src/app/operations/PropertyRenameOperation.ts b/src/app/operations/PropertyRenameOperation.ts new file mode 100644 index 0000000..cd3a192 --- /dev/null +++ b/src/app/operations/PropertyRenameOperation.ts @@ -0,0 +1,160 @@ +import { ClassTypedPropertyLocator, PropertyMatch } from '@domain/property/ClassTypedPropertyLocator'; +import { ConstructorSpanFinder } from '@domain/property/ConstructorSpanFinder'; +import { PropertyNameResolver } from '@domain/property/PropertyNameResolver'; +import { PropertyRenameConfigKeys } from '@domain/property/PropertyRenameConfigKeys'; +import { ConfigurationLocator } from '@domain/workspace/ConfigurationLocator'; +import { WorkspacePathResolver } from '@domain/workspace/WorkspacePathResolver'; +import { WorkspaceIndex } from '@infra/index/WorkspaceIndex'; +import { FileEditApplier } from '@infra/vscode/FileEditApplier'; +import { TextDocumentOpener } from '@infra/vscode/TextDocumentOpener'; +import { inject, injectable } from 'tsyringe'; +import { Range, TextDocument, Uri, WorkspaceEdit } from 'vscode'; + +interface Props { + oldUri: Uri + newUri: Uri +} + +@injectable() +export class PropertyRenameOperation { + constructor( + @inject(WorkspacePathResolver) private workspacePathResolver: WorkspacePathResolver, + @inject(WorkspaceIndex) private workspaceIndex: WorkspaceIndex, + @inject(TextDocumentOpener) private textDocumentOpener: TextDocumentOpener, + @inject(FileEditApplier) private fileEditApplier: FileEditApplier, + @inject(ConfigurationLocator) private configurationLocator: ConfigurationLocator, + @inject(ClassTypedPropertyLocator) private classTypedPropertyLocator: ClassTypedPropertyLocator, + @inject(PropertyNameResolver) private propertyNameResolver: PropertyNameResolver, + @inject(ConstructorSpanFinder) private constructorSpanFinder: ConstructorSpanFinder, + ) {} + + public async execute({ oldUri, newUri }: Props): Promise { + const oldClassName = this.workspacePathResolver.extractClassNameFromPath(oldUri.fsPath); + const newClassName = this.workspacePathResolver.extractClassNameFromPath(newUri.fsPath); + + if (!oldClassName || !newClassName || oldClassName === newClassName) { + return; + } + + const expectedOldName = this.propertyNameResolver.resolve(oldClassName); + const expectedNewName = this.propertyNameResolver.resolve(newClassName); + + const renameMismatched = this.configurationLocator.get({ + key: PropertyRenameConfigKeys.RENAME_MISMATCHED_NAMES, + defaultValue: false, + }); + + const files = await this.getCandidateFiles(newUri); + const edit = new WorkspaceEdit(); + + await Promise.all(files.map(async (file) => { + try { + const { document, text } = await this.textDocumentOpener.execute({ uri: file }); + if (!text.includes(newClassName)) { + return; + } + + const match = this.classTypedPropertyLocator.execute({ text, className: newClassName }); + if (!match || match.propertyName === expectedNewName) { + return; + } + + const matchesOldConvention = match.propertyName === expectedOldName; + if (!matchesOldConvention && !renameMismatched) { + return; + } + + this.addPropertyRenameEdits(edit, file, document, text, newClassName, match, expectedNewName); + } catch (_) { + return; + } + })); + + await this.fileEditApplier.apply(edit); + } + + private addPropertyRenameEdits( + edit: WorkspaceEdit, + uri: Uri, + document: TextDocument, + text: string, + className: string, + match: PropertyMatch, + newName: string, + ): void { + const oldName = match.propertyName; + + const variableSpans = this.buildVariableRenameSpans(text, className, match); + for (const [start, end] of variableSpans) { + this.replaceInRange(edit, uri, document, text, start, end, new RegExp(`\\$${oldName}\\b`, 'g'), `$${newName}`); + } + + this.replaceInRange( + edit, uri, document, text, 0, text.length, + new RegExp(`\\$this->${oldName}\\b`, 'g'), `$this->${newName}`, + ); + } + + private buildVariableRenameSpans(text: string, className: string, match: PropertyMatch): [number, number][] { + const spans: [number, number][] = []; + + const constructorSpan = this.findConstructorSpan(text); + if (constructorSpan) { + spans.push(constructorSpan); + } + + if (match.hasSeparateDeclaration) { + const declarationSpan = this.findDeclarationSpan(text, className, match.propertyName); + if (declarationSpan) { + spans.push(declarationSpan); + } + } + + return spans; + } + + private findConstructorSpan(text: string): [number, number] | null { + const span = this.constructorSpanFinder.find(text); + return span ? [span.constructorStart, span.bodyEnd] : null; + } + + private findDeclarationSpan(text: string, className: string, propertyName: string): [number, number] | null { + const pattern = new RegExp( + `(?:public|protected|private)\\s+(?:readonly\\s+)?\\??\\b${className}\\b\\s+\\$${propertyName}\\s*;`, + ); + const match = pattern.exec(text); + return match ? [match.index, match.index + match[0].length] : null; + } + + private async getCandidateFiles(newUri: Uri): Promise { + const files = [newUri, ...await this.workspaceIndex.execute()]; + const seen = new Set(); + + return files.filter((file) => { + if (seen.has(file.fsPath)) { + return false; + } + seen.add(file.fsPath); + return true; + }); + } + + private replaceInRange( + edit: WorkspaceEdit, + uri: Uri, + document: TextDocument, + text: string, + rangeStart: number, + rangeEnd: number, + regex: RegExp, + replacement: string, + ): void { + const scoped = text.slice(rangeStart, rangeEnd); + + for (const match of scoped.matchAll(regex)) { + const start = rangeStart + (match.index as number); + const end = start + match[0].length; + edit.replace(uri, new Range(document.positionAt(start), document.positionAt(end)), replacement); + } + } +} diff --git a/src/domain/property/ClassTypedPropertyLocator.ts b/src/domain/property/ClassTypedPropertyLocator.ts new file mode 100644 index 0000000..7a58aa4 --- /dev/null +++ b/src/domain/property/ClassTypedPropertyLocator.ts @@ -0,0 +1,126 @@ +import { inject, injectable } from 'tsyringe'; + +import { ConstructorSpanFinder } from './ConstructorSpanFinder'; + +const VISIBILITY = 'public|protected|private'; + +interface Props { + text: string + className: string +} + +export interface PropertyMatch { + propertyName: string + isPromoted: boolean + hasSeparateDeclaration: boolean +} + +/** + * Locates the single constructor property that represents an instance of + * `className` in a PHP file's source text - promoted or not, readonly or + * not. Returns null when there's no such property, or when more than one + * parameter shares that type (renaming either would be a guess that risks + * a variable-name collision). + */ +@injectable() +export class ClassTypedPropertyLocator { + constructor( + @inject(ConstructorSpanFinder) private constructorSpanFinder: ConstructorSpanFinder, + ) {} + + public execute({ text, className }: Props): PropertyMatch | null { + const span = this.constructorSpanFinder.find(text); + if (!span) { + return null; + } + + const params = text.slice(span.paramsStart, span.paramsEnd); + const body = text.slice(span.bodyStart, span.bodyEnd); + + const candidates = this.splitParams(params) + .map(param => this.matchParam(param, body, text, className)) + .filter((match): match is PropertyMatch => match !== null); + + return candidates.length === 1 ? candidates[0] : null; + } + + private hasPropertyDeclaration(text: string, className: string, varName: string): boolean { + const pattern = new RegExp( + `(?:${VISIBILITY})\\s+(?:readonly\\s+)?\\??\\b${className}\\b\\s+\\$${varName}\\s*;`, + ); + return pattern.test(text); + } + + private isAssignedToThis(constructorBody: string, varName: string): boolean { + const pattern = new RegExp(`\\$this->${varName}\\s*=\\s*\\$${varName}\\s*;`); + return pattern.test(constructorBody); + } + + private matchParam(param: string, constructorBody: string, text: string, className: string): PropertyMatch | null { + const cleanedParam = this.stripAttributes(param); + + const promotedName = this.matchPromoted(cleanedParam, className); + if (promotedName) { + return { propertyName: promotedName, isPromoted: true, hasSeparateDeclaration: false }; + } + + const plainName = this.matchPlain(cleanedParam, className); + if (!plainName || !this.isAssignedToThis(constructorBody, plainName)) { + return null; + } + + return { + propertyName: plainName, + isPromoted: false, + hasSeparateDeclaration: this.hasPropertyDeclaration(text, className, plainName), + }; + } + + private matchPlain(cleanedParam: string, className: string): string | null { + if (new RegExp(`\\b(?:${VISIBILITY}|readonly)\\b`).test(cleanedParam)) { + return null; + } + + const pattern = new RegExp(`\\??\\b${className}\\b\\s+\\$(\\w+)`); + return pattern.exec(cleanedParam)?.[1] ?? null; + } + + private matchPromoted(cleanedParam: string, className: string): string | null { + const pattern = new RegExp( + `(?:(?:${VISIBILITY})\\s+(?:readonly\\s+)?|readonly\\s+(?:${VISIBILITY})\\s+)\\??\\b${className}\\b\\s+\\$(\\w+)`, + ); + return pattern.exec(cleanedParam)?.[1] ?? null; + } + + private splitParams(params: string): string[] { + const result: string[] = []; + let depth = 0; + let current = ''; + + for (const char of params) { + if (char === '(' || char === '[' || char === '{') { + depth++; + } else if (char === ')' || char === ']' || char === '}') { + depth--; + } + + if (char === ',' && depth === 0) { + result.push(current); + current = ''; + continue; + } + + current += char; + } + + if (current.trim()) { + result.push(current); + } + + return result; + } + + private stripAttributes(param: string): string { + return param.replace(/#\[[^\]]*\]/g, ' ').trim(); + } +} diff --git a/src/domain/property/ConstructorSpanFinder.ts b/src/domain/property/ConstructorSpanFinder.ts new file mode 100644 index 0000000..256e6dd --- /dev/null +++ b/src/domain/property/ConstructorSpanFinder.ts @@ -0,0 +1,61 @@ +import { injectable } from 'tsyringe'; + +export interface ConstructorSpan { + constructorStart: number + paramsStart: number + paramsEnd: number + bodyStart: number + bodyEnd: number +} + +@injectable() +export class ConstructorSpanFinder { + public find(text: string): ConstructorSpan | null { + const signatureMatch = /function\s+__construct\s*\(/.exec(text); + if (!signatureMatch) { + return null; + } + + const paramsStart = signatureMatch.index + signatureMatch[0].length; + const paramsEnd = this.findMatching(text, paramsStart - 1, '(', ')'); + if (paramsEnd === -1) { + return null; + } + + const bodyStart = text.indexOf('{', paramsEnd); + if (bodyStart === -1) { + return { + constructorStart: signatureMatch.index, + paramsStart, + paramsEnd, + bodyStart: paramsEnd + 1, + bodyEnd: paramsEnd + 1, + }; + } + + const bodyEnd = this.findMatching(text, bodyStart, '{', '}'); + + return { + constructorStart: signatureMatch.index, + paramsStart, + paramsEnd, + bodyStart, + bodyEnd: bodyEnd === -1 ? text.length : bodyEnd + 1, + }; + } + + public findMatching(text: string, openIndex: number, open: string, close: string): number { + let depth = 0; + for (let i = openIndex; i < text.length; i++) { + if (text[i] === open) { + depth++; + } else if (text[i] === close) { + depth--; + if (depth === 0) { + return i; + } + } + } + return -1; + } +} diff --git a/src/domain/property/PropertyNameResolver.ts b/src/domain/property/PropertyNameResolver.ts new file mode 100644 index 0000000..fe75a44 --- /dev/null +++ b/src/domain/property/PropertyNameResolver.ts @@ -0,0 +1,8 @@ +import { injectable } from 'tsyringe'; + +@injectable() +export class PropertyNameResolver { + public resolve(className: string): string { + return className.charAt(0).toLowerCase() + className.slice(1); + } +} diff --git a/src/domain/property/PropertyRenameConfigKeys.ts b/src/domain/property/PropertyRenameConfigKeys.ts new file mode 100644 index 0000000..740996b --- /dev/null +++ b/src/domain/property/PropertyRenameConfigKeys.ts @@ -0,0 +1,3 @@ +export const PropertyRenameConfigKeys = { + RENAME_MISMATCHED_NAMES: 'renameProperties.renameMismatchedNames', +} as const; diff --git a/src/domain/workspace/ConfigurationLocator.ts b/src/domain/workspace/ConfigurationLocator.ts index e0d1e7c..437f1f9 100644 --- a/src/domain/workspace/ConfigurationLocator.ts +++ b/src/domain/workspace/ConfigurationLocator.ts @@ -10,6 +10,7 @@ export const ConfigKeys = { ADDITIONAL_EXTENSIONS: 'additionalExtensions', RENAME: 'rename', EDIT_FILES_IN_BACKGROUND: 'editFilesInBackground', + RENAME_PROPERTIES: 'renameProperties', } as const; export type Props = { diff --git a/src/test/ClassTypedPropertyLocator.test.ts b/src/test/ClassTypedPropertyLocator.test.ts new file mode 100644 index 0000000..e859674 --- /dev/null +++ b/src/test/ClassTypedPropertyLocator.test.ts @@ -0,0 +1,102 @@ +import 'reflect-metadata'; + +import * as assert from 'assert'; + +import { ClassTypedPropertyLocator } from '../domain/property/ClassTypedPropertyLocator'; +import { ConstructorSpanFinder } from '../domain/property/ConstructorSpanFinder'; + +function locate(text: string, className: string) { + const locator = new ClassTypedPropertyLocator(new ConstructorSpanFinder()); + return locator.execute({ text, className }); +} + +suite('ClassTypedPropertyLocator', () => { + test('finds a promoted property', () => { + const text = [ + 'class UserController', + '{', + ' public function __construct(private Teste $teste)', + ' {', + ' }', + '}', + ].join('\n'); + + const match = locate(text, 'Teste'); + assert.ok(match); + assert.strictEqual(match!.propertyName, 'teste'); + assert.strictEqual(match!.isPromoted, true); + assert.strictEqual(match!.hasSeparateDeclaration, false); + }); + + test('finds a promoted readonly property regardless of modifier order', () => { + const first = locate('function __construct(private readonly Teste $teste) {}', 'Teste'); + const second = locate('function __construct(readonly private Teste $teste) {}', 'Teste'); + + assert.strictEqual(first!.propertyName, 'teste'); + assert.strictEqual(second!.propertyName, 'teste'); + }); + + test('finds a non-promoted property confirmed by a constructor assignment', () => { + const text = [ + 'class UserController', + '{', + ' private Teste $teste;', + '', + ' public function __construct(Teste $teste)', + ' {', + ' $this->teste = $teste;', + ' }', + '}', + ].join('\n'); + + const match = locate(text, 'Teste'); + assert.ok(match); + assert.strictEqual(match!.propertyName, 'teste'); + assert.strictEqual(match!.isPromoted, false); + assert.strictEqual(match!.hasSeparateDeclaration, true); + }); + + test('ignores a non-promoted parameter that is never stored on $this', () => { + const text = [ + 'class Validator', + '{', + ' public function __construct(Teste $teste)', + ' {', + ' $teste->validate();', + ' }', + '}', + ].join('\n'); + + assert.strictEqual(locate(text, 'Teste'), null); + }); + + test('matches a property with a mismatched name', () => { + const text = 'function __construct(private Teste $service) {}'; + const match = locate(text, 'Teste'); + + assert.ok(match); + assert.strictEqual(match!.propertyName, 'service'); + }); + + test('matches a nullable type hint', () => { + const text = 'function __construct(private ?Teste $teste) {}'; + const match = locate(text, 'Teste'); + + assert.ok(match); + assert.strictEqual(match!.propertyName, 'teste'); + }); + + test('returns null when there is no constructor', () => { + assert.strictEqual(locate('class Teste {}', 'Teste'), null); + }); + + test('returns null when the class type does not appear in the constructor', () => { + const text = 'function __construct(private Other $other) {}'; + assert.strictEqual(locate(text, 'Teste'), null); + }); + + test('returns null when two parameters share the same type (ambiguous)', () => { + const text = 'function __construct(private Teste $a, private Teste $b) {}'; + assert.strictEqual(locate(text, 'Teste'), null); + }); +}); diff --git a/src/test/PropertyRenameOperation.test.ts b/src/test/PropertyRenameOperation.test.ts new file mode 100644 index 0000000..e005126 --- /dev/null +++ b/src/test/PropertyRenameOperation.test.ts @@ -0,0 +1,165 @@ +import 'reflect-metadata'; + +import * as assert from 'assert'; +import { promises as fs } from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import * as vscode from 'vscode'; + +import { PropertyRenameOperation } from '../app/operations/PropertyRenameOperation'; +import { ClassTypedPropertyLocator } from '../domain/property/ClassTypedPropertyLocator'; +import { ConstructorSpanFinder } from '../domain/property/ConstructorSpanFinder'; +import { PropertyNameResolver } from '../domain/property/PropertyNameResolver'; +import { ConfigurationLocator, Props } from '../domain/workspace/ConfigurationLocator'; +import { FeatureFlagManager } from '../domain/workspace/FeatureFlagManager'; +import { FileExtensionResolver } from '../domain/workspace/FileExtensionResolver'; +import { WorkspacePathResolver } from '../domain/workspace/WorkspacePathResolver'; +import { ComposerAutoloadManager } from '../infra/autoload/ComposerAutoloadManager'; +import { WorkspaceIndex } from '../infra/index/WorkspaceIndex'; +import { FileEditApplier } from '../infra/vscode/FileEditApplier'; +import { TextDocumentOpener } from '../infra/vscode/TextDocumentOpener'; + +function fakePassthroughConfigurationLocator(): ConfigurationLocator { + return { + get: ({ defaultValue }: Props): T => defaultValue as T, + } as ConfigurationLocator; +} + +function fakeRenameMismatchConfigurationLocator(renameMismatched: boolean): ConfigurationLocator { + return { + get: (): T => renameMismatched as unknown as T, + } as unknown as ConfigurationLocator; +} + +function fakeFeatureFlagManager(editFilesInBackground = true): FeatureFlagManager { + return { + isActive: ({ defaultValue = true }) => defaultValue && editFilesInBackground, + } as FeatureFlagManager; +} + +function buildOperation({ + files = [] as vscode.Uri[], + renameMismatched = false, + editFilesInBackground = true, +} = {}): PropertyRenameOperation { + const workspacePathResolver = new WorkspacePathResolver( + new ComposerAutoloadManager(), + new FileExtensionResolver(fakePassthroughConfigurationLocator()), + ); + const workspaceIndex = { execute: async () => files } as unknown as WorkspaceIndex; + const fileEditApplier = new FileEditApplier(fakeFeatureFlagManager(editFilesInBackground)); + const constructorSpanFinder = new ConstructorSpanFinder(); + + return new PropertyRenameOperation( + workspacePathResolver, + workspaceIndex, + new TextDocumentOpener(), + fileEditApplier, + fakeRenameMismatchConfigurationLocator(renameMismatched), + new ClassTypedPropertyLocator(constructorSpanFinder), + new PropertyNameResolver(), + constructorSpanFinder, + ); +} + +async function writeTempPhpFile(dir: string, fileName: string, content: string): Promise { + const filePath = path.join(dir, fileName); + await fs.writeFile(filePath, content, 'utf8'); + return vscode.Uri.file(filePath); +} + +suite('PropertyRenameOperation', () => { + test('renames a promoted property that matches the old class-name convention', async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'php-namespace-refactor-')); + const oldUri = vscode.Uri.file(path.join(dir, 'Teste.php')); + const newUri = vscode.Uri.file(path.join(dir, 'Novo.php')); + + const consumerContent = 'teste->run();\n }\n}\n'; + const consumerUri = await writeTempPhpFile(dir, 'UserController.php', consumerContent); + + const operation = buildOperation({ files: [consumerUri] }); + await operation.execute({ oldUri, newUri }); + + const text = (await vscode.workspace.openTextDocument(consumerUri)).getText(); + assert.ok(text.includes('private Novo $novo'), `expected the promoted property to be renamed, got:\n${text}`); + assert.ok(text.includes('$this->novo->run();'), `expected $this-> usages to be renamed, got:\n${text}`); + assert.ok(!text.includes('teste'), `expected no leftover old property name, got:\n${text}`); + }); + + test('renames a non-promoted property confirmed by its constructor assignment', async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'php-namespace-refactor-')); + const oldUri = vscode.Uri.file(path.join(dir, 'Teste.php')); + const newUri = vscode.Uri.file(path.join(dir, 'Novo.php')); + + const consumerContent = 'teste = $teste;\n }\n}\n'; + const consumerUri = await writeTempPhpFile(dir, 'UserController.php', consumerContent); + + const operation = buildOperation({ files: [consumerUri] }); + await operation.execute({ oldUri, newUri }); + + const text = (await vscode.workspace.openTextDocument(consumerUri)).getText(); + assert.ok(text.includes('private Novo $novo;'), `expected the declared property to be renamed, got:\n${text}`); + assert.ok(text.includes('__construct(Novo $novo)'), `expected the constructor parameter to be renamed, got:\n${text}`); + assert.ok(text.includes('$this->novo = $novo;'), `expected the assignment to be renamed, got:\n${text}`); + }); + + test('leaves a mismatched property name untouched when the sub-flag is off', async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'php-namespace-refactor-')); + const oldUri = vscode.Uri.file(path.join(dir, 'Teste.php')); + const newUri = vscode.Uri.file(path.join(dir, 'Novo.php')); + + const consumerContent = ' { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'php-namespace-refactor-')); + const oldUri = vscode.Uri.file(path.join(dir, 'Teste.php')); + const newUri = vscode.Uri.file(path.join(dir, 'Novo.php')); + + const consumerContent = 'service->run();\n }\n}\n'; + const consumerUri = await writeTempPhpFile(dir, 'UserController.php', consumerContent); + + const operation = buildOperation({ files: [consumerUri], renameMismatched: true }); + await operation.execute({ oldUri, newUri }); + + const text = (await vscode.workspace.openTextDocument(consumerUri)).getText(); + assert.ok(text.includes('private Novo $novo'), `expected the mismatched property to be renamed, got:\n${text}`); + assert.ok(text.includes('$this->novo->run();'), `expected $this-> usages to be renamed, got:\n${text}`); + }); + + test('skips a file when two properties share the renamed class type (ambiguous)', async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'php-namespace-refactor-')); + const oldUri = vscode.Uri.file(path.join(dir, 'Teste.php')); + const newUri = vscode.Uri.file(path.join(dir, 'Novo.php')); + + const consumerContent = ' { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'php-namespace-refactor-')); + const sameUri = vscode.Uri.file(path.join(dir, 'Teste.php')); + + const consumerContent = ' Date: Wed, 29 Jul 2026 22:33:04 -0300 Subject: [PATCH 4/7] docs: document optional property renaming during file moves Add documentation for the new property-rename step, its config flags, and where it fits in the file-move workflow so users can understand why the extra behavior is opt-in and when it applies. --- README.md | 19 ++++++++++++++++++- docs/architecture.md | 2 +- docs/configuration.md | 19 ++++++++++++++++++- docs/operations/file-move.md | 29 ++++++++++++++++++++++------- 4 files changed, 59 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index cd51aa9..0387167 100644 --- a/README.md +++ b/README.md @@ -24,6 +24,8 @@ Ideal for projects using PSR-4, making it easy to reorganize directories without - Additional Extensions: Specify the file extensions to consider during the namespace refactoring process. +- Rename Properties (off by default): When a class is renamed, also rename its class-typed constructor properties (promoted or not, readonly or not) and their `$this->x` usages to match the new class name. + ## Requirements - PHP 7.4+ @@ -47,7 +49,9 @@ This extension contributes the following settings: "php" ], "phpNamespaceRefactor.rename": true, - "phpNamespaceRefactor.editFilesInBackground": true + "phpNamespaceRefactor.editFilesInBackground": true, + "phpNamespaceRefactor.renameProperties": false, + "phpNamespaceRefactor.renameProperties.renameMismatchedNames": false } ``` @@ -91,6 +95,19 @@ This extension contributes the following settings: - Default: true. +**phpNamespaceRefactor.renameProperties** + +- When a class is renamed, also renames its class-typed constructor properties (promoted or not, readonly or not) and every `$this->x` usage to match the new class name — e.g. `private Teste $teste` becomes `private Novo $novo` when `Teste` is renamed to `Novo`. +- If more than one property shares the same type in a constructor, the file is skipped rather than guessing which one to rename. + +- Default: false. + +**phpNamespaceRefactor.renameProperties.renameMismatchedNames** + +- Only applies when `renameProperties` is enabled. By default, only properties whose name already matches the old class name are renamed. Enable this to also rename properties named differently from their type (e.g. `private Teste $service` becomes `private Novo $novo`). + +- Default: false. + ## Documentation For architecture, internals, and troubleshooting notes, see [./docs/](./docs/README.md). diff --git a/docs/architecture.md b/docs/architecture.md index c0fe10f..ddc8470 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -16,7 +16,7 @@ Inside `app/`: - `commands/` — entry points triggered by VS Code commands or events (`RenameHandler`, `FileRenameHandler`) - `features/` — orchestrates the flow of a single user interaction (`RenameFeature`) -- `operations/` — runs a full refactor operation (`ClassRenameOperation`, `NamespaceRenameOperation`, `FileMoveOperation`) +- `operations/` — runs a full refactor operation (`ClassRenameOperation`, `NamespaceRenameOperation`, `FileMoveOperation`, `PropertyRenameOperation`) - `services/` — reusable steps used by the operations (`NamespaceBatchUpdater`, `MissingClassImporter`, `DirectoryMovedFilesResolver`, `remove/ImportRemover`, `update/*`) - `subscribers/` — react to workspace events to keep the namespace index up to date (`FileCreatedSubscriber`, `FileDeletedSubscriber`, `FileSavedSubscriber`) diff --git a/docs/configuration.md b/docs/configuration.md index ef05441..3d28948 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -4,6 +4,8 @@ Reference for every setting contributed by the extension (`phpNamespaceRefactor. All keys are centralized in `ConfigKeys` (`src/domain/workspace/ConfigurationLocator.ts`). No other place in the codebase references a setting name as a loose string — if a key needs renaming, this is the only spot to change (besides `package.json`). +The one deliberate exception is `PropertyRenameConfigKeys` (`src/domain/property/PropertyRenameConfigKeys.ts`): `renameProperties.renameMismatchedNames` is a sub-setting of the `renameProperties` feature only, not a global flag, so it's kept in its own small constant instead of `ConfigKeys` — see [`phpNamespaceRefactor.renameProperties`](#phpnamespacerefactorrenameproperties) below. + | Setting | Key (`ConfigKeys`) | Type | Default | Read by | |---|---|---|---|---| | `phpNamespaceRefactor.ignoredDirectories` | `IGNORED_DIRECTORIES` | `string[]` | `["/vendor/", "/var/", "/cache/"]` | `WorkspaceIndex` (filters the workspace file scan) | @@ -12,13 +14,15 @@ All keys are centralized in `ConfigKeys` (`src/domain/workspace/ConfigurationLoc | `phpNamespaceRefactor.additionalExtensions` | `ADDITIONAL_EXTENSIONS` | `string[]` | `["php"]` | `FileExtensionResolver` and `WorkspaceIndex` | | `phpNamespaceRefactor.rename` | `RENAME` | `boolean` | `true` | The `phpNamespaceRefactor.rename` command (`extension.ts`) and the F2 keybinding's `when` clause (`package.json`) | | `phpNamespaceRefactor.editFilesInBackground` | `EDIT_FILES_IN_BACKGROUND` | `boolean` | `true` | `FileEditApplier` | +| `phpNamespaceRefactor.renameProperties` | `ConfigKeys.RENAME_PROPERTIES` | `boolean` | `false` | `FileMoveOperation` (decides whether to call `PropertyRenameOperation`) | +| `phpNamespaceRefactor.renameProperties.renameMismatchedNames` | `PropertyRenameConfigKeys.RENAME_MISMATCHED_NAMES` | `boolean` | `false` | `PropertyRenameOperation` | ## How configuration is read Two classes access `workspace.getConfiguration('phpNamespaceRefactor')`, each with a distinct purpose: - **`ConfigurationLocator`** (`src/domain/workspace/ConfigurationLocator.ts`) — generic read, used for settings of any type (`ignoredDirectories`, `additionalExtensions`) -- **`FeatureFlagManager`** (`src/domain/workspace/FeatureFlagManager.ts`) — specialized `boolean` read, with `defaultValue = true`. Used for every on/off flag (`autoImportNamespace`, `removeUnusedImports`, `rename`, `editFilesInBackground`) +- **`FeatureFlagManager`** (`src/domain/workspace/FeatureFlagManager.ts`) — specialized `boolean` read, with `defaultValue = true`. Used for every on/off flag (`autoImportNamespace`, `removeUnusedImports`, `rename`, `editFilesInBackground`). `renameProperties` also goes through it, but `FileMoveOperation` passes an explicit `defaultValue: false` to flip the usual default, since this flag is opt-in Neither class caches the `WorkspaceConfiguration` — each instance reads `workspace.getConfiguration()` in its constructor, and since both are `@injectable()` (not singleton), a fresh read happens on every `container.resolve()`. This means a change to the user's configuration is picked up on the next operation, with no need to reload the window. @@ -38,3 +42,16 @@ Filters files by simple substring match against `fsPath` (not a glob) — see `W ## `phpNamespaceRefactor.additionalExtensions` Normalized by `normalizeExtensions()` (`src/infra/utils/extensions.ts`): strips leading dots and whitespace, lowercases, and always guarantees `php` is included even if not listed. `FileExtensionResolver.match()` sorts extensions from longest to shortest before comparing, so a compound extension (e.g. `class.php`) is never shadowed by the plain `php` entry. + +## `phpNamespaceRefactor.renameProperties` + +Master switch for `PropertyRenameOperation` — off by default, unlike the extension's other flags, because it renames identifiers (constructor parameters, promoted/non-promoted properties, `$this->x` usages), not just type hints. When a class is renamed and this is enabled, `FileMoveOperation` calls `PropertyRenameOperation` right after `NamespaceBatchUpdater`, so it only ever acts on properties whose type hint was just updated to the new class name. + +For each candidate file, `ClassTypedPropertyLocator` looks for the single constructor property typed as the renamed class (promoted or not, readonly or not); if more than one property shares that type, the file is skipped entirely rather than guessing which one to rename — see [file-move.md](./operations/file-move.md#3-property-rename-propertyrenameoperation-optional). + +### `phpNamespaceRefactor.renameProperties.renameMismatchedNames` + +Only has an effect when `renameProperties` is also enabled. Controls what happens when the property's current name doesn't already follow the class-name convention: + +- `false` (default) — a property is only renamed if its name already matches the *old* class name (e.g. `$teste` for `Teste`); a property named something else on purpose (e.g. `$service`) is left untouched +- `true` — mismatched names are renamed too, to match the *new* class name (e.g. `$service` → `$novo`) diff --git a/docs/operations/file-move.md b/docs/operations/file-move.md index c87173a..d725008 100644 --- a/docs/operations/file-move.md +++ b/docs/operations/file-move.md @@ -15,8 +15,9 @@ onDidRenameFiles (VS Code event) → DirectoryMovedFilesResolver.execute() 1. expands directory moves into per-file moves → for each .php file: → NamespaceBatchUpdater.execute() 2. updates namespace/class + references - → MissingClassImporter.execute() 3. (optional) auto-imports classes from the old directory - → ImportRemover.execute() 4. (optional, checked internally) removes stale imports + → PropertyRenameOperation.execute() 3. (optional) renames class-typed properties to match the new class name + → MissingClassImporter.execute() 4. (optional) auto-imports classes from the old directory + → ImportRemover.execute() 5. (optional, checked internally) removes stale imports ``` ### Serialized queue @@ -62,13 +63,24 @@ For each affected file, it replaces via regex: Every edit across every affected file is accumulated into a **single `WorkspaceEdit`** before being applied (`FileEditApplier.apply`), so the whole refactor shows up as one undo-stack entry in VS Code instead of one edit per file — see [issue #72](https://github.com/rejmann/php-namespace-refactor/issues/72). -At the end, `MultiFileReferenceUpdater` always calls `ImportRemover.execute({ uri: newUri })` for the moved file itself (in addition to the call `FileMoveOperation` already makes in step 4 below — both are no-ops if the flag is disabled or there's nothing to remove). +At the end, `MultiFileReferenceUpdater` always calls `ImportRemover.execute({ uri: newUri })` for the moved file itself (in addition to the call `FileMoveOperation` already makes in step 5 below — both are no-ops if the flag is disabled or there's nothing to remove). -### 3. Auto-import of classes from the source directory (`MissingClassImporter`, optional) +### 3. Property rename (`PropertyRenameOperation`, optional) + +Only runs if the `renameProperties` flag is enabled (off by default — see [configuration.md](../configuration.md#phpnamespacerefactorrenameproperties)). Only acts when the class itself was actually renamed (`oldClassName !== newClassName`); a plain move to a different directory with the same class name is a no-op. + +1. Derives the expected old/new property names from the class names (`PropertyNameResolver`, e.g. `Teste` → `teste`) +2. Builds the candidate file list: the moved file itself plus every file returned by `WorkspaceIndex.execute()` (the same workspace-wide file listing used elsewhere, respecting `ignoredDirectories`/`additionalExtensions`) +3. For each candidate file whose text contains the new class name, `ClassTypedPropertyLocator` looks for the constructor property typed as that class — promoted or not, readonly or not, confirming a non-promoted parameter by checking for a `$this->x = $x;` assignment in the constructor body (see `ConstructorSpanFinder` for how the constructor's parameter list and body are located via brace/paren matching) +4. If more than one property shares that type in the same file, it's ambiguous — the file is skipped entirely rather than guessing +5. If exactly one property is found: it's renamed when its current name already matches the *old* class-name convention, or — only when `renameProperties.renameMismatchedNames` is also enabled — regardless of what it was named before +6. Renaming rewrites the constructor parameter/property declaration and every `$this->x` usage in that file, accumulated into a single `WorkspaceEdit` (its own undo stop, separate from `MultiFileReferenceUpdater`'s) + +### 4. Auto-import of classes from the source directory (`MissingClassImporter`, optional) Only runs if the `autoImportNamespace` flag is enabled. Lists the `.php` files still left in the source directory, checks which classes from those files are used in the moved file's text but not imported, and inserts the corresponding `use` statements. -### 4. Removing stale imports (`ImportRemover`) +### 5. Removing stale imports (`ImportRemover`) Unlike the other flags, the `removeUnusedImports` check happens **inside** `ImportRemover` itself (not in `FileMoveOperation`) — so it's always called, but returns immediately if the flag is disabled. @@ -78,8 +90,10 @@ When enabled: it collects the class names declared in the other files of the mov | Flag | Behavior | |---|---| -| `autoImportNamespace` | Enables step 3 (auto-import of classes from the old directory) | -| `removeUnusedImports` | Enables the import removal in step 4 (checked inside `ImportRemover`) | +| `renameProperties` | Enables step 3 (renaming class-typed constructor properties to match the new class name) | +| `renameProperties.renameMismatchedNames` | Extends step 3 to also rename properties whose name doesn't already match the class-name convention | +| `autoImportNamespace` | Enables step 4 (auto-import of classes from the old directory) | +| `removeUnusedImports` | Enables the import removal in step 5 (checked inside `ImportRemover`) | | `editFilesInBackground` | Doesn't change what's edited, only whether touched files open a tab in the editor or are saved silently — see [configuration.md](../configuration.md) | ## Error handling @@ -90,6 +104,7 @@ Each file in the batch is processed inside its own `try/catch` in `FileMoveOpera - `DirectoryMovedFilesResolver` — expands directory moves into per-file moves - `NamespaceBatchUpdater` — orchestrates the namespace, class name, and reference update +- `PropertyRenameOperation` — renames class-typed constructor properties (and their `$this->x` usages) to match a renamed class - `MissingClassImporter` — detects and injects missing imports - `ImportRemover` — removes unused imports - `FeatureFlagManager` — checks which features are enabled in the settings From dd4b706168119f45eb633455d775ebfa23b163e0 Mon Sep 17 00:00:00 2001 From: Rejman Nascimento Date: Wed, 29 Jul 2026 23:43:05 -0300 Subject: [PATCH 5/7] refactor(property-rename)!: rename property flag and scope renames to affected files rename the master setting to `renameProperties.enabled` so it can coexist with nested options in VS Code's schema. also limit property renaming to files already affected by the class rename, which avoids unrelated workspace-wide matches and keeps the operation aligned with the namespace update that triggered it. BREAKING CHANGE: the configuration key is now `phpNamespaceRefactor.renameProperties.enabled` instead of `phpNamespaceRefactor.renameProperties` --- README.md | 6 +- docs/configuration.md | 14 ++-- docs/operations/file-move.md | 10 +-- package.json | 4 +- src/app/operations/FileMoveOperation.ts | 4 +- src/app/operations/PropertyRenameOperation.ts | 23 ++++--- src/app/services/NamespaceBatchUpdater.ts | 8 +-- .../update/MultiFileReferenceUpdater.ts | 4 +- .../property/ClassTypedPropertyLocator.ts | 6 +- .../property/PropertyDeclarationPattern.ts | 16 +++++ src/domain/workspace/ConfigurationLocator.ts | 6 +- src/test/ClassTypedPropertyLocator.test.ts | 23 +++++++ src/test/PropertyRenameOperation.test.ts | 68 ++++++++++++++----- 13 files changed, 139 insertions(+), 53 deletions(-) create mode 100644 src/domain/property/PropertyDeclarationPattern.ts diff --git a/README.md b/README.md index 0387167..146977f 100644 --- a/README.md +++ b/README.md @@ -50,7 +50,7 @@ This extension contributes the following settings: ], "phpNamespaceRefactor.rename": true, "phpNamespaceRefactor.editFilesInBackground": true, - "phpNamespaceRefactor.renameProperties": false, + "phpNamespaceRefactor.renameProperties.enabled": false, "phpNamespaceRefactor.renameProperties.renameMismatchedNames": false } ``` @@ -95,7 +95,7 @@ This extension contributes the following settings: - Default: true. -**phpNamespaceRefactor.renameProperties** +**phpNamespaceRefactor.renameProperties.enabled** - When a class is renamed, also renames its class-typed constructor properties (promoted or not, readonly or not) and every `$this->x` usage to match the new class name — e.g. `private Teste $teste` becomes `private Novo $novo` when `Teste` is renamed to `Novo`. - If more than one property shares the same type in a constructor, the file is skipped rather than guessing which one to rename. @@ -104,7 +104,7 @@ This extension contributes the following settings: **phpNamespaceRefactor.renameProperties.renameMismatchedNames** -- Only applies when `renameProperties` is enabled. By default, only properties whose name already matches the old class name are renamed. Enable this to also rename properties named differently from their type (e.g. `private Teste $service` becomes `private Novo $novo`). +- Only applies when `renameProperties.enabled` is true. By default, only properties whose name already matches the old class name are renamed. Enable this to also rename properties named differently from their type (e.g. `private Teste $service` becomes `private Novo $novo`). - Default: false. diff --git a/docs/configuration.md b/docs/configuration.md index 3d28948..6ae737c 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -4,7 +4,7 @@ Reference for every setting contributed by the extension (`phpNamespaceRefactor. All keys are centralized in `ConfigKeys` (`src/domain/workspace/ConfigurationLocator.ts`). No other place in the codebase references a setting name as a loose string — if a key needs renaming, this is the only spot to change (besides `package.json`). -The one deliberate exception is `PropertyRenameConfigKeys` (`src/domain/property/PropertyRenameConfigKeys.ts`): `renameProperties.renameMismatchedNames` is a sub-setting of the `renameProperties` feature only, not a global flag, so it's kept in its own small constant instead of `ConfigKeys` — see [`phpNamespaceRefactor.renameProperties`](#phpnamespacerefactorrenameproperties) below. +The one deliberate exception is `PropertyRenameConfigKeys` (`src/domain/property/PropertyRenameConfigKeys.ts`): `renameProperties.renameMismatchedNames` is a sub-setting of the `renameProperties` feature only, not a global flag, so it's kept in its own small constant instead of `ConfigKeys` — see [`phpNamespaceRefactor.renameProperties.enabled`](#phpnamespacerefactorrenamepropertiesenabled) below. Note that `renameProperties` itself is **not** a standalone setting — see why in that section. | Setting | Key (`ConfigKeys`) | Type | Default | Read by | |---|---|---|---|---| @@ -14,7 +14,7 @@ The one deliberate exception is `PropertyRenameConfigKeys` (`src/domain/property | `phpNamespaceRefactor.additionalExtensions` | `ADDITIONAL_EXTENSIONS` | `string[]` | `["php"]` | `FileExtensionResolver` and `WorkspaceIndex` | | `phpNamespaceRefactor.rename` | `RENAME` | `boolean` | `true` | The `phpNamespaceRefactor.rename` command (`extension.ts`) and the F2 keybinding's `when` clause (`package.json`) | | `phpNamespaceRefactor.editFilesInBackground` | `EDIT_FILES_IN_BACKGROUND` | `boolean` | `true` | `FileEditApplier` | -| `phpNamespaceRefactor.renameProperties` | `ConfigKeys.RENAME_PROPERTIES` | `boolean` | `false` | `FileMoveOperation` (decides whether to call `PropertyRenameOperation`) | +| `phpNamespaceRefactor.renameProperties.enabled` | `ConfigKeys.RENAME_PROPERTIES` | `boolean` | `false` | `FileMoveOperation` (decides whether to call `PropertyRenameOperation`) | | `phpNamespaceRefactor.renameProperties.renameMismatchedNames` | `PropertyRenameConfigKeys.RENAME_MISMATCHED_NAMES` | `boolean` | `false` | `PropertyRenameOperation` | ## How configuration is read @@ -22,7 +22,7 @@ The one deliberate exception is `PropertyRenameConfigKeys` (`src/domain/property Two classes access `workspace.getConfiguration('phpNamespaceRefactor')`, each with a distinct purpose: - **`ConfigurationLocator`** (`src/domain/workspace/ConfigurationLocator.ts`) — generic read, used for settings of any type (`ignoredDirectories`, `additionalExtensions`) -- **`FeatureFlagManager`** (`src/domain/workspace/FeatureFlagManager.ts`) — specialized `boolean` read, with `defaultValue = true`. Used for every on/off flag (`autoImportNamespace`, `removeUnusedImports`, `rename`, `editFilesInBackground`). `renameProperties` also goes through it, but `FileMoveOperation` passes an explicit `defaultValue: false` to flip the usual default, since this flag is opt-in +- **`FeatureFlagManager`** (`src/domain/workspace/FeatureFlagManager.ts`) — specialized `boolean` read, with `defaultValue = true`. Used for every on/off flag (`autoImportNamespace`, `removeUnusedImports`, `rename`, `editFilesInBackground`). `renameProperties.enabled` also goes through it, but `FileMoveOperation` passes an explicit `defaultValue: false` to flip the usual default, since this flag is opt-in Neither class caches the `WorkspaceConfiguration` — each instance reads `workspace.getConfiguration()` in its constructor, and since both are `@injectable()` (not singleton), a fresh read happens on every `container.resolve()`. This means a change to the user's configuration is picked up on the next operation, with no need to reload the window. @@ -43,15 +43,17 @@ Filters files by simple substring match against `fsPath` (not a glob) — see `W Normalized by `normalizeExtensions()` (`src/infra/utils/extensions.ts`): strips leading dots and whitespace, lowercases, and always guarantees `php` is included even if not listed. `FileExtensionResolver.match()` sorts extensions from longest to shortest before comparing, so a compound extension (e.g. `class.php`) is never shadowed by the plain `php` entry. -## `phpNamespaceRefactor.renameProperties` +## `phpNamespaceRefactor.renameProperties.enabled` -Master switch for `PropertyRenameOperation` — off by default, unlike the extension's other flags, because it renames identifiers (constructor parameters, promoted/non-promoted properties, `$this->x` usages), not just type hints. When a class is renamed and this is enabled, `FileMoveOperation` calls `PropertyRenameOperation` right after `NamespaceBatchUpdater`, so it only ever acts on properties whose type hint was just updated to the new class name. +Master switch for `PropertyRenameOperation` — off by default, unlike the extension's other flags, because it renames identifiers (constructor parameters, promoted/non-promoted properties, `$this->x` usages), not just type hints. Two conditions gate it: the class itself must have actually been renamed (`oldClassName !== newClassName` — a plain directory move is a no-op), and only files `NamespaceBatchUpdater`/`MultiFileReferenceUpdater` already determined were affected by that specific rename are ever touched, never an independent workspace-wide scan — so an unrelated class in another namespace that happens to share a short name is never at risk. For each candidate file, `ClassTypedPropertyLocator` looks for the single constructor property typed as the renamed class (promoted or not, readonly or not); if more than one property shares that type, the file is skipped entirely rather than guessing which one to rename — see [file-move.md](./operations/file-move.md#3-property-rename-propertyrenameoperation-optional). +**Why `.enabled` and not a bare `renameProperties` boolean:** VS Code's settings schema doesn't allow a key to be both a leaf value and the parent of another key. An earlier version declared the master flag as plain `phpNamespaceRefactor.renameProperties` (boolean) alongside `phpNamespaceRefactor.renameProperties.renameMismatchedNames` — VS Code detected the conflict, logged `Ignoring phpNamespaceRefactor.renameProperties.renameMismatchedNames as phpNamespaceRefactor.renameProperties is false` in the console, and silently resolved **both** settings to `false` regardless of what the user configured. `renameProperties` is now purely a namespace prefix; `.enabled` and `.renameMismatchedNames` are its only two leaves. + ### `phpNamespaceRefactor.renameProperties.renameMismatchedNames` -Only has an effect when `renameProperties` is also enabled. Controls what happens when the property's current name doesn't already follow the class-name convention: +Only has an effect when `renameProperties.enabled` is also `true`. Controls what happens when the property's current name doesn't already follow the class-name convention: - `false` (default) — a property is only renamed if its name already matches the *old* class name (e.g. `$teste` for `Teste`); a property named something else on purpose (e.g. `$service`) is left untouched - `true` — mismatched names are renamed too, to match the *new* class name (e.g. `$service` → `$novo`) diff --git a/docs/operations/file-move.md b/docs/operations/file-move.md index d725008..9e664a3 100644 --- a/docs/operations/file-move.md +++ b/docs/operations/file-move.md @@ -47,6 +47,8 @@ For each moved file, `src/app/services/NamespaceBatchUpdater.ts`: 5. If the namespace declaration wasn't found/changed (step 4 returns `false`), the operation stops here — there's nothing to propagate for a file with no `namespace` declaration 6. Otherwise, calls `MultiFileReferenceUpdater` to propagate the change across the rest of the project +`NamespaceBatchUpdater.execute()` returns the list of files `MultiFileReferenceUpdater` determined were affected (empty when it never ran, e.g. no `namespace` declaration to key off). `FileMoveOperation` forwards that same list into step 3 below, so property renaming only ever touches files this specific rename actually reached — never an independent, broader scan. + #### `MultiFileReferenceUpdater` — how affected files are found Combines two sources, without relying on either alone: @@ -67,11 +69,11 @@ At the end, `MultiFileReferenceUpdater` always calls `ImportRemover.execute({ ur ### 3. Property rename (`PropertyRenameOperation`, optional) -Only runs if the `renameProperties` flag is enabled (off by default — see [configuration.md](../configuration.md#phpnamespacerefactorrenameproperties)). Only acts when the class itself was actually renamed (`oldClassName !== newClassName`); a plain move to a different directory with the same class name is a no-op. +Only runs if the `renameProperties.enabled` flag is on (off by default — see [configuration.md](../configuration.md#phpnamespacerefactorrenamepropertiesenabled)). Only acts when the class itself was actually renamed (`oldClassName !== newClassName`); a plain move to a different directory with the same class name is a no-op. 1. Derives the expected old/new property names from the class names (`PropertyNameResolver`, e.g. `Teste` → `teste`) -2. Builds the candidate file list: the moved file itself plus every file returned by `WorkspaceIndex.execute()` (the same workspace-wide file listing used elsewhere, respecting `ignoredDirectories`/`additionalExtensions`) -3. For each candidate file whose text contains the new class name, `ClassTypedPropertyLocator` looks for the constructor property typed as that class — promoted or not, readonly or not, confirming a non-promoted parameter by checking for a `$this->x = $x;` assignment in the constructor body (see `ConstructorSpanFinder` for how the constructor's parameter list and body are located via brace/paren matching) +2. Builds the candidate file list from the `affectedFiles` `FileMoveOperation` received back from `NamespaceBatchUpdater` (step 2's output), plus the moved file itself — deliberately **not** an independent workspace-wide scan, so a differently-namespaced class that happens to share a short name is never touched +3. For each candidate file whose text contains the new class name, `ClassTypedPropertyLocator` looks for the constructor property typed as that class — promoted or not, readonly or not, confirming a non-promoted parameter by checking for a `$this->x = $x;` assignment in the constructor body (see `ConstructorSpanFinder` for how the constructor's parameter list and body are located via brace/paren matching). The property's own declaration line doesn't need a type hint to be found this way — a legacy `private $x;` typed only via a `@var ClassName` docblock is still matched and renamed, via `PropertyDeclarationPattern`, once the constructor param already confirmed what class it holds 4. If more than one property shares that type in the same file, it's ambiguous — the file is skipped entirely rather than guessing 5. If exactly one property is found: it's renamed when its current name already matches the *old* class-name convention, or — only when `renameProperties.renameMismatchedNames` is also enabled — regardless of what it was named before 6. Renaming rewrites the constructor parameter/property declaration and every `$this->x` usage in that file, accumulated into a single `WorkspaceEdit` (its own undo stop, separate from `MultiFileReferenceUpdater`'s) @@ -90,7 +92,7 @@ When enabled: it collects the class names declared in the other files of the mov | Flag | Behavior | |---|---| -| `renameProperties` | Enables step 3 (renaming class-typed constructor properties to match the new class name) | +| `renameProperties.enabled` | Enables step 3 (renaming class-typed constructor properties to match the new class name) | | `renameProperties.renameMismatchedNames` | Extends step 3 to also rename properties whose name doesn't already match the class-name convention | | `autoImportNamespace` | Enables step 4 (auto-import of classes from the old directory) | | `removeUnusedImports` | Enables the import removal in step 5 (checked inside `ImportRemover`) | diff --git a/package.json b/package.json index d127d9f..b89cfcd 100644 --- a/package.json +++ b/package.json @@ -110,7 +110,7 @@ "default": true, "description": "Apply refactor edits to files without opening them, keeping only files that were already open in the editor. Disable to have every edited file opened in the editor as before." }, - "phpNamespaceRefactor.renameProperties": { + "phpNamespaceRefactor.renameProperties.enabled": { "type": "boolean", "default": false, "description": "Rename constructor-typed properties (promoted or not, readonly or not) to match the class name when renaming a class." @@ -118,7 +118,7 @@ "phpNamespaceRefactor.renameProperties.renameMismatchedNames": { "type": "boolean", "default": false, - "description": "Also rename properties whose current name doesn't match the class name (e.g. $service for a Teste type). Only applies when renameProperties is enabled." + "description": "Also rename properties whose current name doesn't match the class name (e.g. $service for a Teste type). Only applies when renameProperties.enabled is true." } } } diff --git a/src/app/operations/FileMoveOperation.ts b/src/app/operations/FileMoveOperation.ts index caa2681..a4dd042 100644 --- a/src/app/operations/FileMoveOperation.ts +++ b/src/app/operations/FileMoveOperation.ts @@ -29,10 +29,10 @@ export class FileMoveOperation { } try { - await this.namespaceBatchUpdater.execute({ newUri, oldUri }); + const affectedFiles = await this.namespaceBatchUpdater.execute({ newUri, oldUri }); if (this.featureFlagManager.isActive({ key: ConfigKeys.RENAME_PROPERTIES, defaultValue: false })) { - await this.propertyRenameOperation.execute({ oldUri, newUri }); + await this.propertyRenameOperation.execute({ oldUri, newUri, affectedFiles }); } if (this.featureFlagManager.isActive({ key: ConfigKeys.AUTO_IMPORT_NAMESPACE })) { diff --git a/src/app/operations/PropertyRenameOperation.ts b/src/app/operations/PropertyRenameOperation.ts index cd3a192..53153b5 100644 --- a/src/app/operations/PropertyRenameOperation.ts +++ b/src/app/operations/PropertyRenameOperation.ts @@ -1,10 +1,10 @@ import { ClassTypedPropertyLocator, PropertyMatch } from '@domain/property/ClassTypedPropertyLocator'; import { ConstructorSpanFinder } from '@domain/property/ConstructorSpanFinder'; +import { buildPropertyDeclarationPattern } from '@domain/property/PropertyDeclarationPattern'; import { PropertyNameResolver } from '@domain/property/PropertyNameResolver'; import { PropertyRenameConfigKeys } from '@domain/property/PropertyRenameConfigKeys'; import { ConfigurationLocator } from '@domain/workspace/ConfigurationLocator'; import { WorkspacePathResolver } from '@domain/workspace/WorkspacePathResolver'; -import { WorkspaceIndex } from '@infra/index/WorkspaceIndex'; import { FileEditApplier } from '@infra/vscode/FileEditApplier'; import { TextDocumentOpener } from '@infra/vscode/TextDocumentOpener'; import { inject, injectable } from 'tsyringe'; @@ -13,13 +13,13 @@ import { Range, TextDocument, Uri, WorkspaceEdit } from 'vscode'; interface Props { oldUri: Uri newUri: Uri + affectedFiles: Uri[] } @injectable() export class PropertyRenameOperation { constructor( @inject(WorkspacePathResolver) private workspacePathResolver: WorkspacePathResolver, - @inject(WorkspaceIndex) private workspaceIndex: WorkspaceIndex, @inject(TextDocumentOpener) private textDocumentOpener: TextDocumentOpener, @inject(FileEditApplier) private fileEditApplier: FileEditApplier, @inject(ConfigurationLocator) private configurationLocator: ConfigurationLocator, @@ -28,7 +28,7 @@ export class PropertyRenameOperation { @inject(ConstructorSpanFinder) private constructorSpanFinder: ConstructorSpanFinder, ) {} - public async execute({ oldUri, newUri }: Props): Promise { + public async execute({ oldUri, newUri, affectedFiles }: Props): Promise { const oldClassName = this.workspacePathResolver.extractClassNameFromPath(oldUri.fsPath); const newClassName = this.workspacePathResolver.extractClassNameFromPath(newUri.fsPath); @@ -44,7 +44,7 @@ export class PropertyRenameOperation { defaultValue: false, }); - const files = await this.getCandidateFiles(newUri); + const files = this.getCandidateFiles(newUri, affectedFiles); const edit = new WorkspaceEdit(); await Promise.all(files.map(async (file) => { @@ -119,15 +119,18 @@ export class PropertyRenameOperation { } private findDeclarationSpan(text: string, className: string, propertyName: string): [number, number] | null { - const pattern = new RegExp( - `(?:public|protected|private)\\s+(?:readonly\\s+)?\\??\\b${className}\\b\\s+\\$${propertyName}\\s*;`, - ); - const match = pattern.exec(text); + const match = buildPropertyDeclarationPattern(className, propertyName).exec(text); return match ? [match.index, match.index + match[0].length] : null; } - private async getCandidateFiles(newUri: Uri): Promise { - const files = [newUri, ...await this.workspaceIndex.execute()]; + /** + * Only the files MultiFileReferenceUpdater already determined were + * affected by this exact class rename (plus the renamed file itself) - + * never a broader workspace scan, so a differently-namespaced class that + * happens to share a short name is never touched. + */ + private getCandidateFiles(newUri: Uri, affectedFiles: Uri[]): Uri[] { + const files = [newUri, ...affectedFiles]; const seen = new Set(); return files.filter((file) => { diff --git a/src/app/services/NamespaceBatchUpdater.ts b/src/app/services/NamespaceBatchUpdater.ts index efddd27..4c3a5b2 100644 --- a/src/app/services/NamespaceBatchUpdater.ts +++ b/src/app/services/NamespaceBatchUpdater.ts @@ -20,11 +20,11 @@ export class NamespaceBatchUpdater { @inject(ClassNameUpdater) private classNameUpdater: ClassNameUpdater, ) {} - public async execute({ newUri, oldUri }: Props) { + public async execute({ newUri, oldUri }: Props): Promise { const { namespace, fullNamespace } = await this.getNamespace(newUri); if (!namespace) { - return; + return []; } const { namespace: old, fullNamespace: oldFullNamespace } = await this.getNamespace(oldUri); @@ -39,10 +39,10 @@ export class NamespaceBatchUpdater { }); if (!isUpdated) { - return; + return []; } - await this.multiFileReferenceUpdater.execute({ + return await this.multiFileReferenceUpdater.execute({ useOldNamespace: oldFullNamespace, useNewNamespace: fullNamespace, newUri, diff --git a/src/app/services/update/MultiFileReferenceUpdater.ts b/src/app/services/update/MultiFileReferenceUpdater.ts index f24ab9d..adf098c 100644 --- a/src/app/services/update/MultiFileReferenceUpdater.ts +++ b/src/app/services/update/MultiFileReferenceUpdater.ts @@ -41,7 +41,7 @@ export class MultiFileReferenceUpdater { useNewNamespace, newUri, oldUri, - }: Props) { + }: Props): Promise { const directoryPath = this.workspacePathResolver.extractDirectoryFromPath(oldUri.fsPath); const className = this.workspacePathResolver.extractClassNameFromPath(oldUri.fsPath); const newClassName = this.workspacePathResolver.extractClassNameFromPath(newUri.fsPath); @@ -117,6 +117,8 @@ export class MultiFileReferenceUpdater { await this.fileEditApplier.apply(edit); await this.importRemover.execute({ uri: newUri }); + + return [...affectedPaths.map(fsPath => Uri.file(fsPath)), ...sameDirectoryFiles]; } /** diff --git a/src/domain/property/ClassTypedPropertyLocator.ts b/src/domain/property/ClassTypedPropertyLocator.ts index 7a58aa4..0b39828 100644 --- a/src/domain/property/ClassTypedPropertyLocator.ts +++ b/src/domain/property/ClassTypedPropertyLocator.ts @@ -1,6 +1,7 @@ import { inject, injectable } from 'tsyringe'; import { ConstructorSpanFinder } from './ConstructorSpanFinder'; +import { buildPropertyDeclarationPattern } from './PropertyDeclarationPattern'; const VISIBILITY = 'public|protected|private'; @@ -45,10 +46,7 @@ export class ClassTypedPropertyLocator { } private hasPropertyDeclaration(text: string, className: string, varName: string): boolean { - const pattern = new RegExp( - `(?:${VISIBILITY})\\s+(?:readonly\\s+)?\\??\\b${className}\\b\\s+\\$${varName}\\s*;`, - ); - return pattern.test(text); + return buildPropertyDeclarationPattern(className, varName).test(text); } private isAssignedToThis(constructorBody: string, varName: string): boolean { diff --git a/src/domain/property/PropertyDeclarationPattern.ts b/src/domain/property/PropertyDeclarationPattern.ts new file mode 100644 index 0000000..bbfb898 --- /dev/null +++ b/src/domain/property/PropertyDeclarationPattern.ts @@ -0,0 +1,16 @@ +const VISIBILITY = 'public|protected|private'; + +/** + * Matches a class-body property declaration for `varName` - e.g. + * `private Teste $teste;` or, since the type hint is optional in PHP, + * a legacy `private $teste;` typed only via a `@var Teste` docblock. + * `className` is accepted but not required, so a property whose type was + * never declared in code (only documented) is still found once the caller + * has already confirmed by other means (e.g. a constructor assignment) + * that it holds an instance of that class. + */ +export function buildPropertyDeclarationPattern(className: string, varName: string): RegExp { + return new RegExp( + `(?:${VISIBILITY})\\s+(?:readonly\\s+)?(?:\\??\\b${className}\\b\\s+)?\\$${varName}\\s*;`, + ); +} diff --git a/src/domain/workspace/ConfigurationLocator.ts b/src/domain/workspace/ConfigurationLocator.ts index 437f1f9..720af5a 100644 --- a/src/domain/workspace/ConfigurationLocator.ts +++ b/src/domain/workspace/ConfigurationLocator.ts @@ -10,7 +10,11 @@ export const ConfigKeys = { ADDITIONAL_EXTENSIONS: 'additionalExtensions', RENAME: 'rename', EDIT_FILES_IN_BACKGROUND: 'editFilesInBackground', - RENAME_PROPERTIES: 'renameProperties', + // Namespaced under "renameProperties.*" (not the bare "renameProperties") because + // VS Code's settings schema can't have a key be both a leaf boolean and the parent + // of another setting (PropertyRenameConfigKeys.RENAME_MISMATCHED_NAMES) at once - + // doing so silently drops both values instead of erroring. + RENAME_PROPERTIES: 'renameProperties.enabled', } as const; export type Props = { diff --git a/src/test/ClassTypedPropertyLocator.test.ts b/src/test/ClassTypedPropertyLocator.test.ts index e859674..2a9e260 100644 --- a/src/test/ClassTypedPropertyLocator.test.ts +++ b/src/test/ClassTypedPropertyLocator.test.ts @@ -56,6 +56,29 @@ suite('ClassTypedPropertyLocator', () => { assert.strictEqual(match!.hasSeparateDeclaration, true); }); + test('finds an untyped property declared only via a @var docblock', () => { + const text = [ + 'class UserService', + '{', + ' /**', + ' * @var UserRepository', + ' */', + ' private $repository;', + '', + ' public function __construct(UserRepository $repository)', + ' {', + ' $this->repository = $repository;', + ' }', + '}', + ].join('\n'); + + const match = locate(text, 'UserRepository'); + assert.ok(match); + assert.strictEqual(match!.propertyName, 'repository'); + assert.strictEqual(match!.isPromoted, false); + assert.strictEqual(match!.hasSeparateDeclaration, true); + }); + test('ignores a non-promoted parameter that is never stored on $this', () => { const text = [ 'class Validator', diff --git a/src/test/PropertyRenameOperation.test.ts b/src/test/PropertyRenameOperation.test.ts index e005126..2df4b2f 100644 --- a/src/test/PropertyRenameOperation.test.ts +++ b/src/test/PropertyRenameOperation.test.ts @@ -15,7 +15,6 @@ import { FeatureFlagManager } from '../domain/workspace/FeatureFlagManager'; import { FileExtensionResolver } from '../domain/workspace/FileExtensionResolver'; import { WorkspacePathResolver } from '../domain/workspace/WorkspacePathResolver'; import { ComposerAutoloadManager } from '../infra/autoload/ComposerAutoloadManager'; -import { WorkspaceIndex } from '../infra/index/WorkspaceIndex'; import { FileEditApplier } from '../infra/vscode/FileEditApplier'; import { TextDocumentOpener } from '../infra/vscode/TextDocumentOpener'; @@ -38,7 +37,6 @@ function fakeFeatureFlagManager(editFilesInBackground = true): FeatureFlagManage } function buildOperation({ - files = [] as vscode.Uri[], renameMismatched = false, editFilesInBackground = true, } = {}): PropertyRenameOperation { @@ -46,13 +44,11 @@ function buildOperation({ new ComposerAutoloadManager(), new FileExtensionResolver(fakePassthroughConfigurationLocator()), ); - const workspaceIndex = { execute: async () => files } as unknown as WorkspaceIndex; const fileEditApplier = new FileEditApplier(fakeFeatureFlagManager(editFilesInBackground)); const constructorSpanFinder = new ConstructorSpanFinder(); return new PropertyRenameOperation( workspacePathResolver, - workspaceIndex, new TextDocumentOpener(), fileEditApplier, fakeRenameMismatchConfigurationLocator(renameMismatched), @@ -77,8 +73,8 @@ suite('PropertyRenameOperation', () => { const consumerContent = 'teste->run();\n }\n}\n'; const consumerUri = await writeTempPhpFile(dir, 'UserController.php', consumerContent); - const operation = buildOperation({ files: [consumerUri] }); - await operation.execute({ oldUri, newUri }); + const operation = buildOperation(); + await operation.execute({ oldUri, newUri, affectedFiles: [consumerUri] }); const text = (await vscode.workspace.openTextDocument(consumerUri)).getText(); assert.ok(text.includes('private Novo $novo'), `expected the promoted property to be renamed, got:\n${text}`); @@ -94,8 +90,8 @@ suite('PropertyRenameOperation', () => { const consumerContent = 'teste = $teste;\n }\n}\n'; const consumerUri = await writeTempPhpFile(dir, 'UserController.php', consumerContent); - const operation = buildOperation({ files: [consumerUri] }); - await operation.execute({ oldUri, newUri }); + const operation = buildOperation(); + await operation.execute({ oldUri, newUri, affectedFiles: [consumerUri] }); const text = (await vscode.workspace.openTextDocument(consumerUri)).getText(); assert.ok(text.includes('private Novo $novo;'), `expected the declared property to be renamed, got:\n${text}`); @@ -103,6 +99,24 @@ suite('PropertyRenameOperation', () => { assert.ok(text.includes('$this->novo = $novo;'), `expected the assignment to be renamed, got:\n${text}`); }); + test('renames an untyped property declared only via a @var docblock', async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'php-namespace-refactor-')); + const oldUri = vscode.Uri.file(path.join(dir, 'UserRepository.php')); + const newUri = vscode.Uri.file(path.join(dir, 'ClienteRepository.php')); + + const consumerContent = 'repository = $repository;\n }\n}\n'; + const consumerUri = await writeTempPhpFile(dir, 'UserService.php', consumerContent); + + const operation = buildOperation({ renameMismatched: true }); + await operation.execute({ oldUri, newUri, affectedFiles: [consumerUri] }); + + const text = (await vscode.workspace.openTextDocument(consumerUri)).getText(); + assert.ok(text.includes('private $clienteRepository;'), `expected the untyped declaration to be renamed, got:\n${text}`); + assert.ok(text.includes('__construct(ClienteRepository $clienteRepository)'), `expected the constructor parameter to be renamed, got:\n${text}`); + assert.ok(text.includes('$this->clienteRepository = $clienteRepository;'), `expected the assignment to be renamed, got:\n${text}`); + assert.ok(!text.includes('$repository'), `expected no leftover old property name, got:\n${text}`); + }); + test('leaves a mismatched property name untouched when the sub-flag is off', async () => { const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'php-namespace-refactor-')); const oldUri = vscode.Uri.file(path.join(dir, 'Teste.php')); @@ -111,8 +125,8 @@ suite('PropertyRenameOperation', () => { const consumerContent = ' { const consumerContent = 'service->run();\n }\n}\n'; const consumerUri = await writeTempPhpFile(dir, 'UserController.php', consumerContent); - const operation = buildOperation({ files: [consumerUri], renameMismatched: true }); - await operation.execute({ oldUri, newUri }); + const operation = buildOperation({ renameMismatched: true }); + await operation.execute({ oldUri, newUri, affectedFiles: [consumerUri] }); const text = (await vscode.workspace.openTextDocument(consumerUri)).getText(); assert.ok(text.includes('private Novo $novo'), `expected the mismatched property to be renamed, got:\n${text}`); @@ -142,8 +156,8 @@ suite('PropertyRenameOperation', () => { const consumerContent = ' { const consumerContent = ' { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'php-namespace-refactor-')); + const oldUri = vscode.Uri.file(path.join(dir, 'Teste.php')); + const newUri = vscode.Uri.file(path.join(dir, 'Novo.php')); + + const unrelatedContent = ' Date: Thu, 30 Jul 2026 00:10:09 -0300 Subject: [PATCH 6/7] feat(property-rename)!: consolidate property rename settings into one polymorphic key Replace the split enabled/mismatch flags with a single renameProperties setting that can be read as boolean or object. This avoids the VS Code schema conflict while preserving the same behavior through a resolver-driven flow. BREAKING CHANGE: renameProperties.enabled and renameProperties.renameMismatchedNames were removed in favor of phpNamespaceRefactor.renameProperties --- README.md | 21 ++++--- docs/configuration.md | 31 ++++++---- docs/operations/file-move.md | 8 +-- package.json | 19 +++--- src/app/operations/FileMoveOperation.ts | 12 +++- src/app/operations/PropertyRenameOperation.ts | 13 +---- .../property/PropertyRenameConfigKeys.ts | 3 - .../PropertyRenameSettingsResolver.ts | 35 +++++++++++ src/domain/workspace/ConfigurationLocator.ts | 9 ++- src/test/PropertyRenameOperation.test.ts | 36 +++++------- .../PropertyRenameSettingsResolver.test.ts | 58 +++++++++++++++++++ 11 files changed, 171 insertions(+), 74 deletions(-) delete mode 100644 src/domain/property/PropertyRenameConfigKeys.ts create mode 100644 src/domain/property/PropertyRenameSettingsResolver.ts create mode 100644 src/test/PropertyRenameSettingsResolver.test.ts diff --git a/README.md b/README.md index 146977f..c3668b6 100644 --- a/README.md +++ b/README.md @@ -50,8 +50,7 @@ This extension contributes the following settings: ], "phpNamespaceRefactor.rename": true, "phpNamespaceRefactor.editFilesInBackground": true, - "phpNamespaceRefactor.renameProperties.enabled": false, - "phpNamespaceRefactor.renameProperties.renameMismatchedNames": false + "phpNamespaceRefactor.renameProperties": false } ``` @@ -95,16 +94,20 @@ This extension contributes the following settings: - Default: true. -**phpNamespaceRefactor.renameProperties.enabled** +**phpNamespaceRefactor.renameProperties** - When a class is renamed, also renames its class-typed constructor properties (promoted or not, readonly or not) and every `$this->x` usage to match the new class name — e.g. `private Teste $teste` becomes `private Novo $novo` when `Teste` is renamed to `Novo`. - If more than one property shares the same type in a constructor, the file is skipped rather than guessing which one to rename. - -- Default: false. - -**phpNamespaceRefactor.renameProperties.renameMismatchedNames** - -- Only applies when `renameProperties.enabled` is true. By default, only properties whose name already matches the old class name are renamed. Enable this to also rename properties named differently from their type (e.g. `private Teste $service` becomes `private Novo $novo`). +- Accepts either a boolean or an object: + ```json + "phpNamespaceRefactor.renameProperties": true + ``` + ```json + "phpNamespaceRefactor.renameProperties": { + "renameMismatchedNames": true + } + ``` + Setting an object automatically enables the feature. `renameMismatchedNames` (default `false`) additionally renames properties whose current name doesn't already match the class name on purpose (e.g. `private Teste $service` becomes `private Novo $novo`); without it, only properties already named after the old class are renamed. - Default: false. diff --git a/docs/configuration.md b/docs/configuration.md index 6ae737c..5b8e7f6 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -4,8 +4,6 @@ Reference for every setting contributed by the extension (`phpNamespaceRefactor. All keys are centralized in `ConfigKeys` (`src/domain/workspace/ConfigurationLocator.ts`). No other place in the codebase references a setting name as a loose string — if a key needs renaming, this is the only spot to change (besides `package.json`). -The one deliberate exception is `PropertyRenameConfigKeys` (`src/domain/property/PropertyRenameConfigKeys.ts`): `renameProperties.renameMismatchedNames` is a sub-setting of the `renameProperties` feature only, not a global flag, so it's kept in its own small constant instead of `ConfigKeys` — see [`phpNamespaceRefactor.renameProperties.enabled`](#phpnamespacerefactorrenamepropertiesenabled) below. Note that `renameProperties` itself is **not** a standalone setting — see why in that section. - | Setting | Key (`ConfigKeys`) | Type | Default | Read by | |---|---|---|---|---| | `phpNamespaceRefactor.ignoredDirectories` | `IGNORED_DIRECTORIES` | `string[]` | `["/vendor/", "/var/", "/cache/"]` | `WorkspaceIndex` (filters the workspace file scan) | @@ -14,15 +12,15 @@ The one deliberate exception is `PropertyRenameConfigKeys` (`src/domain/property | `phpNamespaceRefactor.additionalExtensions` | `ADDITIONAL_EXTENSIONS` | `string[]` | `["php"]` | `FileExtensionResolver` and `WorkspaceIndex` | | `phpNamespaceRefactor.rename` | `RENAME` | `boolean` | `true` | The `phpNamespaceRefactor.rename` command (`extension.ts`) and the F2 keybinding's `when` clause (`package.json`) | | `phpNamespaceRefactor.editFilesInBackground` | `EDIT_FILES_IN_BACKGROUND` | `boolean` | `true` | `FileEditApplier` | -| `phpNamespaceRefactor.renameProperties.enabled` | `ConfigKeys.RENAME_PROPERTIES` | `boolean` | `false` | `FileMoveOperation` (decides whether to call `PropertyRenameOperation`) | -| `phpNamespaceRefactor.renameProperties.renameMismatchedNames` | `PropertyRenameConfigKeys.RENAME_MISMATCHED_NAMES` | `boolean` | `false` | `PropertyRenameOperation` | +| `phpNamespaceRefactor.renameProperties` | `RENAME_PROPERTIES` | `boolean \| { renameMismatchedNames?: boolean }` | `false` | `PropertyRenameSettingsResolver`, consumed by `FileMoveOperation`/`PropertyRenameOperation` | ## How configuration is read -Two classes access `workspace.getConfiguration('phpNamespaceRefactor')`, each with a distinct purpose: +Three classes access `workspace.getConfiguration('phpNamespaceRefactor')`, each with a distinct purpose: - **`ConfigurationLocator`** (`src/domain/workspace/ConfigurationLocator.ts`) — generic read, used for settings of any type (`ignoredDirectories`, `additionalExtensions`) -- **`FeatureFlagManager`** (`src/domain/workspace/FeatureFlagManager.ts`) — specialized `boolean` read, with `defaultValue = true`. Used for every on/off flag (`autoImportNamespace`, `removeUnusedImports`, `rename`, `editFilesInBackground`). `renameProperties.enabled` also goes through it, but `FileMoveOperation` passes an explicit `defaultValue: false` to flip the usual default, since this flag is opt-in +- **`FeatureFlagManager`** (`src/domain/workspace/FeatureFlagManager.ts`) — specialized `boolean` read, with `defaultValue = true`. Used for every plain on/off flag (`autoImportNamespace`, `removeUnusedImports`, `rename`, `editFilesInBackground`) +- **`PropertyRenameSettingsResolver`** (`src/domain/property/PropertyRenameSettingsResolver.ts`) — the one setting whose raw value isn't a plain boolean; see [`phpNamespaceRefactor.renameProperties`](#phpnamespacerefactorrenameproperties) below Neither class caches the `WorkspaceConfiguration` — each instance reads `workspace.getConfiguration()` in its constructor, and since both are `@injectable()` (not singleton), a fresh read happens on every `container.resolve()`. This means a change to the user's configuration is picked up on the next operation, with no need to reload the window. @@ -43,17 +41,28 @@ Filters files by simple substring match against `fsPath` (not a glob) — see `W Normalized by `normalizeExtensions()` (`src/infra/utils/extensions.ts`): strips leading dots and whitespace, lowercases, and always guarantees `php` is included even if not listed. `FileExtensionResolver.match()` sorts extensions from longest to shortest before comparing, so a compound extension (e.g. `class.php`) is never shadowed by the plain `php` entry. -## `phpNamespaceRefactor.renameProperties.enabled` +## `phpNamespaceRefactor.renameProperties` Master switch for `PropertyRenameOperation` — off by default, unlike the extension's other flags, because it renames identifiers (constructor parameters, promoted/non-promoted properties, `$this->x` usages), not just type hints. Two conditions gate it: the class itself must have actually been renamed (`oldClassName !== newClassName` — a plain directory move is a no-op), and only files `NamespaceBatchUpdater`/`MultiFileReferenceUpdater` already determined were affected by that specific rename are ever touched, never an independent workspace-wide scan — so an unrelated class in another namespace that happens to share a short name is never at risk. For each candidate file, `ClassTypedPropertyLocator` looks for the single constructor property typed as the renamed class (promoted or not, readonly or not); if more than one property shares that type, the file is skipped entirely rather than guessing which one to rename — see [file-move.md](./operations/file-move.md#3-property-rename-propertyrenameoperation-optional). -**Why `.enabled` and not a bare `renameProperties` boolean:** VS Code's settings schema doesn't allow a key to be both a leaf value and the parent of another key. An earlier version declared the master flag as plain `phpNamespaceRefactor.renameProperties` (boolean) alongside `phpNamespaceRefactor.renameProperties.renameMismatchedNames` — VS Code detected the conflict, logged `Ignoring phpNamespaceRefactor.renameProperties.renameMismatchedNames as phpNamespaceRefactor.renameProperties is false` in the console, and silently resolved **both** settings to `false` regardless of what the user configured. `renameProperties` is now purely a namespace prefix; `.enabled` and `.renameMismatchedNames` are its only two leaves. +### Accepted values + +This one setting doubles as its own sub-option, via `PropertyRenameSettingsResolver`: -### `phpNamespaceRefactor.renameProperties.renameMismatchedNames` +```jsonc +"phpNamespaceRefactor.renameProperties": false // disabled (default) +"phpNamespaceRefactor.renameProperties": true // enabled, mismatched names left alone +"phpNamespaceRefactor.renameProperties": {} // enabled, mismatched names left alone +"phpNamespaceRefactor.renameProperties": { "renameMismatchedNames": true } // enabled, mismatched names renamed too +``` -Only has an effect when `renameProperties.enabled` is also `true`. Controls what happens when the property's current name doesn't already follow the class-name convention: +`renameMismatchedNames` controls what happens when a property's current name doesn't already follow the class-name convention: -- `false` (default) — a property is only renamed if its name already matches the *old* class name (e.g. `$teste` for `Teste`); a property named something else on purpose (e.g. `$service`) is left untouched +- unset/`false` (default) — a property is only renamed if its name already matches the *old* class name (e.g. `$teste` for `Teste`); a property named something else on purpose (e.g. `$service`) is left untouched - `true` — mismatched names are renamed too, to match the *new* class name (e.g. `$service` → `$novo`) + +Any object value implies the feature is enabled — `false` is the only way to turn it off; there's no `{ "enabled": false }` form. + +**Why one polymorphic setting instead of two plain booleans:** VS Code's settings schema doesn't allow a key to be both a leaf value and the parent of another key. An earlier version declared `phpNamespaceRefactor.renameProperties` (boolean) alongside `phpNamespaceRefactor.renameProperties.renameMismatchedNames` (boolean) as two separate keys — VS Code detected the conflict, logged `Ignoring phpNamespaceRefactor.renameProperties.renameMismatchedNames as phpNamespaceRefactor.renameProperties is false` in the console, and silently resolved **both** settings to `false` regardless of what the user configured. Collapsing them into a single `boolean | object` setting sidesteps the conflict entirely, since there's only ever one registered key. diff --git a/docs/operations/file-move.md b/docs/operations/file-move.md index 9e664a3..b139f11 100644 --- a/docs/operations/file-move.md +++ b/docs/operations/file-move.md @@ -69,13 +69,13 @@ At the end, `MultiFileReferenceUpdater` always calls `ImportRemover.execute({ ur ### 3. Property rename (`PropertyRenameOperation`, optional) -Only runs if the `renameProperties.enabled` flag is on (off by default — see [configuration.md](../configuration.md#phpnamespacerefactorrenamepropertiesenabled)). Only acts when the class itself was actually renamed (`oldClassName !== newClassName`); a plain move to a different directory with the same class name is a no-op. +Only runs if `renameProperties` resolves to enabled (off by default — see [configuration.md](../configuration.md#phpnamespacerefactorrenameproperties)). `PropertyRenameSettingsResolver` reads the raw setting (`boolean | { renameMismatchedNames?: boolean }`) once in `FileMoveOperation` and turns it into `{ enabled, renameMismatchedNames }`; only `enabled` gates whether this step runs at all. Also only acts when the class itself was actually renamed (`oldClassName !== newClassName`); a plain move to a different directory with the same class name is a no-op. 1. Derives the expected old/new property names from the class names (`PropertyNameResolver`, e.g. `Teste` → `teste`) 2. Builds the candidate file list from the `affectedFiles` `FileMoveOperation` received back from `NamespaceBatchUpdater` (step 2's output), plus the moved file itself — deliberately **not** an independent workspace-wide scan, so a differently-namespaced class that happens to share a short name is never touched 3. For each candidate file whose text contains the new class name, `ClassTypedPropertyLocator` looks for the constructor property typed as that class — promoted or not, readonly or not, confirming a non-promoted parameter by checking for a `$this->x = $x;` assignment in the constructor body (see `ConstructorSpanFinder` for how the constructor's parameter list and body are located via brace/paren matching). The property's own declaration line doesn't need a type hint to be found this way — a legacy `private $x;` typed only via a `@var ClassName` docblock is still matched and renamed, via `PropertyDeclarationPattern`, once the constructor param already confirmed what class it holds 4. If more than one property shares that type in the same file, it's ambiguous — the file is skipped entirely rather than guessing -5. If exactly one property is found: it's renamed when its current name already matches the *old* class-name convention, or — only when `renameProperties.renameMismatchedNames` is also enabled — regardless of what it was named before +5. If exactly one property is found: it's renamed when its current name already matches the *old* class-name convention, or — only when `renameMismatchedNames` also resolved to `true` — regardless of what it was named before 6. Renaming rewrites the constructor parameter/property declaration and every `$this->x` usage in that file, accumulated into a single `WorkspaceEdit` (its own undo stop, separate from `MultiFileReferenceUpdater`'s) ### 4. Auto-import of classes from the source directory (`MissingClassImporter`, optional) @@ -92,8 +92,8 @@ When enabled: it collects the class names declared in the other files of the mov | Flag | Behavior | |---|---| -| `renameProperties.enabled` | Enables step 3 (renaming class-typed constructor properties to match the new class name) | -| `renameProperties.renameMismatchedNames` | Extends step 3 to also rename properties whose name doesn't already match the class-name convention | +| `renameProperties` (`true`/`{}`) | Enables step 3 (renaming class-typed constructor properties to match the new class name) | +| `renameProperties: { renameMismatchedNames: true }` | Extends step 3 to also rename properties whose name doesn't already match the class-name convention | | `autoImportNamespace` | Enables step 4 (auto-import of classes from the old directory) | | `removeUnusedImports` | Enables the import removal in step 5 (checked inside `ImportRemover`) | | `editFilesInBackground` | Doesn't change what's edited, only whether touched files open a tab in the editor or are saved silently — see [configuration.md](../configuration.md) | diff --git a/package.json b/package.json index b89cfcd..1182fd3 100644 --- a/package.json +++ b/package.json @@ -110,15 +110,18 @@ "default": true, "description": "Apply refactor edits to files without opening them, keeping only files that were already open in the editor. Disable to have every edited file opened in the editor as before." }, - "phpNamespaceRefactor.renameProperties.enabled": { - "type": "boolean", + "phpNamespaceRefactor.renameProperties": { + "type": ["boolean", "object"], "default": false, - "description": "Rename constructor-typed properties (promoted or not, readonly or not) to match the class name when renaming a class." - }, - "phpNamespaceRefactor.renameProperties.renameMismatchedNames": { - "type": "boolean", - "default": false, - "description": "Also rename properties whose current name doesn't match the class name (e.g. $service for a Teste type). Only applies when renameProperties.enabled is true." + "properties": { + "renameMismatchedNames": { + "type": "boolean", + "default": false, + "description": "Also rename properties whose current name doesn't match the class name (e.g. $service for a Teste type)." + } + }, + "additionalProperties": false, + "description": "Rename constructor-typed properties (promoted or not, readonly or not) - and their $this->x usages - to match the class name when renaming a class. Set to true/false to toggle, or to an object like { \"renameMismatchedNames\": true } to also rename properties whose current name doesn't match the class." } } } diff --git a/src/app/operations/FileMoveOperation.ts b/src/app/operations/FileMoveOperation.ts index a4dd042..6418aaa 100644 --- a/src/app/operations/FileMoveOperation.ts +++ b/src/app/operations/FileMoveOperation.ts @@ -2,6 +2,7 @@ import { DirectoryMovedFilesResolver } from '@app/services/DirectoryMovedFilesRe import { MissingClassImporter } from '@app/services/MissingClassImporter'; import { NamespaceBatchUpdater } from '@app/services/NamespaceBatchUpdater'; import { ImportRemover } from '@app/services/remove/ImportRemover'; +import { PropertyRenameSettingsResolver } from '@domain/property/PropertyRenameSettingsResolver'; import { ConfigKeys } from '@domain/workspace/ConfigurationLocator'; import { FeatureFlagManager } from '@domain/workspace/FeatureFlagManager'; import { inject, injectable } from 'tsyringe'; @@ -18,6 +19,7 @@ export class FileMoveOperation { @inject(ImportRemover) private importRemover: ImportRemover, @inject(FeatureFlagManager) private featureFlagManager: FeatureFlagManager, @inject(PropertyRenameOperation) private propertyRenameOperation: PropertyRenameOperation, + @inject(PropertyRenameSettingsResolver) private propertyRenameSettingsResolver: PropertyRenameSettingsResolver, ) {} public async execute(files: ReadonlyArray): Promise { @@ -31,8 +33,14 @@ export class FileMoveOperation { try { const affectedFiles = await this.namespaceBatchUpdater.execute({ newUri, oldUri }); - if (this.featureFlagManager.isActive({ key: ConfigKeys.RENAME_PROPERTIES, defaultValue: false })) { - await this.propertyRenameOperation.execute({ oldUri, newUri, affectedFiles }); + const propertyRenameSettings = this.propertyRenameSettingsResolver.resolve(); + if (propertyRenameSettings.enabled) { + await this.propertyRenameOperation.execute({ + oldUri, + newUri, + affectedFiles, + renameMismatchedNames: propertyRenameSettings.renameMismatchedNames, + }); } if (this.featureFlagManager.isActive({ key: ConfigKeys.AUTO_IMPORT_NAMESPACE })) { diff --git a/src/app/operations/PropertyRenameOperation.ts b/src/app/operations/PropertyRenameOperation.ts index 53153b5..654baa6 100644 --- a/src/app/operations/PropertyRenameOperation.ts +++ b/src/app/operations/PropertyRenameOperation.ts @@ -2,8 +2,6 @@ import { ClassTypedPropertyLocator, PropertyMatch } from '@domain/property/Class import { ConstructorSpanFinder } from '@domain/property/ConstructorSpanFinder'; import { buildPropertyDeclarationPattern } from '@domain/property/PropertyDeclarationPattern'; import { PropertyNameResolver } from '@domain/property/PropertyNameResolver'; -import { PropertyRenameConfigKeys } from '@domain/property/PropertyRenameConfigKeys'; -import { ConfigurationLocator } from '@domain/workspace/ConfigurationLocator'; import { WorkspacePathResolver } from '@domain/workspace/WorkspacePathResolver'; import { FileEditApplier } from '@infra/vscode/FileEditApplier'; import { TextDocumentOpener } from '@infra/vscode/TextDocumentOpener'; @@ -14,6 +12,7 @@ interface Props { oldUri: Uri newUri: Uri affectedFiles: Uri[] + renameMismatchedNames: boolean } @injectable() @@ -22,13 +21,12 @@ export class PropertyRenameOperation { @inject(WorkspacePathResolver) private workspacePathResolver: WorkspacePathResolver, @inject(TextDocumentOpener) private textDocumentOpener: TextDocumentOpener, @inject(FileEditApplier) private fileEditApplier: FileEditApplier, - @inject(ConfigurationLocator) private configurationLocator: ConfigurationLocator, @inject(ClassTypedPropertyLocator) private classTypedPropertyLocator: ClassTypedPropertyLocator, @inject(PropertyNameResolver) private propertyNameResolver: PropertyNameResolver, @inject(ConstructorSpanFinder) private constructorSpanFinder: ConstructorSpanFinder, ) {} - public async execute({ oldUri, newUri, affectedFiles }: Props): Promise { + public async execute({ oldUri, newUri, affectedFiles, renameMismatchedNames }: Props): Promise { const oldClassName = this.workspacePathResolver.extractClassNameFromPath(oldUri.fsPath); const newClassName = this.workspacePathResolver.extractClassNameFromPath(newUri.fsPath); @@ -39,11 +37,6 @@ export class PropertyRenameOperation { const expectedOldName = this.propertyNameResolver.resolve(oldClassName); const expectedNewName = this.propertyNameResolver.resolve(newClassName); - const renameMismatched = this.configurationLocator.get({ - key: PropertyRenameConfigKeys.RENAME_MISMATCHED_NAMES, - defaultValue: false, - }); - const files = this.getCandidateFiles(newUri, affectedFiles); const edit = new WorkspaceEdit(); @@ -60,7 +53,7 @@ export class PropertyRenameOperation { } const matchesOldConvention = match.propertyName === expectedOldName; - if (!matchesOldConvention && !renameMismatched) { + if (!matchesOldConvention && !renameMismatchedNames) { return; } diff --git a/src/domain/property/PropertyRenameConfigKeys.ts b/src/domain/property/PropertyRenameConfigKeys.ts deleted file mode 100644 index 740996b..0000000 --- a/src/domain/property/PropertyRenameConfigKeys.ts +++ /dev/null @@ -1,3 +0,0 @@ -export const PropertyRenameConfigKeys = { - RENAME_MISMATCHED_NAMES: 'renameProperties.renameMismatchedNames', -} as const; diff --git a/src/domain/property/PropertyRenameSettingsResolver.ts b/src/domain/property/PropertyRenameSettingsResolver.ts new file mode 100644 index 0000000..3d04c79 --- /dev/null +++ b/src/domain/property/PropertyRenameSettingsResolver.ts @@ -0,0 +1,35 @@ +import { ConfigKeys, ConfigurationLocator } from '@domain/workspace/ConfigurationLocator'; +import { inject, injectable } from 'tsyringe'; + +export interface PropertyRenameSettings { + enabled: boolean + renameMismatchedNames: boolean +} + +type RenamePropertiesValue = boolean | { renameMismatchedNames?: boolean }; + +/** + * `phpNamespaceRefactor.renameProperties` is a single setting that accepts + * either a boolean or an object (`{ renameMismatchedNames: boolean }`) - + * any object value implies the feature is enabled, since a bare boolean + * `false` is the only way to turn it off. + */ +@injectable() +export class PropertyRenameSettingsResolver { + constructor( + @inject(ConfigurationLocator) private configurationLocator: ConfigurationLocator, + ) {} + + public resolve(): PropertyRenameSettings { + const value = this.configurationLocator.get({ + key: ConfigKeys.RENAME_PROPERTIES, + defaultValue: false, + }); + + if (typeof value === 'object' && value !== null) { + return { enabled: true, renameMismatchedNames: value.renameMismatchedNames === true }; + } + + return { enabled: value === true, renameMismatchedNames: false }; + } +} diff --git a/src/domain/workspace/ConfigurationLocator.ts b/src/domain/workspace/ConfigurationLocator.ts index 720af5a..78010c3 100644 --- a/src/domain/workspace/ConfigurationLocator.ts +++ b/src/domain/workspace/ConfigurationLocator.ts @@ -10,11 +10,10 @@ export const ConfigKeys = { ADDITIONAL_EXTENSIONS: 'additionalExtensions', RENAME: 'rename', EDIT_FILES_IN_BACKGROUND: 'editFilesInBackground', - // Namespaced under "renameProperties.*" (not the bare "renameProperties") because - // VS Code's settings schema can't have a key be both a leaf boolean and the parent - // of another setting (PropertyRenameConfigKeys.RENAME_MISMATCHED_NAMES) at once - - // doing so silently drops both values instead of erroring. - RENAME_PROPERTIES: 'renameProperties.enabled', + // Value is boolean|object (see PropertyRenameSettingsResolver) rather than a plain + // boolean - a single polymorphic key avoids VS Code's settings schema conflict that + // comes from one key being both a leaf boolean and the parent of another setting. + RENAME_PROPERTIES: 'renameProperties', } as const; export type Props = { diff --git a/src/test/PropertyRenameOperation.test.ts b/src/test/PropertyRenameOperation.test.ts index 2df4b2f..ca83e93 100644 --- a/src/test/PropertyRenameOperation.test.ts +++ b/src/test/PropertyRenameOperation.test.ts @@ -24,12 +24,6 @@ function fakePassthroughConfigurationLocator(): ConfigurationLocator { } as ConfigurationLocator; } -function fakeRenameMismatchConfigurationLocator(renameMismatched: boolean): ConfigurationLocator { - return { - get: (): T => renameMismatched as unknown as T, - } as unknown as ConfigurationLocator; -} - function fakeFeatureFlagManager(editFilesInBackground = true): FeatureFlagManager { return { isActive: ({ defaultValue = true }) => defaultValue && editFilesInBackground, @@ -37,7 +31,6 @@ function fakeFeatureFlagManager(editFilesInBackground = true): FeatureFlagManage } function buildOperation({ - renameMismatched = false, editFilesInBackground = true, } = {}): PropertyRenameOperation { const workspacePathResolver = new WorkspacePathResolver( @@ -51,7 +44,6 @@ function buildOperation({ workspacePathResolver, new TextDocumentOpener(), fileEditApplier, - fakeRenameMismatchConfigurationLocator(renameMismatched), new ClassTypedPropertyLocator(constructorSpanFinder), new PropertyNameResolver(), constructorSpanFinder, @@ -74,7 +66,7 @@ suite('PropertyRenameOperation', () => { const consumerUri = await writeTempPhpFile(dir, 'UserController.php', consumerContent); const operation = buildOperation(); - await operation.execute({ oldUri, newUri, affectedFiles: [consumerUri] }); + await operation.execute({ oldUri, newUri, affectedFiles: [consumerUri], renameMismatchedNames: false }); const text = (await vscode.workspace.openTextDocument(consumerUri)).getText(); assert.ok(text.includes('private Novo $novo'), `expected the promoted property to be renamed, got:\n${text}`); @@ -91,7 +83,7 @@ suite('PropertyRenameOperation', () => { const consumerUri = await writeTempPhpFile(dir, 'UserController.php', consumerContent); const operation = buildOperation(); - await operation.execute({ oldUri, newUri, affectedFiles: [consumerUri] }); + await operation.execute({ oldUri, newUri, affectedFiles: [consumerUri], renameMismatchedNames: false }); const text = (await vscode.workspace.openTextDocument(consumerUri)).getText(); assert.ok(text.includes('private Novo $novo;'), `expected the declared property to be renamed, got:\n${text}`); @@ -107,8 +99,8 @@ suite('PropertyRenameOperation', () => { const consumerContent = 'repository = $repository;\n }\n}\n'; const consumerUri = await writeTempPhpFile(dir, 'UserService.php', consumerContent); - const operation = buildOperation({ renameMismatched: true }); - await operation.execute({ oldUri, newUri, affectedFiles: [consumerUri] }); + const operation = buildOperation(); + await operation.execute({ oldUri, newUri, affectedFiles: [consumerUri], renameMismatchedNames: true }); const text = (await vscode.workspace.openTextDocument(consumerUri)).getText(); assert.ok(text.includes('private $clienteRepository;'), `expected the untyped declaration to be renamed, got:\n${text}`); @@ -117,7 +109,7 @@ suite('PropertyRenameOperation', () => { assert.ok(!text.includes('$repository'), `expected no leftover old property name, got:\n${text}`); }); - test('leaves a mismatched property name untouched when the sub-flag is off', async () => { + test('leaves a mismatched property name untouched when renameMismatchedNames is false', async () => { const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'php-namespace-refactor-')); const oldUri = vscode.Uri.file(path.join(dir, 'Teste.php')); const newUri = vscode.Uri.file(path.join(dir, 'Novo.php')); @@ -125,14 +117,14 @@ suite('PropertyRenameOperation', () => { const consumerContent = ' { + test('renames a mismatched property name when renameMismatchedNames is true', async () => { const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'php-namespace-refactor-')); const oldUri = vscode.Uri.file(path.join(dir, 'Teste.php')); const newUri = vscode.Uri.file(path.join(dir, 'Novo.php')); @@ -140,8 +132,8 @@ suite('PropertyRenameOperation', () => { const consumerContent = 'service->run();\n }\n}\n'; const consumerUri = await writeTempPhpFile(dir, 'UserController.php', consumerContent); - const operation = buildOperation({ renameMismatched: true }); - await operation.execute({ oldUri, newUri, affectedFiles: [consumerUri] }); + const operation = buildOperation(); + await operation.execute({ oldUri, newUri, affectedFiles: [consumerUri], renameMismatchedNames: true }); const text = (await vscode.workspace.openTextDocument(consumerUri)).getText(); assert.ok(text.includes('private Novo $novo'), `expected the mismatched property to be renamed, got:\n${text}`); @@ -156,8 +148,8 @@ suite('PropertyRenameOperation', () => { const consumerContent = ' { const consumerUri = await writeTempPhpFile(dir, 'UserController.php', consumerContent); const operation = buildOperation(); - await operation.execute({ oldUri: sameUri, newUri: sameUri, affectedFiles: [consumerUri] }); + await operation.execute({ oldUri: sameUri, newUri: sameUri, affectedFiles: [consumerUri], renameMismatchedNames: false }); const text = (await vscode.workspace.openTextDocument(consumerUri)).getText(); assert.strictEqual(text, consumerContent, `expected no changes, got:\n${text}`); @@ -193,7 +185,7 @@ suite('PropertyRenameOperation', () => { const unrelatedUri = await writeTempPhpFile(dir, 'UnrelatedController.php', unrelatedContent); const operation = buildOperation(); - await operation.execute({ oldUri, newUri, affectedFiles: [] }); + await operation.execute({ oldUri, newUri, affectedFiles: [], renameMismatchedNames: false }); const text = (await vscode.workspace.openTextDocument(unrelatedUri)).getText(); assert.strictEqual(text, unrelatedContent, `expected the unaffected file to be left untouched, got:\n${text}`); diff --git a/src/test/PropertyRenameSettingsResolver.test.ts b/src/test/PropertyRenameSettingsResolver.test.ts new file mode 100644 index 0000000..0278b9b --- /dev/null +++ b/src/test/PropertyRenameSettingsResolver.test.ts @@ -0,0 +1,58 @@ +import 'reflect-metadata'; + +import * as assert from 'assert'; + +import { PropertyRenameSettingsResolver } from '../domain/property/PropertyRenameSettingsResolver'; +import { ConfigurationLocator } from '../domain/workspace/ConfigurationLocator'; + +function buildResolver(rawValue: unknown): PropertyRenameSettingsResolver { + const configurationLocator = { + get: () => rawValue, + } as unknown as ConfigurationLocator; + + return new PropertyRenameSettingsResolver(configurationLocator); +} + +suite('PropertyRenameSettingsResolver', () => { + test('resolves false to disabled', () => { + assert.deepStrictEqual( + buildResolver(false).resolve(), + { enabled: false, renameMismatchedNames: false }, + ); + }); + + test('resolves undefined (unset) to disabled', () => { + assert.deepStrictEqual( + buildResolver(undefined).resolve(), + { enabled: false, renameMismatchedNames: false }, + ); + }); + + test('resolves true to enabled, without the mismatch behavior', () => { + assert.deepStrictEqual( + buildResolver(true).resolve(), + { enabled: true, renameMismatchedNames: false }, + ); + }); + + test('resolves an empty object to enabled, without the mismatch behavior', () => { + assert.deepStrictEqual( + buildResolver({}).resolve(), + { enabled: true, renameMismatchedNames: false }, + ); + }); + + test('resolves { renameMismatchedNames: true } to enabled with the mismatch behavior on', () => { + assert.deepStrictEqual( + buildResolver({ renameMismatchedNames: true }).resolve(), + { enabled: true, renameMismatchedNames: true }, + ); + }); + + test('resolves { renameMismatchedNames: false } to enabled with the mismatch behavior off', () => { + assert.deepStrictEqual( + buildResolver({ renameMismatchedNames: false }).resolve(), + { enabled: true, renameMismatchedNames: false }, + ); + }); +}); From a22d3abe7121eec57f55370aab0ef98f54b808a7 Mon Sep 17 00:00:00 2001 From: Rejman Nascimento Date: Thu, 30 Jul 2026 00:15:26 -0300 Subject: [PATCH 7/7] docs(docs): clarify config reads and class rename behavior Document that the configuration resolvers do not cache workspace configuration so updates are observed on the next operation. Also note that class renames may optionally rename matching constructor properties when property renaming is enabled. --- README.md | 4 ++-- docs/configuration.md | 2 +- docs/operations/class-rename.md | 1 + src/domain/property/PropertyDeclarationPattern.ts | 2 +- src/test/ClassTypedPropertyLocator.test.ts | 10 +++++----- src/test/PropertyRenameOperation.test.ts | 8 ++++---- 6 files changed, 14 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index c3668b6..07ecd38 100644 --- a/README.md +++ b/README.md @@ -96,7 +96,7 @@ This extension contributes the following settings: **phpNamespaceRefactor.renameProperties** -- When a class is renamed, also renames its class-typed constructor properties (promoted or not, readonly or not) and every `$this->x` usage to match the new class name — e.g. `private Teste $teste` becomes `private Novo $novo` when `Teste` is renamed to `Novo`. +- When a class is renamed, also renames its class-typed constructor properties (promoted or not, readonly or not) and every `$this->x` usage to match the new class name — e.g. `private Test $teste` becomes `private NewTest $newTest` when `Teste` is renamed to `Novo`. - If more than one property shares the same type in a constructor, the file is skipped rather than guessing which one to rename. - Accepts either a boolean or an object: ```json @@ -107,7 +107,7 @@ This extension contributes the following settings: "renameMismatchedNames": true } ``` - Setting an object automatically enables the feature. `renameMismatchedNames` (default `false`) additionally renames properties whose current name doesn't already match the class name on purpose (e.g. `private Teste $service` becomes `private Novo $novo`); without it, only properties already named after the old class are renamed. + Setting an object automatically enables the feature. `renameMismatchedNames` (default `false`) additionally renames properties whose current name doesn't already match the class name on purpose (e.g. `private Test $service` becomes `private NewTest $newTest`); without it, only properties already named after the old class are renamed. - Default: false. diff --git a/docs/configuration.md b/docs/configuration.md index 5b8e7f6..1c2ab71 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -22,7 +22,7 @@ Three classes access `workspace.getConfiguration('phpNamespaceRefactor')`, each - **`FeatureFlagManager`** (`src/domain/workspace/FeatureFlagManager.ts`) — specialized `boolean` read, with `defaultValue = true`. Used for every plain on/off flag (`autoImportNamespace`, `removeUnusedImports`, `rename`, `editFilesInBackground`) - **`PropertyRenameSettingsResolver`** (`src/domain/property/PropertyRenameSettingsResolver.ts`) — the one setting whose raw value isn't a plain boolean; see [`phpNamespaceRefactor.renameProperties`](#phpnamespacerefactorrenameproperties) below -Neither class caches the `WorkspaceConfiguration` — each instance reads `workspace.getConfiguration()` in its constructor, and since both are `@injectable()` (not singleton), a fresh read happens on every `container.resolve()`. This means a change to the user's configuration is picked up on the next operation, with no need to reload the window. +None of the three caches the `WorkspaceConfiguration` — `ConfigurationLocator`/`FeatureFlagManager` read `workspace.getConfiguration()` in their constructor, and `PropertyRenameSettingsResolver` reads through a fresh `ConfigurationLocator` on every `resolve()` call. All three are `@injectable()` (not singleton), so a fresh read happens on every `container.resolve()`. This means a change to the user's configuration is picked up on the next operation, with no need to reload the window. ## `phpNamespaceRefactor.editFilesInBackground` diff --git a/docs/operations/class-rename.md b/docs/operations/class-rename.md index 42068a9..d0b3afa 100644 --- a/docs/operations/class-rename.md +++ b/docs/operations/class-rename.md @@ -31,6 +31,7 @@ It then delegates the rename to `FileRenameHandler.create()`, which triggers VS - Update the `namespace` declaration in the file - Update the class name inside the file (via `ClassNameUpdater`) - Update every `use` statement referencing the class throughout the project +- Optionally, rename class-typed constructor properties (and their `$this->x` usages) in the affected files to match the new class name — only when `renameProperties` is enabled, see `PropertyRenameOperation` in [file-move.md](./file-move.md#3-property-rename-propertyrenameoperation-optional) ## Difference from a direct Explorer rename diff --git a/src/domain/property/PropertyDeclarationPattern.ts b/src/domain/property/PropertyDeclarationPattern.ts index bbfb898..f03454d 100644 --- a/src/domain/property/PropertyDeclarationPattern.ts +++ b/src/domain/property/PropertyDeclarationPattern.ts @@ -2,7 +2,7 @@ const VISIBILITY = 'public|protected|private'; /** * Matches a class-body property declaration for `varName` - e.g. - * `private Teste $teste;` or, since the type hint is optional in PHP, + * `private Test $teste;` or, since the type hint is optional in PHP, * a legacy `private $teste;` typed only via a `@var Teste` docblock. * `className` is accepted but not required, so a property whose type was * never declared in code (only documented) is still found once the caller diff --git a/src/test/ClassTypedPropertyLocator.test.ts b/src/test/ClassTypedPropertyLocator.test.ts index 2a9e260..2da4d42 100644 --- a/src/test/ClassTypedPropertyLocator.test.ts +++ b/src/test/ClassTypedPropertyLocator.test.ts @@ -15,7 +15,7 @@ suite('ClassTypedPropertyLocator', () => { const text = [ 'class UserController', '{', - ' public function __construct(private Teste $teste)', + ' public function __construct(private Test $teste)', ' {', ' }', '}', @@ -30,7 +30,7 @@ suite('ClassTypedPropertyLocator', () => { test('finds a promoted readonly property regardless of modifier order', () => { const first = locate('function __construct(private readonly Teste $teste) {}', 'Teste'); - const second = locate('function __construct(readonly private Teste $teste) {}', 'Teste'); + const second = locate('function __construct(readonly private Test $teste) {}', 'Teste'); assert.strictEqual(first!.propertyName, 'teste'); assert.strictEqual(second!.propertyName, 'teste'); @@ -40,7 +40,7 @@ suite('ClassTypedPropertyLocator', () => { const text = [ 'class UserController', '{', - ' private Teste $teste;', + ' private Test $teste;', '', ' public function __construct(Teste $teste)', ' {', @@ -94,7 +94,7 @@ suite('ClassTypedPropertyLocator', () => { }); test('matches a property with a mismatched name', () => { - const text = 'function __construct(private Teste $service) {}'; + const text = 'function __construct(private Test $service) {}'; const match = locate(text, 'Teste'); assert.ok(match); @@ -119,7 +119,7 @@ suite('ClassTypedPropertyLocator', () => { }); test('returns null when two parameters share the same type (ambiguous)', () => { - const text = 'function __construct(private Teste $a, private Teste $b) {}'; + const text = 'function __construct(private Test $a, private Test $b) {}'; assert.strictEqual(locate(text, 'Teste'), null); }); }); diff --git a/src/test/PropertyRenameOperation.test.ts b/src/test/PropertyRenameOperation.test.ts index ca83e93..4507bdb 100644 --- a/src/test/PropertyRenameOperation.test.ts +++ b/src/test/PropertyRenameOperation.test.ts @@ -69,7 +69,7 @@ suite('PropertyRenameOperation', () => { await operation.execute({ oldUri, newUri, affectedFiles: [consumerUri], renameMismatchedNames: false }); const text = (await vscode.workspace.openTextDocument(consumerUri)).getText(); - assert.ok(text.includes('private Novo $novo'), `expected the promoted property to be renamed, got:\n${text}`); + assert.ok(text.includes('private NewTest $newTest'), `expected the promoted property to be renamed, got:\n${text}`); assert.ok(text.includes('$this->novo->run();'), `expected $this-> usages to be renamed, got:\n${text}`); assert.ok(!text.includes('teste'), `expected no leftover old property name, got:\n${text}`); }); @@ -86,7 +86,7 @@ suite('PropertyRenameOperation', () => { await operation.execute({ oldUri, newUri, affectedFiles: [consumerUri], renameMismatchedNames: false }); const text = (await vscode.workspace.openTextDocument(consumerUri)).getText(); - assert.ok(text.includes('private Novo $novo;'), `expected the declared property to be renamed, got:\n${text}`); + assert.ok(text.includes('private NewTest $newTest;'), `expected the declared property to be renamed, got:\n${text}`); assert.ok(text.includes('__construct(Novo $novo)'), `expected the constructor parameter to be renamed, got:\n${text}`); assert.ok(text.includes('$this->novo = $novo;'), `expected the assignment to be renamed, got:\n${text}`); }); @@ -136,7 +136,7 @@ suite('PropertyRenameOperation', () => { await operation.execute({ oldUri, newUri, affectedFiles: [consumerUri], renameMismatchedNames: true }); const text = (await vscode.workspace.openTextDocument(consumerUri)).getText(); - assert.ok(text.includes('private Novo $novo'), `expected the mismatched property to be renamed, got:\n${text}`); + assert.ok(text.includes('private NewTest $newTest'), `expected the mismatched property to be renamed, got:\n${text}`); assert.ok(text.includes('$this->novo->run();'), `expected $this-> usages to be renamed, got:\n${text}`); }); @@ -159,7 +159,7 @@ suite('PropertyRenameOperation', () => { const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'php-namespace-refactor-')); const sameUri = vscode.Uri.file(path.join(dir, 'Teste.php')); - const consumerContent = '