Fix singular/plural variable collision in baked templates (#1075)

* Fix singular/plural variable collision in baked templates

When baking templates for models where the singular and plural forms are
identical (e.g., "news", "sheep", "fish"), the generated foreach loops
would incorrectly use the same variable for both the collection and the
iteration variable, producing invalid code like:

    foreach ($news as $news):

This fix appends "Item" to the singular variable name when a collision
is detected, producing valid code:

    foreach ($news as $newsItem):

Refs https://discourse.cakephp.org/t/paginator-error/12806/4

* 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

Author: dereuromark

src/Command/ControllerCommand.php

@@ -129,6 +129,12 @@ public function bake(string $controllerName, Arguments $args, ConsoleIo $io): vo
         $singularHumanName = $this->_singularHumanName($controllerName);
         $pluralHumanName = $this->_variableName($controllerName);
 
+        // Handle cases where singular and plural are identical (e.g., "news", "sheep")
+        // to avoid variable collisions in generated controller code
+        if ($singularName === $pluralName) {
+            $singularName .= 'Entity';
+        }
+
         $defaultModel = sprintf('%s\Model\Table\%sTable', $namespace, $controllerName);
         if (!class_exists($defaultModel)) {
             $defaultModel = null;

src/Command/TemplateCommand.php

@@ -325,6 +325,12 @@ protected function _loadController(ConsoleIo $io): array
         $pluralVar = Inflector::variable($this->controllerName);
         $pluralHumanName = $this->_pluralHumanName($this->controllerName);
 
+        // Handle cases where singular and plural are identical (e.g., "news", "sheep")
+        // to avoid generating invalid code like `foreach ($news as $news)`
+        if ($singularVar === $pluralVar) {
+            $singularVar .= 'Entity';
+        }
+
         return compact(
             'modelObject',
             'modelClass',

templates/bake/Template/view.twig

@@ -120,6 +120,9 @@
 {% set relations = associations.BelongsToMany|merge(associations.HasMany) %}
 {% for alias, details in relations %}
     {%~ set otherSingularVar = alias|singularize|variable %}
+    {%~ if otherSingularVar == singularVar %}
+        {%~ set otherSingularVar = otherSingularVar ~ 'Related' %}
+    {%~ endif %}
     {%~ set otherPluralHumanName = details.controller|underscore|humanize %}
             <div class="related">
                 <h4><?= __('Related {{ otherPluralHumanName }}') ?></h4>

templates/bake/element/Controller/add.twig

@@ -40,6 +40,9 @@
 {%- for assoc in associations %}
     {%~ set otherName = Bake.getAssociatedTableAlias(modelObj, assoc) %}
     {%~ set otherPlural = otherName|variable %}
+    {%~ if otherPlural == singularName %}
+        {%~ set otherPlural = otherPlural ~ 'List' %}
+    {%~ endif %}
         ${{ otherPlural }} = $this->{{ currentModelName }}->{{ otherName }}->find('list', limit: 200)->all();
     {%~ set compact = compact|merge(["'#{otherPlural}'"]) %}
 {% endfor %}

templates/bake/element/Controller/edit.twig

@@ -41,6 +41,9 @@
 {% for assoc in belongsTo|merge(belongsToMany) %}
     {%~ set otherName = Bake.getAssociatedTableAlias(modelObj, assoc) %}
     {%~ set otherPlural = otherName|variable %}
+    {%~ if otherPlural == singularName %}
+        {%~ set otherPlural = otherPlural ~ 'List' %}
+    {%~ endif %}
         ${{ otherPlural }} = $this->{{ currentModelName }}->{{ otherName }}->find('list', limit: 200)->all();
     {%~ set compact = compact|merge(["'#{otherPlural}'"]) %}
 {% endfor %}

tests/Fixture/NewsFixture.php

@@ -0,0 +1,28 @@
+<?php
+/**
+ * CakePHP(tm) : Rapid Development Framework (https://cakephp.org)
+ * Copyright (c) Cake Software Foundation, Inc. (https://cakefoundation.org)
+ *
+ * Licensed under The MIT License
+ * For full copyright and license information, please see the LICENSE.txt
+ * Redistributions of files must retain the above copyright notice
+ *
+ * @copyright     Copyright (c) Cake Software Foundation, Inc. (https://cakefoundation.org)
+ * @link          https://cakephp.org CakePHP(tm) Project
+ * @license       https://www.opensource.org/licenses/mit-license.php MIT License
+ */
+namespace Bake\Test\Fixture;
+
+use Cake\TestSuite\Fixture\TestFixture;
+
+/**
+ * NewsFixture - tests singular/plural variable collision
+ * "news" is both singular and plural
+ */
+class NewsFixture extends TestFixture
+{
+    public array $records = [
+        ['title' => 'First News', 'body' => 'First News Body'],
+        ['title' => 'Second News', 'body' => 'Second News Body'],
+    ];
+}

tests/TestCase/Command/ControllerCommandTest.php

@@ -39,6 +39,7 @@ class ControllerCommandTest extends TestCase
         'plugin.Bake.BakeArticlesBakeTags',
         'plugin.Bake.BakeComments',
         'plugin.Bake.BakeTags',
+        'plugin.Bake.News',
         'plugin.Bake.Users',
     ];
 
@@ -435,4 +436,34 @@ public function testMainWithPluginOption()
         $this->assertFileContains('use Company\Pastry\Controller\AppController;', $this->generatedFile);
         $this->assertFileContains('BakeArticlesController extends AppController', $this->generatedFile);
     }
+
+    /**
+     * Test baking controller for models where singular and plural are identical.
+     *
+     * This tests the fix for generating variable collisions like `$news` for both
+     * the entity and the paginated collection when the model name is both singular
+     * and plural (e.g., "news", "sheep", "fish").
+     *
+     * @return void
+     */
+    public function testBakeControllerWithSingularPluralCollision(): void
+    {
+        $this->generatedFile = APP . 'Controller/NewsController.php';
+        if (file_exists($this->generatedFile)) {
+            unlink($this->generatedFile);
+        }
+        $this->exec('bake controller --connection test --no-test News');
+
+        $this->assertExitCode(CommandInterface::CODE_SUCCESS);
+        $this->assertFileExists($this->generatedFile);
+
+        $result = file_get_contents($this->generatedFile);
+
+        // In view/edit/add/delete actions, the entity should use 'newsEntity' to avoid collision
+        $this->assertStringContainsString('$newsEntity = $this->News->get(', $result);
+
+        // In index action, the paginated collection should still use 'news'
+        $this->assertStringContainsString('$news = $this->paginate(', $result);
+        $this->assertStringContainsString("compact('news')", $result);
+    }
 }

