Callback/sticky args refactor #1377

ibelar · 2020-07-24T16:48:01Z

Refactor Callback

Post trigger has been remove! Triggering callback is done only via a $_GET argument.

Small BC break:

No more POST trigger;
Callback::triggered() method was used for two purposes: get callback mode (type) or check if callback was triggered. These functionalities are now split in two separate methods: Callback::getTriggeredValue() and Callback::isTriggered()

src/Callback.php

src/ItemsPerPageSelector.php

src/Callback.php

mvorisek · 2020-07-25T13:06:40Z

src/Callback.php

+    public function setUrlTrigger(string $trigger)
+    {
+        $this->urlTrigger = $trigger;
+        $this->app->stickyGet($this->urlTrigger);


I belive this concept is wrong, here is why, imagine the following structure:

- View elem - - Form A elem with callback inside (this sets stickyGet) - Form B elem with callback inside

if the last element does something with the URL, it will still add the sticky parameter

I think we should instead introduce sticky parameters per View element and then when we build URL for View, collect sticky parameters from the parents/owners.

Not sure I follow what you mean. Settings stickyGet to App vs View will only ensure that the get argument will be set in URL, no matter which View required it. Thus ensuring the callback will run.

I named elements in my example.

Now consider this:

Form A callback is triggered

and wants to return rendered Form B

now form B will have sticky GET from Form A

That is definitelly wrong - sticky GET should not be global, but per tree item and only self and parent items should contribute to the sticky GET params used for a new URL

src/Callback.php

src/Modal.php

- set Url argument properly when isPostTriggered; - clean up triggered() method

src/JsSortable.php

src/Form/Control/Multiline.php

mvorisek · 2020-07-25T18:53:25Z

src/Form/Control/Upload.php

@@ -230,9 +228,10 @@ protected function renderView(): void
        }
        parent::renderView();

-        if (!$this->_isCbRunning && (!$this->hasUploadCb || !$this->hasDeleteCb)) {
+        if (!$this->_isCbRunning && !isset($_GET['__atk_reload']) && (!$this->hasUploadCb || !$this->hasDeleteCb)) {


this check is still triggering/throwing in the nested CRUD

nested CRUD testing demo (you should be able to open/nest CRUD unlimited times and submit it):
https://github.com/atk4/ui/tree/reload_not_working_after_upload

we should test 3x (main + 2x) nested CRUD in tests

CRUD with upload (when you select a file and in uploads, then CRUD can not be submitted (it reloads and does not close))
https://github.com/atk4/ui/tree/reload_not_working_after_upload__add_submit_issue

ibelar · 2020-07-26T13:24:45Z

@georgehristov - @mvorisek - Any idea why Behat Crud test is failing since last Data update?
When in CRUD and search for "united kingdom", then edit the record, Normally after edit the CRUD reload and you should still see the United Kingdom record but it does not. The only change is that the query (_q) argument is changed from "united kingdom" to "united+kingdom" but this was working before...

mvorisek · 2020-07-26T13:47:38Z

last Data update

@ibelar you mean atk4/data update? If yes, than this is not true, the issue is not present on ui develop, this issue is solely in this PR.

ibelar · 2020-07-26T14:16:52Z

last Data update

@ibelar you mean atk4/data update? If yes, than this is not true, the issue is not present on ui develop, this issue is solely in this PR.

Issue is in develop

last Data update

@ibelar you mean atk4/data update? If yes, than this is not true, the issue is not present on ui develop, this issue is solely in this PR.

Nevermind, I see where the error comes from.

- remove app sticky; - remove post triggering support; - remove post triggereing test; - fix wizard triggered value

- remove post trigger check for form; - revert callback desired_name to submit for test; - fix test urlTrigger via setUrlTrigger

- cs fixer - inline edit index value

src/Form.php

mvorisek · 2020-07-28T18:16:10Z

src/Callback.php

-                return $ret;
-            }
+        if ($this->isTriggered()) {
+            $this->owner->stickyGet($this->urlTrigger);


I think we should not registed sticky GET even inside owner, this can/will be issue if the owner contains two Callbacks and at least one directly

I think we do not need this at all - sticky GET set in self::getUrlArguments()

Yes this is need, otherwise triggering element inside a callback will not run, i.e. callback in callback will not work without it.

then we need to move everything related with Url inside View inside a trait or make Callback class extends View :)

Yes this is need, otherwise triggering element inside a callback will not run, i.e. callback in callback will not work without it.

I looked at the code now and I think it is currently not even possible as URL method names in Callback vs. View are different, I think Callback now expect owner to be an instance of View strictly

ibelar · 2020-07-30T13:18:46Z

@mvorisek - Since your last changed, File upload demo (/demos/form-control/upload.php) and Wizard demo step 2 (/demos/interactive/wizard.php) are not working anymore. Please fix.

ibelar · 2020-07-30T13:42:58Z

src/Callback.php

+        return $this->urlTrigger;
+    }
+
+    private function callWithAppStickyTrigger(\Closure $fx)


