Fix Table\Column\ActionButtons duplicate HTML IDs (#2148)

atk4 · Jan 29, 2024 · c8ed4fd · c8ed4fd
1 parent f73cb12
commit c8ed4fd
Show file tree

Hide file tree

Showing 20 changed files with 288 additions and 105 deletions.
diff --git a/demos/_unit-test/scope-builder.php b/demos/_unit-test/scope-builder.php
@@ -37,10 +37,7 @@
 
 $form->onSubmit(static function (Form $form) use ($model) {
     $message = $form->model->get('qb')->toWords($model);
-    $view = (new View(['name' => View::NAME_POSSIBLY_NON_UNIQUE]))->addClass('atk-scope-builder-response');
-    $view->setApp($form->getApp());
-    $view->invokeInit();
-
+    $view = View::addTo($form)->addClass('atk-scope-builder-response');
     $view->set($message);
 
     return $view;

diff --git a/demos/data-action/jsactionsgrid.php b/demos/data-action/jsactionsgrid.php
@@ -25,7 +25,7 @@
 
 // creating special menu item for multi_step action
 $multiAction = $country->getUserAction('multi_step');
-$specialItem = Factory::factory([View::class], ['name' => View::NAME_POSSIBLY_NON_UNIQUE, 'class' => ['item'], 'content' => 'Multi Step']);
+$specialItem = Factory::factory([View::class], ['class' => ['item'], 'content' => 'Multi Step']);
 Icon::addTo($specialItem, ['content' => 'window maximize outline']);
 // register this menu item in factory
 $app->getExecutorFactory()->registerTrigger(ExecutorFactory::TABLE_MENU_ITEM, $specialItem, $multiAction);
@@ -35,12 +35,12 @@
 $grid = Grid::addTo($app, ['menu' => false]);
 $grid->setModel($country);
 
-$divider = Factory::factory([View::class], ['name' => View::NAME_POSSIBLY_NON_UNIQUE, 'class' => ['divider'], 'content' => '']);
+$divider = Factory::factory([View::class], ['class' => ['divider'], 'content' => '']);
 
-$modelHeader = Factory::factory([View::class], ['name' => View::NAME_POSSIBLY_NON_UNIQUE, 'class' => ['header'], 'content' => 'Model Actions']);
+$modelHeader = Factory::factory([View::class], ['class' => ['header'], 'content' => 'Model Actions']);
 Icon::addTo($modelHeader, ['content' => 'database']);
 
-$jsHeader = Factory::factory([View::class], ['name' => View::NAME_POSSIBLY_NON_UNIQUE, 'class' => ['header'], 'content' => 'JS Actions']);
+$jsHeader = Factory::factory([View::class], ['class' => ['header'], 'content' => 'JS Actions']);
 Icon::addTo($jsHeader, ['content' => 'file code']);
 
 $grid->addActionMenuItem($jsHeader);

diff --git a/docs/table.md b/docs/table.md
@@ -346,19 +346,18 @@ in which case we call them "decorators".
 During the render process (see {php:meth}`View::renderView`) Table will perform the following actions:
 
 1. Generate header row.
-2. Generate template for data rows.
-3. Iterate through rows
+2. Iterate through rows
    1. Current row data is accessible through $table->model property.
-   2. Update Totals if {php:meth}`Table::addTotals` was used.
-   3. Insert row values into {php:attr}`Table::$tRow`
+   2. Generate template for data row.
+   3. Update Totals if {php:meth}`Table::addTotals` was used.
+   4. Insert row values into {php:attr}`Table::$tRow`
        1. Template relies on {ref}`uiPersistence` for formatting values
-   4. Collect HTML tags from 'getHtmlTags' hook.
-   5. Collect getHtmlTags() from columns objects
-   6. Inject HTML into {php:attr}`Table::$tRow` template
-   7. Render and append row template to Table Body ({$Body})
-   8. Clear HTML tag values from template.
-4. If no rows were displayed, then "empty message" will be shown (see {php:attr}`Table::$tEmpty`).
-5. If {php:meth}`addTotals` was used, append totals row to table footer.
+   5. Collect HTML tags from 'getHtmlTags' hook.
+   6. Collect getHtmlTags() from columns objects
+   7. Inject HTML into {php:attr}`Table::$tRow` template
+   8. Render and append row template to Table Body ({$Body})
+3. If no rows were displayed, then "empty message" will be shown (see {php:attr}`Table::$tEmpty`).
+4. If {php:meth}`addTotals` was used, append totals row to table footer.
 
 ## Dealing with Multiple decorators
 

diff --git a/docs/tablecolumn.md b/docs/tablecolumn.md
@@ -55,12 +55,8 @@ can rely on various strategies for calculating totals. See {php:meth}`Table::add
 :::{php:method} getDataCellHtml(\Atk4\Data\Field $field): string
 :::
 
-Provided with a field, this method will respond with HTML **template**. In order to keep
-performance of Web Application at the maximum, Table will execute getDataCellHtml for all the
-fields once. When iterating, a combined template will be used to display the values.
-
-The template must not incorporate field values (simply because related model will not be
-loaded just yet), but instead should resort to tags and syntax compatible with {php:class}`Template`.
+Provided with a field, this method will respond with HTML **template**. When iterating,
+a combined template will be used to display the values.
 
 A sample template could be:
 

diff --git a/docs/template.md b/docs/template.md
@@ -140,7 +140,7 @@ Try loading the template. Returns false if template couldn't be loaded. This can
 if you attempt to load template from various locations.
 :::
 
-:::{php:method} loadFromString($string)
+:::{php:method} loadFromString($string, bool $allowParseCache = false)
 Same as using constructor.
 :::
 

diff --git a/src/Behat/Context.php b/src/Behat/Context.php
@@ -205,9 +205,6 @@ protected function assertNoDuplicateId(): void
             })();
             EOF);
 
-        // TODO hack to pass CI testing, fix these issues and remove the error diffs below asap
-        $duplicateIds = array_diff($duplicateIds, ['atk', '__atk4_ui_non_unique___icon', 'atk_icon']); // generated when component is not correctly added to app/layout component tree - should throw, as such name/ID is dangerous to be used
-
         if (count($invalidIds) > 0) {
             throw new \Exception('Page contains element with invalid ID: ' . implode(', ', array_map(static fn ($v) => '"' . $v . '"', $invalidIds)));
         }

diff --git a/src/HtmlTemplate.php b/src/HtmlTemplate.php
@@ -37,7 +37,7 @@ public function __construct(string $template = '')
         $this->loadFromString($template);
     }
 
-    public function _hasTag(string $tag): bool
+    protected function _hasTag(string $tag): bool
     {
         return isset($this->tagTrees[$tag]);
     }
@@ -146,19 +146,36 @@ protected function emptyTagTree(TagTree $tagTree): void
      */
     protected function _setOrAppend($tag, string $value = null, bool $encodeHtml = true, bool $append = false, bool $throwIfNotFound = true): void
     {
-        if ($tag instanceof Model) {
+        if ($tag instanceof Model && $value === null) {
             if (!$encodeHtml) {
                 throw new Exception('HTML is not allowed to be dangerously set from Model');
             }
 
-            $tag = $this->getApp()->uiPersistence->typecastSaveRow($tag, $tag->get());
+            // $tag passed as model
+            // in this case we don't throw exception if tags don't exist
+            $uiPersistence = $this->getApp()->uiPersistence;
+            foreach ($tag->getFields() as $k => $field) {
+                if ($this->_hasTag($k)) {
+                    $v = $uiPersistence->typecastSaveField($field, $tag->get($k));
+                    $this->_setOrAppend($k, $v, $encodeHtml, $append);
+                }
+            }
+
+            return;
         }
 
         // $tag passed as associative array [tag => value]
-        // in this case we don't throw exception if tags don't exist
         if (is_array($tag) && $value === null) {
+            if ($throwIfNotFound) {
+                foreach ($tag as $k => $v) {
+                    if (!$this->_hasTag($k)) {
+                        $this->_setOrAppend($k, $v, $encodeHtml, $append, true);
+                    }
+                }
+            }
+
             foreach ($tag as $k => $v) {
-                $this->_setOrAppend($k, $v, $encodeHtml, $append, false);
+                $this->_setOrAppend($k, $v, $encodeHtml, $append, $throwIfNotFound);
             }
 
             return;
@@ -420,17 +437,17 @@ public function tryLoadFromFile(string $filename)
             return false;
         }
 
-        $this->loadFromString($str);
+        $this->loadFromString($str, true);
 
         return $this;
     }
 
     /**
      * @return $this
      */
-    public function loadFromString(string $str): self
+    public function loadFromString(string $str, bool $allowParseCache = false): self
     {
-        $this->parseTemplate($str);
+        $this->parseTemplate($str, $allowParseCache);
 
         return $this;
     }
@@ -479,7 +496,7 @@ protected function parseTemplateTree(array &$inputReversed, string $openedTag =
         return $tagTree;
     }
 
-    protected function parseTemplate(string $str): void
+    protected function parseTemplate(string $str, bool $allowParseCache): void
     {
         $cKey = static::class . "\0" . $str;
         if (!isset(self::$_parseCache[$cKey])) {
@@ -493,27 +510,33 @@ protected function parseTemplate(string $str): void
             try {
                 $this->tagTrees[self::TOP_TAG] = $this->parseTemplateTree($inputReversed);
                 $tagTrees = $this->tagTrees;
-
-                if (self::$_parseCacheParentTemplate === null) {
-                    $cKeySelfEmpty = self::class . "\0";
-                    self::$_parseCache[$cKeySelfEmpty] = [];
-                    try {
-                        self::$_parseCacheParentTemplate = new self();
-                    } finally {
-                        unset(self::$_parseCache[$cKeySelfEmpty]);
-                    }
-                }
-                $parentTemplate = self::$_parseCacheParentTemplate;
-
-                \Closure::bind(static function () use ($tagTrees, $parentTemplate) {
-                    foreach ($tagTrees as $tagTree) {
-                        $tagTree->parentTemplate = $parentTemplate;
-                    }
-                }, null, TagTree::class)();
-                self::$_parseCache[$cKey] = $tagTrees;
             } finally {
                 $this->tagTrees = [];
             }
+
+            if (!$allowParseCache) {
+                $this->tagTrees = $tagTrees;
+
+                return;
+            }
+
+            if (self::$_parseCacheParentTemplate === null) {
+                $cKeySelfEmpty = self::class . "\0";
+                self::$_parseCache[$cKeySelfEmpty] = [];
+                try {
+                    self::$_parseCacheParentTemplate = new self();
+                } finally {
+                    unset(self::$_parseCache[$cKeySelfEmpty]);
+                }
+            }
+            $parentTemplate = self::$_parseCacheParentTemplate;
+
+            \Closure::bind(static function () use ($tagTrees, $parentTemplate) {
+                foreach ($tagTrees as $tagTree) {
+                    $tagTree->parentTemplate = $parentTemplate;
+                }
+            }, null, TagTree::class)();
+            self::$_parseCache[$cKey] = $tagTrees;
         }
 
         $this->tagTrees = $this->cloneTagTrees(self::$_parseCache[$cKey]);

diff --git a/src/JsCallback.php b/src/JsCallback.php
@@ -167,7 +167,7 @@ private function _jsRenderIntoModal(View $response): JsExpressionable
         if ($response instanceof Modal) {
             $html = $response->getHtml();
         } else {
-            $modal = new Modal(['name' => View::NAME_POSSIBLY_NON_UNIQUE]);
+            $modal = new Modal(['name' => 'js_callback_' . md5(random_bytes(16))]);
             $modal->setApp($this->getApp());
             $modal->add($response);
             $html = $modal->getHtml();

diff --git a/src/Table.php b/src/Table.php
@@ -390,9 +390,7 @@ protected function renderView(): void
             $this->template->dangerouslySetHtml('Head', $this->tHead->renderToHtml());
         }
 
-        // iterate data rows
         $this->_renderedRowsCount = 0;
-
         try {
             foreach ($this->model as $entity) {
                 $this->currentRow = $entity;
@@ -420,6 +418,7 @@ protected function renderView(): void
                 }
             }
         } finally {
+            $this->tRowMaster->set('cells', null);
             $this->tRow = null; // @phpstan-ignore-line
             $this->currentRow = null;
         }

diff --git a/src/Table/Column.php b/src/Table/Column.php
@@ -11,6 +11,7 @@
 use Atk4\Core\TrackableTrait;
 use Atk4\Data\Field;
 use Atk4\Data\Model;
+use Atk4\Ui\Exception;
 use Atk4\Ui\Js\Jquery;
 use Atk4\Ui\Js\JsExpression;
 use Atk4\Ui\Js\JsExpressionable;
@@ -56,11 +57,72 @@ class Column
     /** @var array|null The tag value required for getTag when using an header action. */
     public $headerActionTag;
 
+    private string $nameInTableCache;
+
     public function __construct(array $defaults = [])
     {
         $this->setDefaults($defaults);
     }
 
+    /**
+     * Cloning View is unwanted as some references might be not cloned/set as expected,
+     * remove this method once https://github.com/atk4/ui/issues/1365 is implemented.
+     *
+     * @internal
+     */
+    protected function assertColumnViewNotInitialized(View $view): void
+    {
+        if ($view->isInitialized() || ($view->name ?? null) !== null) {
+            throw (new Exception('Unexpected initialized View instance'))
+                ->addMoreInfo('view', $view);
+        }
+    }
+
+    /**
+     * @template T of View
+     *
+     * @param T $view
+     *
+     * @return T
+     *
+     * @internal
+     */
+    protected function cloneColumnView(View $view, string $nameSuffix): View
+    {
+        $this->assertColumnViewNotInitialized($view);
+
+        $cloneViewWithAddLaterFx = static function (View $view) use (&$cloneViewWithAddLaterFx) {
+            $view = clone $view;
+
+            \Closure::bind(static function () use ($view, $cloneViewWithAddLaterFx) {
+                foreach ($view->_addLater as $k => [$obj]) {
+                    $view->_addLater[$k][0] = $cloneViewWithAddLaterFx($obj); // @phpstan-ignore-line
+                }
+            }, null, View::class)();
+
+            return $view;
+        };
+
+        if (!isset($this->nameInTableCache)) {
+            foreach ($this->table->columns as $n => $columns) {
+                foreach (is_array($columns) ? $columns : [$columns] as $k => $column) {
+                    if ($this === $column) {
+                        $this->nameInTableCache = $n . '_' . $k;
+
+                        break;
+                    }
+                }
+            }
+        }
+
+        $view = $cloneViewWithAddLaterFx($view);
+        $view->shortName = 'c' . $this->nameInTableCache . '_' . $nameSuffix . '_r'
+            . $this->getApp()->uiPersistence->typecastSaveField($this->table->model->getField($this->table->model->idField), $this->table->currentRow->getId());
+        $view->name = \Closure::bind(static fn (Table $table) => $view->_shorten($table->name, $view->shortName, null), null, Table::class)($this->table);
+
+        return $view;
+    }
+
     /**
      * Add popup to header.
      * Use ColumnName for better popup positioning.

diff --git a/src/Table/Column/ActionButtons.php b/src/Table/Column/ActionButtons.php
@@ -54,9 +54,11 @@ public function addButton($button, $action = null, string $confirmMsg = '', $isD
                 $button = [1 => $button];
             }
 
-            $button = Factory::factory([Button::class], Factory::mergeSeeds($button, ['name' => View::NAME_POSSIBLY_NON_UNIQUE]));
+            $button = Factory::factory([Button::class], $button);
         }
 
+        $this->assertColumnViewNotInitialized($button);
+
         if ($isDisabled === true) {
             $button->addClass('disabled');
         } elseif ($isDisabled !== false) {
@@ -122,7 +124,8 @@ public function getDataCellTemplate(Field $field = null): string
 
         // render our buttons
         $outputHtml = '';
-        foreach ($this->buttons as $button) {
+        foreach ($this->buttons as $name => $button) {
+            $button = $this->cloneColumnView($button, $name);
             $outputHtml .= $button->getHtml();
         }
 

diff --git a/src/Table/Column/ActionMenu.php b/src/Table/Column/ActionMenu.php
@@ -22,7 +22,7 @@
  */
 class ActionMenu extends Table\Column
 {
-    /** @var array Menu items collections. */
+    /** @var array<int, View> Menu items collections. */
     protected $items = [];
 
     /** @var array<string, \Closure(Model): bool> Callbacks as defined in UserAction->enabled for evaluating row-specific if an action is enabled. */
@@ -69,9 +69,11 @@ public function addActionMenuItem($item, $action = null, string $confirmMsg = ''
         $name = $this->name . '_action_' . (count($this->items) + 1);
 
         if (!is_object($item)) {
-            $item = Factory::factory([View::class], ['name' => View::NAME_POSSIBLY_NON_UNIQUE, 'ui' => 'item', 'content' => $item]);
+            $item = Factory::factory([View::class], ['ui' => 'item', 'content' => $item]);
         }
 
+        $this->assertColumnViewNotInitialized($item);
+
         $item->setApp($this->getApp());
         $this->items[] = $item;
 
@@ -124,7 +126,8 @@ public function getDataCellTemplate(Field $field = null): string
 
         // render our menus
         $outputHtml = '';
-        foreach ($this->items as $item) {
+        foreach ($this->items as $k => $item) {
+            $item = $this->cloneColumnView($item, (string) $k);
             $outputHtml .= $item->getHtml();
         }