[fix] VirtualPage (#1602)

* [fix] VirtualPage Fix for #1587 Callable for VirtualPage should be called prior to VirtualPage::getHtml() in order to make sure nested callback (Lookup) can be reach. * cs fix * phpstan * fix test * more fix * Add behat test * fix typo * phpstan * phpstan * fix typo * fixes * fix test * phpstan
atk4 · Feb 3, 2021 · 32f18d7 · 32f18d7
1 parent eaa1dbd
commit 32f18d7
Show file tree

Hide file tree

Showing 8 changed files with 70 additions and 84 deletions.
diff --git a/demos/_unit-test/lookup-virtual-page.php b/demos/_unit-test/lookup-virtual-page.php
@@ -0,0 +1,39 @@
+<?php
+
+declare(strict_types=1);
+/**
+ * Test for Lookup inside VirtualPage.
+ */
+
+namespace Atk4\Ui\Demos;
+
+use Atk4\Ui\Form;
+use Atk4\Ui\Grid;
+use Atk4\Ui\JsModal;
+use Atk4\Ui\JsToast;
+use Atk4\Ui\VirtualPage;
+
+/** @var \Atk4\Ui\App $app */
+require_once __DIR__ . '/../init-app.php';
+
+$product = new ProductLock($app->db);
+
+$vp = VirtualPage::addTo($app);
+
+$vp->set(function ($page) {
+    $form = Form::addTo($page);
+    $form->addControl('category', [Form\Control\Lookup::class, 'model' => new Category($page->getApp()->db)]);
+    $form->onSubmit(function ($f) {
+        $category = $f->getControl('category')->model->load($f->model->get('category'));
+
+        return new JsToast($category->getTitle());
+    });
+});
+
+$g = Grid::addTo($app, ['menu' => ['class' => ['atk-grid-menu']]]);
+$g->setModel($product);
+
+$g->menu->addItem(
+    ['Add Category'],
+    new JsModal('New Category', $vp)
+);
diff --git a/demos/interactive/tabs.php b/demos/interactive/tabs.php
@@ -26,7 +26,7 @@
 // dynamic tab
 $tabs->addTab('Dynamic Lorem Ipsum', function ($tab) {
     \Atk4\Ui\Message::addTo($tab, ['Every time you come to this tab, you will see a different text']);
-    \Atk4\Ui\LoremIpsum::addTo($tab, ['size' => (int) $_GET['size'] ?? 1]);
+    \Atk4\Ui\LoremIpsum::addTo($tab, ['size' => (int) ($_GET['size'] ?? 1)]);
 }, ['apiSettings' => ['data' => ['size' => random_int(1, 4)]]]);
 
 // modal tab

diff --git a/demos/interactive/virtual.php b/demos/interactive/virtual.php
@@ -68,7 +68,7 @@
 
 $frame = \Atk4\Ui\VirtualPage::addTo($app);
 $frame->set(function ($frame) {
-    \Atk4\Ui\Header::addTo($frame, ['Clicked row with ID = ' . $_GET['id']]);
+    \Atk4\Ui\Header::addTo($frame, ['Clicked row with ID = ' . ($_GET['id'] ?? '')]);
 });
 
 $table->onRowClick(new \Atk4\Ui\JsModal('Row Clicked', $frame, ['id' => $table->jsRow()->data('id')]));
diff --git a/phpstan.neon.dist b/phpstan.neon.dist
@@ -213,8 +213,6 @@ parameters:
         # for src/Columns.php
         - '~^Property Atk4\\Ui\\Columns::\$calculated_width \(int\) does not accept false\.$~'
         - '~^Property Atk4\\Ui\\View::\$content \(string\|false\) does not accept null\.$~'
-        # for src/Console.php
-        - '~^Parameter #1 \$fx \(Closure\) of method Atk4\\Ui\\Console::set\(\) should be compatible with parameter \$arg1 \(array\|string\) of method Atk4\\Ui\\View::set\(\)$~'
         - '~^Property Atk4\\Ui\\JsSse::\$echoFunction \(Closure\) does not accept false\.$~'
         # for src/Crud.php
         - '~^Property Atk4\\Ui\\Crud::\$jsExecutor \(string\) does not accept default value of type array\<int, string\>\.$~'
@@ -262,18 +260,13 @@ parameters:
         - '~^Method Atk4\\Ui\\Layout::addRightPanel\(\) should return Atk4\\Ui\\Panel\\Loadable but returns Atk4\\Ui\\AbstractView\.$~'
         # for src/Loader.php
         - '~^Property Atk4\\Ui\\Loader::\$shim \(Atk4\\Ui\\View\) does not accept array\<int\|string, array\<int\|string, string\>\|class\-string\>\.$~'
-        - '~^Parameter #1 \$fx \(Closure\) of method Atk4\\Ui\\Loader::set\(\) should be compatible with parameter \$arg1 \(array\|string\) of method Atk4\\Ui\\View::set\(\)$~'
         # for src/Menu.php
         - '~^Method Atk4\\Ui\\Menu::addDivider\(\) should return Atk4\\Ui\\View but returns Atk4\\Ui\\AbstractView\.$~'
         # for src/Message.php
         - '~^Property Atk4\\Ui\\Message::\$icon \(string\) does not accept Atk4\\Ui\\Icon\.$~'
-        # for src/Modal.php
-        - '~^Parameter #1 \$fx \(Closure\) of method Atk4\\Ui\\Modal::set\(\) should be compatible with parameter \$arg1 \(array\|string\) of method Atk4\\Ui\\View::set\(\)$~'
         # for src/Panel/Right.php
         - '~^Method Atk4\\Ui\\Panel\\Right::getDynamicContent\(\) should return Atk4\\Ui\\Panel\\LoadableContent but returns Atk4\\Ui\\View\|null\.$~'
         - '~^Property Atk4\\Ui\\Panel\\Right::\$closeModal \(Atk4\\Ui\\Modal\) does not accept Atk4\\Ui\\AbstractView\.$~'
-        # for src/Popup.php
-        - '~^Parameter #1 \$fx \(Closure\) of method Atk4\\Ui\\Popup::set\(\) should be compatible with parameter \$arg1 \(array\|string\) of method Atk4\\Ui\\View::set\(\)$~'
         # for src/Table.php
         - '~^Property Atk4\\Ui\\Table::\$totals_plan \(bool\) does not accept array\.$~'
         - '~^Argument of an invalid type bool supplied for foreach, only iterables are supported\.$~'
@@ -558,12 +551,6 @@ parameters:
         -
             path: 'src/View.php'
             message: '~^If condition is always true\.$~'
-        -
-            path: 'src/VirtualPage.php'
-            message: '~^If condition is always true\.$~'
-        -
-            path: 'src/VirtualPage.php'
-            message: '~^Unreachable statement \- code above always terminates\.$~'
         -
             path: 'src/Wizard.php'
             message: '~^Negated boolean expression is always false\.$~'
@@ -576,9 +563,6 @@ parameters:
 
         # TODO these rules are generated, this ignores should be fixed in the code
         # for level = 5
-        -
-            path: 'demos/_includes/DemoLookup.php'
-            message: '~^Parameter \#1 \$fx of method Atk4\\Ui\\VirtualPage::set\(\) expects array, Closure\(mixed\): void given\.$~'
         -
             path: 'demos/_includes/ReloadTest.php'
             message: '~^Parameter \#2 \$action of method Atk4\\Ui\\View::js\(\) expects Atk4\\Ui\\JsExpression\|null, Atk4\\Ui\\JsReload given\.$~'
@@ -588,12 +572,12 @@ parameters:
         -
             path: 'demos/_unit-test/callback.php'
             message: '~^Parameter \#2 \$selector of method Atk4\\Ui\\View::on\(\) expects string\|null, Atk4\\Ui\\JsModal given\.$~'
+        -
+            path: 'demos/_unit-test/lookup-virtual-page.php'
+            message: '~^Parameter \#2 \$action of method Atk4\\Ui\\Menu::addItem\(\) expects array\|string\|null, Atk4\\Ui\\JsModal given\.$~'
         -
             path: 'demos/_unit-test/reload.php'
             message: '~^Parameter \#2 \$selector of method Atk4\\Ui\\View::on\(\) expects string\|null, Atk4\\Ui\\JsReload given\.$~'
-        -
-            path: 'demos/basic/button.php'
-            message: '~^Parameter \#1 \$label of class Atk4\\Ui\\Button constructor expects array\|string\|null, int given\.$~'
         -
             path: 'demos/basic/label.php'
             message: '~^Parameter \#2 \$selector of method Atk4\\Ui\\View::on\(\) expects string\|null, Atk4\\Ui\\JsReload given\.$~'
@@ -606,9 +590,6 @@ parameters:
         -
             path: 'demos/basic/view.php'
             message: '~^Parameter \#2 \$selector of method Atk4\\Ui\\View::on\(\) expects string\|null, Atk4\\Ui\\JsReload given\.$~'
-        -
-            path: 'demos/basic/view.php'
-            message: '~^Parameter \#1 \$fx of method Atk4\\Ui\\VirtualPage::set\(\) expects array, Closure\(mixed\): void given\.$~'
         -
             path: 'demos/basic/view.php'
             message: '~^Parameter \#2 \$selector of method Atk4\\Ui\\View::on\(\) expects string\|null, Atk4\\Ui\\JsModal given\.$~'
@@ -627,9 +608,6 @@ parameters:
         -
             path: 'demos/collection/grid.php'
             message: '~^Parameter \#2 \$selector of method Atk4\\Ui\\View::on\(\) expects string\|null, Atk4\\Ui\\JsExpression given\.$~'
-        -
-            path: 'demos/collection/multitable.php'
-            message: '~^Parameter \#1 \$fx of method Atk4\\Ui\\VirtualPage::set\(\) expects array, Closure\(mixed\): void given\.$~'
         -
             path: 'demos/collection/multitable.php'
             message: '~^Parameter \#2 \$selector of method Atk4\\Ui\\View::on\(\) expects string\|null, Atk4\\Ui\\JsModal given\.$~'
@@ -693,9 +671,6 @@ parameters:
         -
             path: 'demos/interactive/virtual.php'
             message: '~^Parameter \#1 \$title of class Atk4\\Ui\\JsModal constructor expects string, null given\.$~'
-        -
-            path: 'demos/interactive/virtual.php'
-            message: '~^Parameter \#1 \$fx of method Atk4\\Ui\\VirtualPage::set\(\) expects array, Closure\(mixed\): void given\.$~'
         -
             path: 'demos/javascript/reloading.php'
             message: '~^Parameter \#2 \$action of method Atk4\\Ui\\View::js\(\) expects Atk4\\Ui\\JsExpression\|null, Atk4\\Ui\\JsReload given\.$~'
@@ -732,9 +707,6 @@ parameters:
         -
             path: 'demos/tutorial/intro.php'
             message: '~^Parameter \#2 \$selector of method Atk4\\Ui\\View::on\(\) expects string\|null, Closure\(\): string given\.$~'
-        -
-            path: 'src/Accordion.php'
-            message: '~^Parameter \#1 \$fx of method Atk4\\Ui\\VirtualPage::set\(\) expects array, Closure given\.$~'
         -
             path: 'src/App.php'
             message: '~^Parameter \#2 \$attr of method Atk4\\Ui\\App::getTag\(\) expects string\|null, array\<string, bool\|string\> given\.$~'
@@ -744,9 +716,6 @@ parameters:
         -
             path: 'src/Card.php'
             message: '~^Parameter \#1 \$object of method Atk4\\Ui\\View::add\(\) expects Atk4\\Ui\\View, array\<int\|string, \$this\(Atk4\\Ui\\Card\)\|string\> given\.$~'
-        -
-            path: 'src/Card.php'
-            message: '~^Parameter \#1 \$fx of method Atk4\\Ui\\VirtualPage::set\(\) expects array, Closure\(mixed\): void given\.$~'
         -
             path: 'src/Card.php'
             message: '~^Parameter \#2 \$selector of method Atk4\\Ui\\View::on\(\) expects string\|null, Atk4\\Ui\\JsModal given\.$~'
@@ -786,9 +755,6 @@ parameters:
         -
             path: 'src/Form/Control/Input.php'
             message: '~^Parameter \#2 \$selector of method Atk4\\Ui\\View::on\(\) expects string\|null, Atk4\\Data\\Model\\UserAction given\.$~'
-        -
-            path: 'src/Form/Control/Lookup.php'
-            message: '~^Parameter \#1 \$fx of method Atk4\\Ui\\VirtualPage::set\(\) expects array, Closure\(mixed\): void given\.$~'
         -
             path: 'src/Form/Control/Lookup.php'
             message: '~^Parameter \#2 \$attr of method Atk4\\Ui\\App::getTag\(\) expects string\|null, array given\.$~'
@@ -819,9 +785,6 @@ parameters:
         -
             path: 'src/Grid.php'
             message: '~^Parameter \#2 \$action of method Atk4\\Ui\\Menu::addItem\(\) expects array\|string\|null, Atk4\\Ui\\JsReload given\.$~'
-        -
-            path: 'src/ItemsPerPageSelector.php'
-            message: '~^Parameter \#1 \$arg1 of method Atk4\\Ui\\View::set\(\) expects array\|string\|null, int\|null given\.$~'
         -
             path: 'src/JsSearch.php'
             message: '~^Parameter \#2 \$value of method Atk4\\Ui\\HtmlTemplate::trySet\(\) expects string\|null, Atk4\\Ui\\Form\\Control\\Line given\.$~'
@@ -879,9 +842,6 @@ parameters:
         -
             path: 'src/Table/Column/Tooltip.php'
             message: '~^Parameter \#2 \$attr of method Atk4\\Ui\\App::getTag\(\) expects string\|null, array\<string, string\> given\.$~'
-        -
-            path: 'src/Tabs.php'
-            message: '~^Parameter \#1 \$fx of method Atk4\\Ui\\VirtualPage::set\(\) expects array, Closure given\.$~'
         -
             path: 'src/UserAction/BasicExecutor.php'
             message: '~^Parameter \#2 \$selector of method Atk4\\Ui\\View::on\(\) expects string\|null, Closure\(\): mixed given\.$~'
@@ -918,9 +878,6 @@ parameters:
         -
             path: 'tests-behat/bootstrap/Context.php'
             message: '~^Parameter \#1 \$id of method Behat\\Mink\\Element\\TraversableElement::findById\(\) expects string, null given\.$~'
-        -
-            path: 'tests/CallbackTest.php'
-            message: '~^Parameter \#1 \$fx of method Atk4\\Ui\\VirtualPage::set\(\) expects array, Closure\(mixed\): void given\.$~'
         -
             path: 'tests/HtmlTemplateTest.php'
             message: '~^Parameter \#2 \$value of method Atk4\\Ui\\HtmlTemplate::set\(\) expects string\|null, stdClass given\.$~'

diff --git a/src/View.php b/src/View.php
@@ -391,8 +391,8 @@ public function getClosestOwner(self $object, $class)
      * Override this method without compatibility with parent, if you wish
      * to set your own things your own way for your view.
      *
-     * @param string|array $arg1
-     * @param string|null  $arg2
+     * @param mixed $arg1
+     * @param mixed $arg2
      *
      * @return $this
      */

diff --git a/src/VirtualPage.php b/src/VirtualPage.php
@@ -18,9 +18,6 @@ class VirtualPage extends View
     /** @var Callback */
     public $cb;
 
-    /** @var \Closure Optional callback function of virtual page */
-    public $fx;
-
     /** @var string specify custom callback trigger for the URL (see Callback::$urlTrigger) */
     public $urlTrigger;
 
@@ -40,26 +37,18 @@ protected function init(): void
     /**
      * Set callback function of virtual page.
      *
-     * Note that only one callback function can be defined.
-     *
-     * @param array $fx   Need this to be defined as array otherwise we get warning in PHP7
-     * @param mixed $junk
+     * @param \Closure $fx
+     * @param mixed    $junk
      *
      * @return $this
      */
-    public function set($fx = [], $junk = null)
+    public function set($fx = null, $junk = null)
     {
-        if (!$fx) {
-            return $this;
+        if (!$fx || !$fx instanceof \Closure) {
+            throw new Exception('Virtual page requires a Closure.');
         }
 
-        if ($this->fx) {
-            throw (new Exception('Callback for this Virtual Page is already defined'))
-                ->addMoreInfo('vp', $this)
-                ->addMoreInfo('old_fx', $this->fx)
-                ->addMoreInfo('new_fx', $fx);
-        }
-        $this->fx = $fx;
+        $this->cb->set($fx, [$this]);
 
         return $this;
     }
@@ -104,14 +93,8 @@ public function getJsUrl($mode = 'callback')
      */
     public function getHtml()
     {
-        $this->cb->set(function () {
-            // if virtual page callback is triggered
+        if ($this->cb->canTerminate()) {
             if ($mode = $this->cb->getTriggeredValue()) {
-                // process callback
-                if ($this->fx) {
-                    ($this->fx)($this);
-                }
-
                 // special treatment for popup
                 if ($mode === 'popup') {
                     $this->getApp()->html->template->set('title', $this->getApp()->title);
@@ -161,7 +144,7 @@ public function getHtml()
             $this->getApp()->html->template->dangerouslyAppendHtml('HEAD', $this->getApp()->layout->getJs());
 
             $this->getApp()->terminateHtml($this->getApp()->html->template);
-        });
+        }
     }
 
     protected function mergeStickyArgsFromChildView(): ?AbstractView

diff --git a/tests-behat/lookup.feature b/tests-behat/lookup.feature
@@ -1,12 +1,17 @@
 Feature: Lookup
   Testing Lookup control
 
-  Scenario:
-    Given I am on "_unit-test/lookup.php"
-
   Scenario: Testing lookup in modal
+    Given I am on "_unit-test/lookup.php"
     Then I press button "Edit"
     Then I select value "Dairy" in lookup "atk_fp_product__product_category_id"
     Then I select value "Yogourt" in lookup "atk_fp_product__product_sub_category_id"
     Then I press button "EditMe"
     Then Toast display should contains text 'Dairy - Yogourt'
+
+  Scenario: Testing lookup in VirtualPage
+    Given I am on "_unit-test/lookup-virtual-page.php"
+    Then I press menu button "Add Category" using class "atk-grid-menu"
+    Then I select value "Beverages" in lookup "category"
+    Then I press Modal button "Save"
+    Then Toast display should contains text 'Beverages'
diff --git a/tests/CallbackTest.php b/tests/CallbackTest.php
@@ -144,14 +144,14 @@ public function testVirtualPage()
         $var = null;
 
         $vp = \Atk4\Ui\VirtualPage::addTo($this->app);
+
         // simulate triggering
+        $_GET[$vp->name] = '1';
 
         $vp->set(function ($p) use (&$var) {
             $var = 25;
         });
 
-        $_GET[$vp->name] = '1';
-
         $this->expectOutputRegex('/^..DOCTYPE/');
         $this->app->run();
         $this->assertSame(25, $var);
@@ -162,13 +162,14 @@ public function testVirtualPageCustomTrigger()
         $var = null;
 
         $vp = \Atk4\Ui\VirtualPage::addTo($this->app, ['urlTrigger' => 'bah']);
-        $vp->set(function ($p) use (&$var) {
-            $var = 25;
-        });
 
         // simulate triggering
         $_GET['bah'] = '1';
 
+        $vp->set(function ($p) use (&$var) {
+            $var = 25;
+        });
+
         $this->expectOutputRegex('/^..DOCTYPE/');
         $this->app->run();
         $this->assertSame(25, $var);
@@ -186,11 +187,12 @@ public function testPull230()
         $var = null;
 
         $vp = \Atk4\Ui\VirtualPage::addTo($this->app);
-        $vp->set([$this, 'callPull230']);
 
         // simulate triggering
         $_GET[$vp->name] = '1';
 
+        $vp->set(\Closure::fromCallable([$this, 'callPull230']));
+
         $this->expectOutputRegex('/^..DOCTYPE/');
         $this->app->run();
         $this->assertSame(26, $this->var);