Why do we need this? All was working perfectly fine before...

not, it was not - the described reload issue on Discord is not resolved (and even this does not solve it), I belive, it never will with app/global sticky args approach

This should be the same as before, but only the app/global sticky args are set when the callback is executing - once it is done, it is no longer app/global sticky - we need to converge to a solution without app/global sticky at all

I do not see any issue using your branch describe on Discord. All was working fine for me locally. I can even click 'Add Country" three time and Add will still work using the third modal.

https://discord.com/channels/657649422280425523/664534590160896006/737766184044199947

how to reproduce https://discord.com/channels/657649422280425523/664534590160896006/738128513412104303

mvorisek · 2020-07-30T13:57:53Z

/demos/form-control/upload.php

fixed, support of multiple files already in #1380

/demos/interactive/wizard.php (Wizard demo step 2)

yes, this issue is related with b459488 , can you look at it?

ibelar · 2020-07-30T14:19:09Z

@mvorisek - I revert Callback to the last working state. Let's merge this and we can look into your issue after.

mvorisek · 2020-07-30T15:06:02Z

@mvorisek - I revert Callback to the last working state. Let's merge this and we can look into your issue after.

I fixed the revert to revert only the too strict code

I belive these global sticky args must be removed, so I opened #1377 (comment) .

we should test 3x (main + 2x) nested CRUD in tests

please add some nested tests to this PR and then let's merge it
please also add test for Wizard demo (which will fail if we revert the revert)

mvorisek · 2020-07-30T19:33:41Z

demos/_unit-test/callback_2.php