tests/TestCase/Command/TemplateCommandTest.php

@@ -54,6 +54,7 @@ class TemplateCommandTest extends TestCase
         'plugin.Bake.BakeTemplateProfiles',
         'plugin.Bake.CategoryThreads',
         'plugin.Bake.HiddenFields',
+        'plugin.Bake.News',
     ];
 
     /**
@@ -891,4 +892,29 @@ public function testMainWithMissingTable()
 
         $this->assertExitCode(CommandInterface::CODE_ERROR);
     }
+
+    /**
+     * Test baking templates for models where singular and plural are identical.
+     *
+     * This tests the fix for generating invalid code like `foreach ($news as $news)`
+     * when the model name is both singular and plural (e.g., "news", "sheep", "fish").
+     *
+     * @return void
+     */
+    public function testBakeIndexWithSingularPluralCollision()
+    {
+        $this->generatedFile = ROOT . 'templates/News/index.php';
+        $this->exec('bake template News index');
+
+        $this->assertExitCode(CommandInterface::CODE_SUCCESS);
+        $this->assertFileExists($this->generatedFile);
+
+        $result = file_get_contents($this->generatedFile);
+
+        // Should NOT have `foreach ($news as $news)` which would overwrite the collection
+        $this->assertStringNotContainsString('foreach ($news as $news)', $result);
+
+        // Should have `foreach ($news as $newsEntity)` instead
+        $this->assertStringContainsString('foreach ($news as $newsEntity)', $result);
+    }
 }

tests/schema.php

@@ -587,4 +587,16 @@
             'unique_self_referencing_parent' => ['type' => 'unique', 'columns' => ['parent_id']],
         ],
     ],
+    // "news" is both singular and plural - tests variable collision fix
+    [
+        'table' => 'news',
+        'columns' => [
+            'id' => ['type' => 'integer'],
+            'title' => ['type' => 'string', 'length' => 255, 'null' => false],
+            'body' => ['type' => 'text'],
+            'created' => ['type' => 'datetime'],
+            'modified' => ['type' => 'datetime'],
+        ],
+        'constraints' => ['primary' => ['type' => 'primary', 'columns' => ['id']]],
+    ],
 ];