Skip to content

Commit 89dd5a1

Browse files
committed
Fix additional singular/plural variable collisions
- Change suffix from 'Item' to 'Entity' (less likely to collide with model names like 'NewsItem') - Add same fix to ControllerCommand for controller action generation - Add collision detection in view.twig for related entity loops (appends 'Related' suffix when otherSingularVar matches singularVar) - Add collision detection in Controller/add.twig and edit.twig for association list variables (appends 'List' suffix when otherPlural matches singularName) - Add test for ControllerCommand singular/plural collision handling
1 parent 6b32fee commit 89dd5a1

7 files changed

Lines changed: 46 additions & 3 deletions

File tree

‎src/Command/ControllerCommand.php‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,12 @@ public function bake(string $controllerName, Arguments $args, ConsoleIo $io): vo
129129
$singularHumanName = $this->_singularHumanName($controllerName);
130130
$pluralHumanName = $this->_variableName($controllerName);
131131

132+
// Handle cases where singular and plural are identical (e.g., "news", "sheep")
133+
// to avoid variable collisions in generated controller code
134+
if ($singularName === $pluralName) {
135+
$singularName .= 'Entity';
136+
}
137+
132138
$defaultModel = sprintf('%s\Model\Table\%sTable', $namespace, $controllerName);
133139
if (!class_exists($defaultModel)) {
134140
$defaultModel = null;

‎src/Command/TemplateCommand.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -328,7 +328,7 @@ protected function _loadController(ConsoleIo $io): array
328328
// Handle cases where singular and plural are identical (e.g., "news", "sheep")
329329
// to avoid generating invalid code like `foreach ($news as $news)`
330330
if ($singularVar === $pluralVar) {
331-
$singularVar .= 'Item';
331+
$singularVar .= 'Entity';
332332
}
333333

334334
return compact(

‎templates/bake/Template/view.twig‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,9 @@
120120
{% set relations = associations.BelongsToMany|merge(associations.HasMany) %}
121121
{% for alias, details in relations %}
122122
{%~ set otherSingularVar = alias|singularize|variable %}
123+
{%~ if otherSingularVar == singularVar %}
124+
{%~ set otherSingularVar = otherSingularVar ~ 'Related' %}
125+
{%~ endif %}
123126
{%~ set otherPluralHumanName = details.controller|underscore|humanize %}
124127
<div class="related">
125128
<h4><?= __('Related {{ otherPluralHumanName }}') ?></h4>

‎templates/bake/element/Controller/add.twig‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,9 @@
4040
{%- for assoc in associations %}
4141
{%~ set otherName = Bake.getAssociatedTableAlias(modelObj, assoc) %}
4242
{%~ set otherPlural = otherName|variable %}
43+
{%~ if otherPlural == singularName %}
44+
{%~ set otherPlural = otherPlural ~ 'List' %}
45+
{%~ endif %}
4346
${{ otherPlural }} = $this->{{ currentModelName }}->{{ otherName }}->find('list', limit: 200)->all();
4447
{%~ set compact = compact|merge(["'#{otherPlural}'"]) %}
4548
{% endfor %}

‎templates/bake/element/Controller/edit.twig‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,9 @@
4141
{% for assoc in belongsTo|merge(belongsToMany) %}
4242
{%~ set otherName = Bake.getAssociatedTableAlias(modelObj, assoc) %}
4343
{%~ set otherPlural = otherName|variable %}
44+
{%~ if otherPlural == singularName %}
45+
{%~ set otherPlural = otherPlural ~ 'List' %}
46+
{%~ endif %}
4447
${{ otherPlural }} = $this->{{ currentModelName }}->{{ otherName }}->find('list', limit: 200)->all();
4548
{%~ set compact = compact|merge(["'#{otherPlural}'"]) %}
4649
{% endfor %}

‎tests/TestCase/Command/ControllerCommandTest.php‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ class ControllerCommandTest extends TestCase
3939
'plugin.Bake.BakeArticlesBakeTags',
4040
'plugin.Bake.BakeComments',
4141
'plugin.Bake.BakeTags',
42+
'plugin.Bake.News',
4243
'plugin.Bake.Users',
4344
];
4445

@@ -435,4 +436,31 @@ public function testMainWithPluginOption()
435436
$this->assertFileContains('use Company\Pastry\Controller\AppController;', $this->generatedFile);
436437
$this->assertFileContains('BakeArticlesController extends AppController', $this->generatedFile);
437438
}
439+
440+
/**
441+
* Test baking controller for models where singular and plural are identical.
442+
*
443+
* This tests the fix for generating variable collisions like `$news` for both
444+
* the entity and the paginated collection when the model name is both singular
445+
* and plural (e.g., "news", "sheep", "fish").
446+
*
447+
* @return void
448+
*/
449+
public function testBakeControllerWithSingularPluralCollision(): void
450+
{
451+
$this->generatedFile = APP . 'Controller/NewsController.php';
452+
$this->exec('bake controller --connection test --no-test News');
453+
454+
$this->assertExitCode(CommandInterface::CODE_SUCCESS);
455+
$this->assertFileExists($this->generatedFile);
456+
457+
$result = file_get_contents($this->generatedFile);
458+
459+
// In view/edit/add/delete actions, the entity should use 'newsEntity' to avoid collision
460+
$this->assertStringContainsString('$newsEntity = $this->News->get(', $result);
461+
462+
// In index action, the paginated collection should still use 'news'
463+
$this->assertStringContainsString('$news = $this->paginate(', $result);
464+
$this->assertStringContainsString("compact('news')", $result);
465+
}
438466
}

‎tests/TestCase/Command/TemplateCommandTest.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -914,7 +914,7 @@ public function testBakeIndexWithSingularPluralCollision()
914914
// Should NOT have `foreach ($news as $news)` which would overwrite the collection
915915
$this->assertStringNotContainsString('foreach ($news as $news)', $result);
916916

917-
// Should have `foreach ($news as $newsItem)` instead
918-
$this->assertStringContainsString('foreach ($news as $newsItem)', $result);
917+
// Should have `foreach ($news as $newsEntity)` instead
918+
$this->assertStringContainsString('foreach ($news as $newsEntity)', $result);
919919
}
920920
}

0 commit comments

Comments
 (0)