Form\Control::onChange() should not accept JS code as string

demos/form-control/input2.php

@@ -153,34 +153,30 @@
 $group = $form->addGroup('Calendar');
 $c1 = $group->addControl('c1', new Form\Control\Calendar(['type' => 'date']));
 $c2 = $group->addControl('c2', new Form\Control\Calendar(['type' => 'date']));
-$c3 = $group->addControl('c3', new Form\Control\Calendar(['type' => 'date']));
 
-$c1->onChange('console.log(\'c1 changed: \' + date + \', \' + text + \', \' + mode)');
-$c2->onChange(new JsExpression('console.log(\'c2 changed: \' + date + \', \' + text + \', \' + mode)'));
-$c3->onChange([
-    new JsExpression('console.log(\'c3 changed: \' + date + \', \' + text + \', \' + mode)'),
-    new JsExpression('console.log(\'c3 really changed: \' + date + \', \' + text + \', \' + mode)'),
+$c1->onChange(new JsExpression('console.log(\'c1 changed: \' + date + \', \' + text + \', \' + mode)'));
+$c2->onChange([
+    new JsExpression('console.log(\'c2 changed: \' + date + \', \' + text + \', \' + mode)'),
+    new JsExpression('console.log(\'c2 really changed: \' + date + \', \' + text + \', \' + mode)'),
 ]);
 
 $group = $form->addGroup('Line');
 $f1 = $group->addControl('f1');
 $f2 = $group->addControl('f2');
 $f3 = $group->addControl('f3');
-$f4 = $group->addControl('f4');
 
-$f1->onChange('console.log(\'f1 changed\')');
-$f2->onChange(new JsExpression('console.log(\'f2 changed\')'));
-$f3->onChange([
-    new JsExpression('console.log(\'f3 changed\')'),
-    new JsExpression('console.log(\'f3 really changed\')'),
+$f1->onChange(new JsExpression('console.log(\'f1 changed\')'));
+$f2->onChange([
+    new JsExpression('console.log(\'f2 changed\')'),
+    new JsExpression('console.log(\'f2 really changed\')'),
 ]);
-$f4->onChange(function () {
-    return new JsExpression('console.log(\'f4 changed\')');
+$f3->onChange(function () {
+    return new JsExpression('console.log(\'f3 changed\')');
 });
 
 $group = $form->addGroup('CheckBox');
 $b1 = $group->addControl('b1', new Form\Control\Checkbox());
-$b1->onChange('console.log(\'b1 changed\')');
+$b1->onChange(new JsExpression('console.log(\'b1 changed\')'));
 
 $group = $form->addGroup(['Dropdown', 'width' => 'three']);
 $d1 = $group->addControl('d1', new Form\Control\Dropdown([
@@ -191,7 +187,7 @@
         'file' => ['File', 'icon' => 'file'],
     ],
 ]));
-$d1->onChange('console.log(\'Dropdown changed\')');
+$d1->onChange(new JsExpression('console.log(\'Dropdown changed\')'));
 
 $group = $form->addGroup('Radio');
 $r1 = $group->addControl('r1', new Form\Control\Radio([
@@ -202,7 +198,7 @@
         'File',
     ],
 ]));
-$r1->onChange('console.log(\'radio changed\')');
+$r1->onChange(new JsExpression('console.log(\'radio changed\')'));
 
 Header::addTo($app, ['Line ends of Textarea']);
 

demos/interactive/popup.php

@@ -142,8 +142,6 @@ protected function init(): void
      * Associate your shelf with cart, so that when item is clicked, the content of a
      * cart is updated.
      *
-     * Also - you can supply jsAction to execute when this happens.
-     *
      * @param JsExpressionable|array $jsAction
      */
     public function linkCart(View $cart, $jsAction = null): void

docs/callbacks.rst

@@ -279,7 +279,7 @@ will send browser screen width back to the callback::
 
     $cb->set(function (\Atk4\Ui\Js\Jquery $j, $arg1) {
         return 'width is ' . $arg1;
-    }, [new \Atk4\Ui\Js\JsExpression( '$(window).width()' )]);
+    }, [new \Atk4\Ui\Js\JsExpression('$(window).width()')]);
 
     $label->detail = $cb->getUrl();
     $label->on('click', $cb);
@@ -292,15 +292,15 @@ also supports argument passing::
 
     $label->on('click', function (Jquery $j, $arg1) {
         return 'width is ' . $arg1;
-    }, ['confirm' => 'sure?', 'args' => [new \Atk4\Ui\Js\JsExpression( '$(window).width()' )]]);
+    }, ['confirm' => 'sure?', 'args' => [new \Atk4\Ui\Js\JsExpression('$(window).width()')]]);
 
 If you do not need to specify confirm, you can actually pass arguments in a key-less array too::
 
     $label = \Atk4\Ui\Label::addTo($app, ['Callback test']);
 
     $label->on('click', function (Jquery $j, $arg1) {
         return 'width is ' . $arg1;
-    }, [new \Atk4\Ui\Js\JsExpression( '$(window).width()' )]);
+    }, [new \Atk4\Ui\Js\JsExpression('$(window).width()')]);
 
 
 Refering to event origin

