From c731f16c97d27b3a23535521d7326516392336a7 Mon Sep 17 00:00:00 2001 From: Akshat Date: Sun, 30 Aug 2026 12:11:16 +0530 Subject: [PATCH] fix(templates): make Services.php + its reset() wiring accessor-conditional (#16, #17) - Services.php now ships only when at least one module registers a Services accessor. A zero-accessor build (e.g. --modules shortcode) no longer carries a dead class with nothing but set()/reset() over a permanently empty array. - Plugin_TestCase's `use Services;` import and `Services::reset()` call in tearDown() are gated on the same has_services flag, via the existing {{#if flag}} template engine. - Services_Test's generated accessor test now covers the default construction path (assertInstanceOf on a bare call, before any set()) in addition to the memoisation, set()-override, and reset()-clears-the-override paths already covered. - Update the CLI's own test suite to match: the "foundational classes" test no longer expects src/Services.php in a zero-module scaffold, and the test-isolation test (#14, #15) asserts the has_services split and the rewritten Services_Test body. Closes re-audit #3 items #16 and #17. --- index.js | 47 +++++++++++++++--------- templates/tests/Unit/Plugin_TestCase.php | 12 ++++-- tests/generator.test.js | 26 +++++++++---- 3 files changed, 57 insertions(+), 28 deletions(-) diff --git a/index.js b/index.js index fded6eb..4570754 100644 --- a/index.js +++ b/index.js @@ -825,6 +825,10 @@ function scaffoldInto(answers, targetDir) { has_woo: hasAnyWoo, // The only modules that write a templates/ directory (WC template overrides). has_wc_template_overrides: hasWooEmail || hasWooMyAccount, + // Set from servicesAccessors after the module loop: when no module + // registers an accessor, Services.php (and its reset() wiring in + // Plugin_TestCase) is not generated. + has_services: false, lint_wp_org: lintTarget === 'wp-org' || lintTarget === 'both', lint_vip: needsVip }; @@ -958,7 +962,7 @@ function scaffoldInto(answers, targetDir) { writeTemplateFile(path.join(templatesDir, 'phpcs.xml'), 'phpcs.xml'); writeTemplateFile(path.join(templatesDir, 'tests/bootstrap.php'), 'tests/bootstrap.php'); writeTemplateFile(path.join(templatesDir, 'phpunit.xml.dist'), 'phpunit.xml.dist'); - writeTemplateFile(path.join(templatesDir, 'tests/Unit/Plugin_TestCase.php'), 'tests/Unit/Plugin_TestCase.php'); + // Plugin_TestCase.php is written further down, once has_services is known. writeTemplateFile(path.join(templatesDir, 'tests/Unit/Example_Test.php'), 'tests/Unit/Example_Test.php'); writeTemplateFile(path.join(templatesDir, 'gitignore.tpl'), '.gitignore'); writeTemplateFile(path.join(templatesDir, 'editorconfig.tpl'), '.editorconfig'); @@ -1014,22 +1018,22 @@ function scaffoldInto(answers, targetDir) { ].join('\n')); servicesAccessorTests.push([ '\t/**', - `\t * Services::${name}() is a memoised singleton, overridable via set()/reset().`, + `\t * Services::${name}() builds the real ${type} once, then hands back a`, + '\t * double after set(), and forgets it on reset().', '\t *', '\t * @return void', '\t */', `\tpublic function test_${name}_is_a_memoised_singleton(): void {`, - `\t\t$a = $this->createMock( \\{{NS}}\\${short}::class );`, - `\t\tServices::set( '${name}', $a );`, - '', - `\t\t$this->assertSame( $a, Services::${name}() );`, - `\t\t$this->assertSame( Services::${name}(), Services::${name}() );`, + `\t\t$this->assertInstanceOf( \\{{NS}}\\${short}::class, Services::${name}() );`, + `\t\t$this->assertSame( Services::${name}(), Services::${name}(), 'built once' );`, '', '\t\tServices::reset();', - `\t\t$b = $this->createMock( \\{{NS}}\\${short}::class );`, - `\t\tServices::set( '${name}', $b );`, + `\t\t$double = $this->createMock( \\{{NS}}\\${short}::class );`, + `\t\tServices::set( '${name}', $double );`, + `\t\t$this->assertSame( $double, Services::${name}(), 'set() overrides' );`, '', - `\t\t$this->assertSame( $b, Services::${name}(), 'reset() cleared the previous override' );`, + '\t\tServices::reset();', + `\t\t$this->assertNotSame( $double, Services::${name}(), 'reset() cleared the override' );`, '\t}', '', ].join('\n')); @@ -1379,6 +1383,12 @@ ${entries.join('\n')} writeTemplateFile(path.join(templatesDir, 'package.json'), 'package.json'); + // Now that every module block has run, we know whether any Services + // accessor exists. Plugin_TestCase's Services::reset() wiring, Services.php + // itself, and Services_Test are all gated on this. + templateFlags.has_services = servicesAccessors.length > 0; + writeTemplateFile(path.join(templatesDir, 'tests/Unit/Plugin_TestCase.php'), 'tests/Unit/Plugin_TestCase.php'); + // Assemble Plugin::boot()'s body: non-woo module lines, then every woo // line inside one class_exists( 'WooCommerce' ) guard. const allBootLines = [...bootLines]; @@ -1394,13 +1404,16 @@ ${entries.join('\n')} fs.mkdirSync(path.dirname(pluginDestPath), { recursive: true }); fs.writeFileSync(pluginDestPath, pluginContent, 'utf8'); - // Services.php: the memoised accessors for this module set (or none). - let servicesContent = fs.readFileSync(path.join(templatesDir, 'src/Services.php'), 'utf8'); - servicesContent = servicesContent.replace('{{SERVICES_ACCESSORS}}', () => servicesAccessors.join('\n')); - servicesContent = processTemplateContent(servicesContent, 'src/Services.php'); - const servicesDestPath = path.join(targetDir, 'src/Services.php'); - fs.mkdirSync(path.dirname(servicesDestPath), { recursive: true }); - fs.writeFileSync(servicesDestPath, servicesContent, 'utf8'); + // Services.php ships only when a module registers an accessor — otherwise + // it's a dead class (just set()/reset() over a permanently empty array). + if (templateFlags.has_services) { + let servicesContent = fs.readFileSync(path.join(templatesDir, 'src/Services.php'), 'utf8'); + servicesContent = servicesContent.replace('{{SERVICES_ACCESSORS}}', () => servicesAccessors.join('\n')); + servicesContent = processTemplateContent(servicesContent, 'src/Services.php'); + const servicesDestPath = path.join(targetDir, 'src/Services.php'); + fs.mkdirSync(path.dirname(servicesDestPath), { recursive: true }); + fs.writeFileSync(servicesDestPath, servicesContent, 'utf8'); + } // Services_Test.php only ships when there's at least one accessor to // exercise — with none, the class has nothing behavioural to test. diff --git a/templates/tests/Unit/Plugin_TestCase.php b/templates/tests/Unit/Plugin_TestCase.php index aedfe8b..02a80aa 100644 --- a/templates/tests/Unit/Plugin_TestCase.php +++ b/templates/tests/Unit/Plugin_TestCase.php @@ -11,15 +11,17 @@ use PHPUnit\Framework\TestCase; use {{NS}}\Plugin; +{{#if has_services}} use {{NS}}\Services; +{{/if}} /** * Class Plugin_TestCase. * - * Clears the two pieces of process-global state the plugin keeps -- the - * Plugin singleton and the Services locator's memoised instances -- after - * every test, so nothing a test builds leaks into the next one. Every unit - * test extends this instead of PHPUnit's TestCase directly. + * Clears the plugin's process-global state -- the Plugin singleton{{#if has_services}} and the + * Services locator's memoised instances{{/if}} -- after every test, so nothing a test + * builds leaks into the next one. Every unit test extends this instead of + * PHPUnit's TestCase directly. */ abstract class Plugin_TestCase extends TestCase { @@ -29,7 +31,9 @@ abstract class Plugin_TestCase extends TestCase { * @return void */ protected function tearDown(): void { +{{#if has_services}} Services::reset(); +{{/if}} Plugin::set_instance( null ); parent::tearDown(); } diff --git a/tests/generator.test.js b/tests/generator.test.js index 84d7784..b7d3e8f 100644 --- a/tests/generator.test.js +++ b/tests/generator.test.js @@ -451,7 +451,7 @@ test('generated plugin version defaults to 1.0.0', () => { fs.rmSync(outDir, { recursive: true, force: true }); }); -test('foundational classes (Plugin bootloader + Services locator) always scaffold with no leftover tokens', () => { +test('foundational classes (Plugin bootloader) always scaffold with no leftover tokens; Services locator is accessor-conditional (#16)', () => { const outDir = path.join(__dirname, '../tmp-test-foundation'); runGenerator({ name: 'Foundation Plugin', @@ -465,7 +465,6 @@ test('foundational classes (Plugin bootloader + Services locator) always scaffol const files = [ 'src/Plugin.php', - 'src/Services.php', 'src/Contracts/Activatable.php', 'src/Contracts/Deactivatable.php' ]; @@ -474,6 +473,9 @@ test('foundational classes (Plugin bootloader + Services locator) always scaffol const content = fs.readFileSync(path.join(outDir, f), 'utf8'); assert.ok(!/\{\{[A-Z_]+\}\}/.test(content), `no unreplaced template tokens should remain in ${f}`); } + // A zero-module scaffold registers no Services accessor, so the locator + // itself doesn't ship -- it would be a dead class (#16). + assert.ok(!fs.existsSync(path.join(outDir, 'src/Services.php')), 'no Services.php with zero accessors (#16)'); assert.ok(!fs.existsSync(path.join(outDir, 'src/Core/Container.php')), 'the DI container is gone: static bootloader + Services locator'); assert.ok(!fs.existsSync(path.join(outDir, 'src/Contracts/Service_Provider.php')), 'no Service_Provider contract'); assert.ok(!fs.existsSync(path.join(outDir, 'src/Core/Uninstaller.php')), 'no Uninstaller without a module that persists cleanup-worthy state (0.7)'); @@ -1447,13 +1449,20 @@ test('test isolation: Plugin_TestCase base always ships; Services_Test only with description: 'x', modules: [], useReact: false, out: bare }); - // Plugin_TestCase ships in every scaffold and resets both globals. + // Plugin_TestCase ships in every scaffold and always resets the Plugin + // singleton; it resets Services too, but only where the locator itself + // ships -- a zero-accessor scaffold has no Services.php to reset (#16). for (const d of [withSvc, bare]) { const base = fs.readFileSync(path.join(d, 'tests/Unit/Plugin_TestCase.php'), 'utf8'); assert.match(base, /abstract class Plugin_TestCase extends TestCase/); - assert.match(base, /Services::reset\(\);/); assert.match(base, /Plugin::set_instance\( null \);/); } + const baseWithSvc = fs.readFileSync(path.join(withSvc, 'tests/Unit/Plugin_TestCase.php'), 'utf8'); + assert.match(baseWithSvc, /use IsoSvc\\Services;/); + assert.match(baseWithSvc, /Services::reset\(\);/); + const baseBare = fs.readFileSync(path.join(bare, 'tests/Unit/Plugin_TestCase.php'), 'utf8'); + assert.ok(!baseBare.includes('Services'), 'no Services import or reset() when the locator itself does not ship (#16)'); + assert.ok(!fs.existsSync(path.join(bare, 'src/Services.php')), 'no Services.php in a scaffold with zero accessors (#16)'); // Every generated *_Test.php extends the base, not PHPUnit's TestCase directly. for (const d of [withSvc, bare]) { @@ -1477,12 +1486,15 @@ test('test isolation: Plugin_TestCase base always ships; Services_Test only with assert.ok(!schemaTest.includes('markTestSkipped')); // Services_Test ships only when there's an accessor, and drives it through - // createMock() + the public accessor (no reflection). + // the public accessor (no reflection) -- default construction, memoisation, + // the set() override, and reset() clearing it (#17). const svcTest = fs.readFileSync(path.join(withSvc, 'tests/Unit/Services_Test.php'), 'utf8'); assert.match(svcTest, /public function test_cache_is_a_memoised_singleton\(\): void/); + assert.ok(svcTest.includes('$this->assertInstanceOf( \\IsoSvc\\Cache\\Cache_Service::class, Services::cache() )'), 'default construction asserted (#17)'); + assert.ok(svcTest.includes("$this->assertSame( Services::cache(), Services::cache(), 'built once' )"), 'memoisation asserted'); assert.ok(svcTest.includes('$this->createMock( \\IsoSvc\\Cache\\Cache_Service::class )')); - assert.ok(svcTest.includes('$this->assertSame( $a, Services::cache() )')); - assert.ok(svcTest.includes('$this->assertSame( Services::cache(), Services::cache() )'), 'memoisation asserted'); + assert.ok(svcTest.includes("$this->assertSame( $double, Services::cache(), 'set() overrides' )")); + assert.ok(svcTest.includes("$this->assertNotSame( $double, Services::cache(), 'reset() cleared the override' )")); assert.ok(!svcTest.includes('ReflectionProperty'), 'no reflection — public API only'); assert.ok(!fs.existsSync(path.join(bare, 'tests/Unit/Services_Test.php')), 'no Services_Test in a scaffold with zero accessors'); assert.ok(!fs.existsSync(path.join(withSvc, 'tests/Unit/Container_Test.php')));