UserAction StepExecutor improvement by ibelar · Pull Request #1687 · atk4/ui

ibelar · 2021-11-04T19:01:20Z

Refactor in order to improve Class extension;
Allow settings Data\UserAction argument from a Data\Model
- arguments can be validated using Model validation.

mvorisek · 2021-11-04T20:26:34Z

src/UserAction/StepExecutorTrait.php

+     */
+    protected function getModelForArgs(): Model
+    {
+        $this->cloneAction = clone $this->getAction();


why do we need to clone, what is the purpose of using cloned&modified action internally?

this can lead also to many bugs when the action is semi-shallowly cloned, like when it has Closures bound still to the original action

Cloning was to duplicate Action arguments when set by model only, i.e. each editable model field need to be added as an argument to the UserAction. However, I realized I do not need to clone UserAction but only it's arguments.

A fix is on the way.

src/UserAction/StepExecutorTrait.php

mvorisek · 2021-11-04T22:24:08Z

src/UserAction/StepExecutorTrait.php

+            $argsModel = Factory::factory($args['__atk_model']);
+            // if seed is supplied, we need to initialize.
+            if (!$argsModel->_initialized) {
+                $argsModel->invokeInit();


Model::init is invoked when persistence is set

when not initialized, I belive we should set new Array_([]) persistence instead

side question, when custom model for args does bring any advantage?

argument can be easily validate using a Validator;

argument can be easily define within a model;

when not initialized, I belive we should set new Array_([]) persistence instead

If using seed then developper should supply proper property within the seed array, including Persistence.

mvorisek · 2021-11-04T22:39:44Z

demos/_includes/DemoActionsUtil.php

+                'caption' => 'Arg with Model',
+                'description' => 'Ask for Arguments set via a Data\Model. Allow usage of model validate() for your arguments',
+                'args' => [
+                    '__atk_model' => new ArgModel(new Array_([])),


[ArgModel::class] should be probably here

It support both.

yes, more like recommended usage ;-)

demos/_includes/DemoActionsUtil.php

ibelar · 2021-11-05T15:32:43Z

Question:
Will you be ok to add a property to Model\UserAction say $argModel instead of using a special key name '__atk_model' in args array?

Then executor can check for both UserAction property ($arg and $argModel) accordingly.

mvorisek · 2021-11-05T15:41:18Z

Question: Will you be ok to add a property to Model\UserAction say $argModel instead of using a special key name '__atk_model' in args array?

Then executor can check for both UserAction property ($arg and $argModel) accordingly.

~~If Model\UserAction::$args can be replaced with a Model, I would prefer that as otherwise args are passed like ...$args which has no typing check neither correct arg names/order/count.~~

I think lets use/stay with __atk_model for now as the args are not refactorable currently anyway. Maybe add it as a const to Model\UserAction class prefixed with the cl. name as other constants.

ibelar · 2021-11-05T15:53:30Z

I am currently checking passing the entire $argModel to callback function as well instead of passing $args separately.
This will allow full usage of Model capability for action argument i.e usage of hook within the model.

mvorisek · 2022-05-22T10:39:02Z

@ibelar how does this PR fix #1680 and what do actaully mean by

Current implementation does not allow for easily overriding each step method

as described in the issue desc?

mvorisek · 2022-05-22T11:58:29Z

src/UserAction/StepExecutorTrait.php

        $this->jsSetPrevHandler($page, $this->step);

        $form->onSubmit(function (Form $form) {
+            $form->model->save();


why is now save needed?

mvorisek · 2022-05-22T12:01:48Z

src/UserAction/StepExecutorTrait.php

    public $finalMsg = 'Complete!';

+    /** @var array An extended copy of UserAction arguments. It contains original action arguments and arguments set by '__atk_model'. */
+    private $cloneArgs;


I think we should add native Model args support to atk4/data model. It is also not completely clear to me, why there are args & fields. In atk4/data, the UserAction::$fields property is described to be used for dirty detection only.

mvorisek · 2023-04-29T17:13:08Z

demos/_includes/DemoActionsUtil.php

+ *  @property string $city   @Atk4\Field()
+ *  @property string $gender @Atk4\Field()
+ */
+class ArgModel extends Model


our testing/demos Model should be used here to test if field aliasing is used properly

mvorisek · 2024-11-23T00:30:55Z

I like the idea, however this should be addressed mainly in atk4/data. Then adjusting atk4/ui is an easy job.

As it seems you do not plan to work on this topic in the near future, I am closing this PR. Thank you for understanding.

UserAction StepExecutor improvement

185747f

ibelar marked this pull request as draft November 4, 2021 19:10

ibelar added 3 commits November 4, 2021 15:34

fix card deck - clone action for setting proper argument as a model

a2d793b

revert card deck clone action

4d70967

clone action at executor - leave original action un touch

90d123f

ibelar marked this pull request as ready for review November 4, 2021 20:17

ibelar added the RTM label Nov 4, 2021

ibelar requested a review from mvorisek November 4, 2021 20:17

mvorisek added the waitingForAnotherPR label Nov 4, 2021

mvorisek reviewed Nov 4, 2021

View reviewed changes

ibelar added 3 commits November 4, 2021 16:54

cloning UserAction arguments only instead of UserAction

51d5d0c

added comment

734602e

cs fix comment

8bbbda8

ibelar marked this pull request as draft November 4, 2021 21:04

ibelar removed the RTM label Nov 4, 2021

added default argument as empty array

8564190

mvorisek reviewed Nov 4, 2021

View reviewed changes

src/UserAction/StepExecutorTrait.php Outdated Show resolved Hide resolved

mvorisek reviewed Nov 4, 2021

View reviewed changes

src/UserAction/StepExecutorTrait.php Show resolved Hide resolved

mvorisek removed the waitingForAnotherPR label Nov 4, 2021

ibelar added 2 commits November 4, 2021 18:02

Merge branch 'develop' into feature/step-executor-enhancement

9c8795c

fix phpstan

5fec5da

ibelar marked this pull request as ready for review November 4, 2021 22:16

ibelar added the RTM label Nov 4, 2021

mvorisek reviewed Nov 4, 2021

View reviewed changes

src/UserAction/StepExecutorTrait.php Outdated Show resolved Hide resolved

mvorisek reviewed Nov 4, 2021

View reviewed changes

src/UserAction/StepExecutorTrait.php Show resolved Hide resolved

mvorisek reviewed Nov 4, 2021

View reviewed changes

remove array filter for UserAction fields

dbd95de

mvorisek reviewed Nov 4, 2021

View reviewed changes

demos/_includes/DemoActionsUtil.php Outdated Show resolved Hide resolved

mvorisek reviewed Nov 4, 2021

View reviewed changes

demos/_includes/DemoActionsUtil.php Outdated Show resolved Hide resolved

mvorisek removed the RTM label Nov 12, 2021

mvorisek added 2 commits November 12, 2021 01:02

Merge branch 'develop' into feature/step-executor-enhancement

569ad99

no need to reset id field type to string, guessing fixed in data

8722647

mvorisek force-pushed the feature/step-executor-enhancement branch from fcdee8f to 8722647 Compare November 12, 2021 10:32

mvorisek added 3 commits May 22, 2022 12:16

Merge branch 'develop' into feature/step-executor-enhancement

5f318b0

adjust to the latest atk4

fcce80d

fix cs

5a1a59f

mvorisek added the waitingForReaction label May 22, 2022

mvorisek mentioned this pull request May 22, 2022

Cherrypick non-functional changes from 1687 PR #1794

Merged

mvorisek added 2 commits May 22, 2022 13:51

update comments before merge

fb8cb1a

Merge branch 'develop' into feature/step-executor-enhancement

9af3020

mvorisek reviewed May 22, 2022

View reviewed changes

fix merge

67fab83

mvorisek added waiting for author and removed waitingForReaction labels May 22, 2022

Merge branch 'develop'

d0ab1c3

mvorisek force-pushed the feature/step-executor-enhancement branch from 8688dc7 to d0ab1c3 Compare April 29, 2023 17:07

mvorisek reviewed Apr 29, 2023

View reviewed changes

mvorisek mentioned this pull request Aug 16, 2023

Normalize user action args atk4/data#1112

Draft

mvorisek added 3 commits November 23, 2024 01:03

Merge branch 'develop'

5efb83f

fix merge

db2075d

fix merge II

170e710

mvorisek closed this Nov 23, 2024

mvorisek deleted the feature/step-executor-enhancement branch November 23, 2024 00:30

Conversation

ibelar commented Nov 4, 2021 • edited by mvorisek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibelar Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ibelar commented Nov 5, 2021

Uh oh!

mvorisek commented Nov 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ibelar commented Nov 5, 2021

Uh oh!

mvorisek commented May 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mvorisek commented Nov 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

ibelar commented Nov 4, 2021 •

edited by mvorisek

Loading

ibelar Nov 4, 2021 •

edited

Loading

mvorisek commented Nov 5, 2021 •

edited

Loading