docs/form-control.rst

@@ -296,7 +296,7 @@ onChange event
 .. php:method:: onChange($expression)
 
 It's prefferable to use this short-hand version of on('change', 'input', $expression) method.
-$expression argument can be string, JsExpression, array of JsExpressions or even PHP callback function.
+$expression argument can be JsExpression, array of JsExpressions or even PHP callback function.
 
     // simple string
     $f1 = $form->addControl('f1');

docs/rightpanel.rst

@@ -64,4 +64,4 @@ This method may take up to three arguments.
     to the trigger element as long as the panel remains open. This help visualize, which element has trigger the
     panel opening.
 
-    $jsTrigger: a JsExpression that represent the jQuery object where the data property reside. Default to $(this).
+    $jsTrigger: JS expression that represent the jQuery object where the data property reside. Default to $(this).

phpstan.neon.dist

@@ -187,9 +187,6 @@ parameters:
         -
             path: 'src/Table/Column.php'
             message: '~^Call to an undefined method Atk4\\Ui\\AbstractView::setHoverable\(\)\.$~'
-        -
-            path: 'src/Table/Column.php'
-            message: '~^Call to an undefined method Atk4\\Ui\\JsCallback::onSelectItem\(\)\.$~'
         -
             path: 'src/Table/Column/Checkbox.php'
             message: '~^Call to an undefined method Atk4\\Ui\\Js\\JsChain::join\(\)\.$~'

src/Accordion.php

@@ -5,6 +5,7 @@
 namespace Atk4\Ui;
 
 use Atk4\Ui\Js\JsChain;
+use Atk4\Ui\Js\JsExpressionable;
 
 /**
  * Accordion is a View holding accordion sections.
@@ -71,7 +72,7 @@ public function activate($section): void
      *
      * @return JsChain
      */
-    public function jsOpen($section, $when = false)
+    public function jsOpen($section, $when = false): JsExpressionable
     {
         return $this->jsBehavior('open', [$this->getSectionIdx($section)], $when);
     }
@@ -81,7 +82,7 @@ public function jsOpen($section, $when = false)
      *
      * @return JsChain
      */
-    public function jsCloseOthers($when = false)
+    public function jsCloseOthers($when = false): JsExpressionable
     {
         return $this->jsBehavior('close others', [], $when);
     }
@@ -92,7 +93,7 @@ public function jsCloseOthers($when = false)
      *
      * @return JsChain
      */
-    public function jsClose($section, $when = false)
+    public function jsClose($section, $when = false): JsExpressionable
     {
         return $this->jsBehavior('close', [$this->getSectionIdx($section)], $when);
     }
@@ -103,7 +104,7 @@ public function jsClose($section, $when = false)
      *
      * @return JsChain
      */
-    public function jsToggle($section, $when = false)
+    public function jsToggle($section, $when = false): JsExpressionable
     {
         return $this->jsBehavior('toggle', [$this->getSectionIdx($section)], $when);
     }
@@ -119,7 +120,7 @@ public function jsToggle($section, $when = false)
      *
      * @return JsChain
      */
-    public function jsBehavior($behavior, array $args, $when = false)
+    public function jsBehavior($behavior, array $args, $when = false): JsExpressionable
     {
         return $this->js($when)->accordion($behavior, ...$args);
     }