+$loader = Loader::addTo($app);
+$loader->loadEvent = false;
+
+$loader->set(function($p) use ($m) {


should be CRUD (as it uses Popup) and probably it should be defined with function, which can be called unlimited times

This test is failing using the current develop branch. So should be enough for now.
But feel free to supply your own sample.

basically in reload_not_working_after_upload__add_submit_issue branch, addCrudDemo, can be very simple, I even do not think we need upload, but it will be nice if crud (like there) can be used for input for field, this is quite complex test which will be needed if we will drop sticky global later...

the field should be used twice and both fields should be edited twice (to simulate issues with with sticky...)

WIP - Callback refactor

c641fe7

ibelar added the BC-break label Jul 24, 2020

ibelar marked this pull request as draft July 24, 2020 16:48

georgehristov reviewed Jul 24, 2020

View reviewed changes

src/Callback.php Outdated Show resolved Hide resolved

mvorisek reviewed Jul 25, 2020

View reviewed changes

src/Callback.php Outdated Show resolved Hide resolved

mvorisek reviewed Jul 25, 2020

View reviewed changes

src/Callback.php Outdated Show resolved Hide resolved

mvorisek reviewed Jul 25, 2020

View reviewed changes

src/ItemsPerPageSelector.php Outdated Show resolved Hide resolved

mvorisek reviewed Jul 25, 2020

View reviewed changes

src/Callback.php Outdated Show resolved Hide resolved

mvorisek reviewed Jul 25, 2020

View reviewed changes

georgehristov reviewed Jul 25, 2020

View reviewed changes

src/Callback.php Outdated Show resolved Hide resolved

src/Modal.php Show resolved Hide resolved

ibelar added 2 commits July 25, 2020 10:32

clean up

13fde21

- set Url argument properly when isPostTriggered; - clean up triggered() method

fix rendering for Upload Control on a reload

658d316

georgehristov reviewed Jul 25, 2020

View reviewed changes

src/JsSortable.php Outdated Show resolved Hide resolved

mvorisek reviewed Jul 25, 2020

View reviewed changes

src/Form/Control/Multiline.php Outdated Show resolved Hide resolved

mvorisek reviewed Jul 25, 2020

View reviewed changes

mvorisek mentioned this pull request Jul 25, 2020

Allow upload multiple #1380

Merged

ibelar added 9 commits July 26, 2020 10:20

fix build query

0956d1f

Merge branch 'develop' into feature/refactor-callbacks

efb0f3b

wip

fcbeae7

- remove app sticky; - remove post triggering support; - remove post triggereing test; - fix wizard triggered value

fix unit test

ca8d431

- remove post trigger check for form; - revert callback desired_name to submit for test; - fix test urlTrigger via setUrlTrigger

more test fix

f7e8586

- cs fixer - inline edit index value

catch exception

f3d481e

fix demo

2c733fb

demos tutorial fix

82803b4

version fix

33f295a

mvorisek reviewed Jul 28, 2020

View reviewed changes

src/Form.php Outdated Show resolved Hide resolved

mvorisek reviewed Jul 28, 2020

View reviewed changes

mvorisek added 5 commits July 30, 2020 13:12

global sticky only when executing

b459488

Merge branch 'develop' into feature/refactor-callbacks

31a4eaf

small fix, improve cast

681cb6e

improve Callback::canTrigger() method

eea1a75

move canXxx method to a better place

8c503ff

ibelar commented Jul 30, 2020

View reviewed changes

fix upload unpacking

3bb7063

revert callback for Wizard fix

5b64bed

fix revert to revert "global sticky only when executing" only

baaaa18

mvorisek mentioned this pull request Jul 30, 2020

Sticky global must apply to self and subtree only #1385

Closed

ibelar added 4 commits July 30, 2020 13:44

add test for trigger on reload

6084f0e

add callback in callback test

846e500

cs fixer

43715f7

add comment

db13a46

mvorisek reviewed Jul 30, 2020

View reviewed changes

ibelar and others added 8 commits July 30, 2020 15:38

Fix button signature for Behat

98ab381

dam button

cc63104

test

a3d4cb4

fix test

9f31eb7

test

d31025e

damit

cc6b970

swap onUpload/onDelete before merge

9dd6191

Merge branch 'develop' into feature/refactor-callbacks

f9f2698

mvorisek approved these changes Aug 1, 2020

View reviewed changes

mvorisek merged commit 08644a6 into develop Aug 1, 2020

mvorisek deleted the feature/refactor-callbacks branch August 1, 2020 19:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Callback/sticky args refactor #1377

Callback/sticky args refactor #1377

ibelar commented Jul 24, 2020 •

edited

mvorisek Jul 25, 2020 •

edited

ibelar Jul 25, 2020

mvorisek Jul 25, 2020

mvorisek Jul 25, 2020 •

edited

ibelar commented Jul 26, 2020

mvorisek commented Jul 26, 2020

ibelar commented Jul 26, 2020

mvorisek Jul 28, 2020

ibelar Jul 28, 2020

mvorisek Jul 28, 2020 •

edited

ibelar commented Jul 30, 2020

ibelar Jul 30, 2020

mvorisek Jul 30, 2020 •

edited

ibelar Jul 30, 2020

mvorisek Jul 30, 2020

mvorisek commented Jul 30, 2020

ibelar commented Jul 30, 2020

mvorisek commented Jul 30, 2020 •

edited

mvorisek Jul 30, 2020

ibelar Jul 30, 2020

mvorisek Jul 30, 2020

Callback/sticky args refactor #1377

Callback/sticky args refactor #1377

Conversation

ibelar commented Jul 24, 2020 • edited

mvorisek Jul 25, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvorisek Jul 25, 2020 • edited

Choose a reason for hiding this comment

ibelar commented Jul 26, 2020

mvorisek commented Jul 26, 2020

ibelar commented Jul 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvorisek Jul 28, 2020 • edited

Choose a reason for hiding this comment

ibelar commented Jul 30, 2020

Choose a reason for hiding this comment

mvorisek Jul 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvorisek commented Jul 30, 2020

ibelar commented Jul 30, 2020

mvorisek commented Jul 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibelar commented Jul 24, 2020 •

edited

mvorisek Jul 25, 2020 •

edited

mvorisek Jul 25, 2020 •

edited

mvorisek Jul 28, 2020 •

edited

mvorisek Jul 30, 2020 •

edited

mvorisek commented Jul 30, 2020 •

edited