src/App.php

@@ -17,6 +17,7 @@
 use Atk4\Ui\Exception\LateOutputError;
 use Atk4\Ui\Exception\UnhandledCallbackExceptionError;
 use Atk4\Ui\Js\JsExpression;
+use Atk4\Ui\Js\JsExpressionable;
 use Atk4\Ui\Persistence\Ui as UiPersistence;
 use Atk4\Ui\UserAction\ExecutorFactory;
 use Psr\Log\LoggerInterface;
@@ -790,7 +791,7 @@ public function redirect($page, bool $permanent = false): void
      *
      * @param string|array $page Destination URL or page/arguments
      */
-    public function jsRedirect($page, bool $newWindow = false): JsExpression
+    public function jsRedirect($page, bool $newWindow = false): JsExpressionable
     {
         return new JsExpression('window.open([], [])', [$this->url($page), $newWindow ? '_blank' : '_top']);
     }

src/CardDeck.php

@@ -225,7 +225,7 @@ protected function initActionExecutor(Model\UserAction $action): ExecutorInterfa
     protected function jsExecute($return, Model\UserAction $action)
     {
         if (is_string($return)) {
-            return $this->getNotifier($action, $return);
+            return $this->jsCreateNotifier($action, $return);
         } elseif (is_array($return) || $return instanceof JsExpressionable) {
             return $return;
         } elseif ($return instanceof Model) {
@@ -238,13 +238,13 @@ protected function jsExecute($return, Model\UserAction $action)
             return $this->jsModelReturn($action, $msg);
         }
 
-        return $this->getNotifier($action, $this->defaultMsg);
+        return $this->jsCreateNotifier($action, $this->defaultMsg);
     }
 
     /**
      * Override this method for setting notifier based on action or model value.
      */
-    protected function getNotifier(Model\UserAction $action, string $msg = null): JsExpressionable
+    protected function jsCreateNotifier(Model\UserAction $action, string $msg = null): JsExpressionable
     {
         $notifier = Factory::factory($this->notifyDefault);
         if ($msg) {
@@ -262,7 +262,7 @@ protected function getNotifier(Model\UserAction $action, string $msg = null): Js
     protected function jsModelReturn(Model\UserAction $action, string $msg = 'Done!'): array
     {
         $js = [];
-        $js[] = $this->getNotifier($action, $msg);
+        $js[] = $this->jsCreateNotifier($action, $msg);
         $card = $action->getEntity()->isLoaded() ? $this->findCard($action->getEntity()) : null;
         if ($card !== null) {
             $js[] = $card->jsReload($this->getReloadArgs());

src/Console.php

@@ -209,8 +209,11 @@ protected function outputHtmlWithoutPre(string $messageHtml, array $context = []
         }, $messageHtml);
 
         $this->_outputBypass = true;
-        $this->sse->send($this->js()->append($messageHtml));
-        $this->_outputBypass = false;
+        try {
+            $this->sse->send($this->js()->append($messageHtml));
+        } finally {
+            $this->_outputBypass = false;
+        }
 
         return $this;
     }
@@ -225,15 +228,16 @@ protected function renderView(): void
     /**
      * Executes a JavaScript action.
      *
-     * @param JsExpressionable $js
-     *
      * @return $this
      */
-    public function send($js)
+    public function send(JsExpressionable $js)
     {
         $this->_outputBypass = true;
-        $this->sse->send($js);
-        $this->_outputBypass = false;
+        try {
+            $this->sse->send($js);
+        } finally {
+            $this->_outputBypass = false;
+        }
 
         return $this;
     }

src/Crud.php

@@ -175,14 +175,14 @@ protected function jsExecute($return, Model\UserAction $action): array
 
         // display msg return by action or depending on action modifier.
         if (is_string($return)) {
-            $js[] = $this->getNotifier($return);
+            $js[] = $this->jsCreateNotifier($return);
         } else {
             if ($action->modifier === Model\UserAction::MODIFIER_CREATE || $action->modifier === Model\UserAction::MODIFIER_UPDATE) {
-                $js[] = $this->getNotifier($this->saveMsg);
+                $js[] = $this->jsCreateNotifier($this->saveMsg);
             } elseif ($action->modifier === Model\UserAction::MODIFIER_DELETE) {
-                $js[] = $this->getNotifier($this->deleteMsg);
+                $js[] = $this->jsCreateNotifier($this->deleteMsg);
             } else {
-                $js[] = $this->getNotifier($this->defaultMsg);
+                $js[] = $this->jsCreateNotifier($this->defaultMsg);
             }
         }
 
@@ -216,14 +216,9 @@ protected function getJsGridAction(Model\UserAction $action): ?JsExpressionable
     }
 
     /**
-     * Return jsNotifier object.
      * Override this method for setting notifier based on action or model value.
-     *
-     * @param string|null $msg the message to display
-     *
-     * @return JsExpressionable
      */
-    protected function getNotifier(string $msg = null)
+    protected function jsCreateNotifier(string $msg = null): JsExpressionable
     {
         $notifier = Factory::factory($this->notifyDefault);
         if ($msg) {

src/Dropdown.php

@@ -46,7 +46,7 @@ public function onChange(\Closure $fx): void
         // setting dropdown option for using callback url.
         $this->dropdownOptions['onChange'] = new JsFunction(['value', 'name', 't'], [
             new JsExpression(
-                "if ($(this).data('currentValue') != value) { $(this).atkAjaxec({ url: [url], urlOptions: { item: value } }); $(this).data('currentValue', value); }",
+                'if ($(this).data(\'currentValue\') != value) { $(this).atkAjaxec({ url: [url], urlOptions: { item: value } }); $(this).data(\'currentValue\', value); }',
                 ['url' => $this->cb->getJsUrl()]
             ), ]);
 

src/Form.php

@@ -15,6 +15,7 @@
 use Atk4\Ui\Js\JsChain;
 use Atk4\Ui\Js\JsConditionalForm;
 use Atk4\Ui\Js\JsExpression;
+use Atk4\Ui\Js\JsExpressionable;
 
 class Form extends View
 {
@@ -32,7 +33,7 @@ class Form extends View
     public $ui = 'form';
     public $defaultTemplate = 'form.html';
 
-    /** @var Callback Callback handling form submission. */
+    /** @var JsCallback Callback handling form submission. */
     public $cb;
 
     /**
@@ -43,7 +44,7 @@ class Form extends View
      * Note:
      * When using your own change handler
      * on an input field, set useDefault parameter to false.
-     * ex: $input->onChange('console.log(), false)
+     * ex: $input->onChange(new JsExpression('console.log()), false)
      * Otherwise, change event is not propagate to all event handler
      * and leaving page might not be prevent.
      *
@@ -289,19 +290,18 @@ public function getControl(string $name): Control
     /**
      * Causes form to generate error.
      *
-     * @param string $fieldName Field name
-     * @param string $str       Error message
+     * @param string $errorMessage
      *
      * @return JsChain|array<int, JsChain>
      */
-    public function error($fieldName, $str)
+    public function error(string $fieldName, $errorMessage)
     {
         // by using this hook you can overwrite default behavior of this method
         if ($this->hookHasCallbacks(self::HOOK_DISPLAY_ERROR)) {
-            return $this->hook(self::HOOK_DISPLAY_ERROR, [$fieldName, $str]);
+            return $this->hook(self::HOOK_DISPLAY_ERROR, [$fieldName, $errorMessage]);
         }
 
-        $jsError = [$this->js()->form('add prompt', $fieldName, $str)];
+        $jsError = [$this->js()->form('add prompt', $fieldName, $errorMessage)];
 
         return $jsError;
     }
@@ -315,7 +315,7 @@ public function error($fieldName, $str)
      *
      * @return JsChain
      */
-    public function success($success = 'Success', $subHeader = null, $useTemplate = true)
+    public function success($success = 'Success', $subHeader = null, bool $useTemplate = true)
     {
         $response = null;
         // by using this hook you can overwrite default behavior of this method
@@ -391,7 +391,7 @@ public function addGroup($title = null)
      *
      * @return Jquery
      */
-    public function jsInput($name)
+    public function jsInput($name): JsExpressionable
     {
         return $this->layout->getControl($name)->js()->find('input');
     }