Improve callbacks url generation 2 by mvorisek · Pull Request #1651 · atk4/ui

mvorisek · 2021-07-22T00:29:38Z

replace https://github.com/atk4/ui/pull/1646/files#diff-489a31f422a2238af13855d2db35e0df62c99606b21cb5c678330f7f857ea90eR53
because of removed
https://github.com/atk4/ui/pull/1646/files#diff-1314d7908e6167b9fbe3d52c6f0c3cadf76637c75eca9c9c36d66106c579edd3L120
and
https://github.com/atk4/ui/pull/1646/files#diff-489a31f422a2238af13855d2db35e0df62c99606b21cb5c678330f7f857ea90eL148

The motivation is to make URL generation independed of the actual request.

Currently, for ex. CRUD generates some link to open a Modal. But when it is opened & saved, a different link is generated to open is again, it can be obseved on the demos/_unit-test/crud.php demo:

related with #2070

ibelar · 2021-07-22T01:51:49Z

Tks! I fixed test and also added a behat test for url

tests/CallbackTest.php

ibelar · 2021-07-22T14:34:44Z

I can update the PR using a static property for runningCallback. Then we can grab argument from it when need.


class Callback 
{
   
    protected static $runningCallbacks = [];

    public static function getRunningCallbackArgs(): array
    {
        $args = [];
        foreach (self::$runningCallbacks as $cb) {
            $args[$cb->urlTrigger] = $cb->getTriggeredValue();
        }

        return $args;
    }

    public function set($fx = null, $args = null)
    {
        if ($this->isTriggered() && $this->canTrigger()) {
            self::$runningCallbacks[] = $this;
            return $fx(...($args ?? []));
        }
    }
}

This way, we do not need to check for Get value and also do not need public const URL_QUERY_TRIGGER_PREFIX

@mvorisek What do you think?

ibelar · 2021-07-22T15:15:25Z

I have made the changes. Feel free to revert if you do not like it.

mvorisek · 2021-07-22T15:38:08Z

I can update the PR using a static property for runningCallback. Then we can grab argument from it when need.

I belive running callbacks can be obtained quite easily by traversing the parents /wo the need to persist them in a static array, I will impl. that later.

src/View.php

mvorisek · 2021-07-26T13:52:19Z

src/VirtualPage.php

        parent::init();

        $this->cb = $this->add([Callback::class, 'urlTrigger' => $this->urlTrigger ?: $this->name]);
+        unset($this->{'urlTrigger'});


this is a very bad hack to dedup urlTrigger property stored at 2 places, but let's keep is for now, related with atk4/core#245 , otherwise this property can not be set from seed

ibelar · 2021-07-26T22:30:19Z

In theory, C3 should contains trigger for the other callbacks only if they where activated (Callback::set()). Maybe you could check for this condition before adding the trigger. Envoyé de mon iPhone

…

Le 26 juill. 2021 à 16:56, Michael Voříšek ***@***.***> a écrit : @mvorisek commented on this pull request. In src/View.php: > @@ -613,16 +613,38 @@ public function jsUrl($page = []) */ public function url($page = []) { - return $this->getApp()->url($page, false, $this->_getStickyArgs()); + return $this->getApp()->url($page, false, array_merge($this->getRunningCallbackArgs(false, is_array($page) ? $page : []), $this->getStickyArgs())); + } + + protected function getRunningCallbackArgs(bool $isTerminated, array $page): array + { + $args = []; + foreach ($this->elements as $v) { + if ($v instanceof Callback) { // @phpstan-ignore-line + if (($page[Callback::URL_QUERY_TARGET] ?? null) === $v->getUrlTrigger()) { any idea what is wrong with the currect algorithm? Consider this render tree: App -> V1 -> C1 -> C2 -> V2 -> C3 currently, when URL for C3 is rendered, both C1 and C2 URL triggers are added. Is that correct? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

mvorisek · 2021-07-27T09:22:11Z

src/View.php

+                    $isTerminated = true;
+                }
+
+                if ($isTerminated && $v->isTriggered() && $v->canTrigger()) {


@ibelar does $v->canTrigger() make sense here and any idea how to fix the remaining failing Behat tests?

@ibelar does $v->canTrigger() make sense here and any idea how to fix the remaining failing Behat tests?

Yes, canTrigger method will check for a reload. Normally, JsCallback should not run during a reload.

I see why the test is failing. ViewTester reload is missing Callback arguments. Thus when reload is call, the callback method is not run.

I think the reason is that ViewTester is added directly to Modal, while the callback is added to Button

$button->on('click', $vp1Modal->show());

Resulting in

App -> Button -> Cb -> Modal -> ViewTester (reload)

Thus the callback is not part of the Modal render tree... missing its arguments when reload URL is rendered.

Thus the callback is not part of the Modal render tree...

any idea how to solve it?

@ibelar the issue is here:

ui/src/JsReload.php

Line 58 in 8014b6c

'uri' => $this->view->jsUrl(['__atk_reload' => $this->view->name]),

in Improve callbacks url generation #1646 you have removed mergeStickyArgsFromChildView which allowed to delegate sticky args from it's children.

in this PR, sticky args are collected from the render tree, but the URL must be obtained thru terminating callback

here, hovewer the URL is obtained thru $this->view which is placed correctly within the Modal render tree, but the Modal is not delegating it's callback sticky args as mergeStickyArgsFromChildView was removed

To better understand the issue, I have pushed a debug code in 53a0352#diff-0741d95fd53e7a9a50ab8b371b0d621c3eaf3cb460c79dfe5a886deaec0814a9R68 , when you open the test URL, the expected and actual value must be the same.

I belive, mergeStickyArgsFromChildView is needed. Or how else should the $this->view->jsUrl(... in JsReload work?

With 29ecd83#diff-c5bb4a733c5ddd3bb0744ef4c29a8fd1e0bd5999fcf06b7f3cfef4c3d1926ce3R640-R668 Behat passes.

Here is a summary of problems to solve:

traverse all callbacks that CAN trigger (can be solved easily but by brute force by analysis whole reachable render tree) - temporary implemented by analysis stack trace

identify THE LAST TERMINATING callback - temporary implemented by pretending that all Modals can terminate - I need a help with this, as terminating callback must be identified from the render tree (not by the user requested terminating callback from URL, as it can differ)

ibelar · 2021-07-27T13:57:06Z

As a quick fix, one way is to collect Callback trigger argument in a static property. Other than that, I do not know. Envoyé de mon iPhone

…

Le 27 juill. 2021 à 09:38, Michael Voříšek ***@***.***> a écrit : @mvorisek commented on this pull request. In src/View.php: > @@ -613,16 +613,38 @@ public function jsUrl($page = []) */ public function url($page = []) { - return $this->getApp()->url($page, false, $this->_getStickyArgs()); + return $this->getApp()->url($page, false, array_merge($this->getRunningCallbackArgs(false, is_array($page) ? $page : []), $this->getStickyArgs())); + } + + protected function getRunningCallbackArgs(bool $isTerminated, array $page): array + { + $args = []; + foreach ($this->elements as $v) { + if ($v instanceof Callback) { // @phpstan-ignore-line + if (($page[Callback::URL_QUERY_TARGET] ?? null) === $v->getUrlTrigger()) { + $isTerminated = true; + } + + if ($isTerminated && $v->isTriggered() && $v->canTrigger()) { Thus the callback is not part of the Modal render tree... any idea how to solve it? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

mvorisek · 2025-03-31T09:57:55Z

Probably impossible to satisfy all goals with the current design - new URL handling and multipass rendering needed.

mvorisek requested a review from ibelar July 22, 2021 00:29

mvorisek mentioned this pull request Jul 22, 2021

Improve callbacks url generation #1646

Merged

mvorisek force-pushed the improve_callback_url branch from 14d0172 to 9b532df Compare July 22, 2021 00:32

mvorisek commented Jul 22, 2021

View reviewed changes

tests/CallbackTest.php Outdated Show resolved Hide resolved

mvorisek force-pushed the improve_callback_url branch from de1780f to 077d40e Compare July 24, 2021 11:11

mvorisek mentioned this pull request Jul 25, 2021

Feature/Virtual Page Action Executor #1649

Merged

mvorisek force-pushed the improve_callback_url branch 5 times, most recently from a221eca to 1cf01e8 Compare July 26, 2021 13:38

mvorisek commented Jul 26, 2021

View reviewed changes

src/View.php Show resolved Hide resolved

mvorisek commented Jul 26, 2021

View reviewed changes

mvorisek force-pushed the improve_callback_url branch 8 times, most recently from f271757 to 428165b Compare July 27, 2021 09:15

mvorisek commented Jul 27, 2021

View reviewed changes

mvorisek force-pushed the improve_callback_url branch from 428165b to 5a5ff3a Compare July 27, 2021 09:23

mvorisek marked this pull request as ready for review July 27, 2021 11:07

mvorisek force-pushed the improve_callback_url branch 4 times, most recently from b1e9b14 to 982c631 Compare October 7, 2021 00:00

mvorisek mentioned this pull request Oct 21, 2021

Fix and test nothing is GET sticky globally #1671

Merged

mvorisek force-pushed the improve_callback_url branch from 982c631 to f985362 Compare December 3, 2021 23:36

mvorisek added the MAJOR label Apr 8, 2022

mvorisek force-pushed the improve_callback_url branch 4 times, most recently from 86ea86e to c592b8f Compare August 28, 2022 18:34

mvorisek force-pushed the improve_callback_url branch from c592b8f to e118723 Compare June 18, 2023 22:07

mvorisek marked this pull request as draft December 30, 2024 09:53

mvorisek force-pushed the improve_callback_url branch from e118723 to 82fad5f Compare March 29, 2025 12:22

mvorisek added 12 commits March 29, 2025 13:23

faked global sticky via _GET filter

3341ad7

fix test and add behat test

8d59993

Revert "faked global sticky via _GET filter"

1ba0d76

track running callbacks using static variable

7f87a14

Revert "track running callbacks using static variable"

72ace2f

collect running callback triggers directly from render tree

07809fb

remote unneeded behat test

2733e49

collect triggers with Callback::set condition

6e3d4e9

callback always trigger on reload

39c27c3

Revert "callback always trigger on reload"

bf6d0ff

DEBUG better understand the issue/render tree

e893e42

DEBUG fix uncollected callbacks by analysing stack trace

e086dd9

mvorisek force-pushed the improve_callback_url branch from 82fad5f to e086dd9 Compare March 29, 2025 22:19

mvorisek closed this Mar 31, 2025

mvorisek deleted the improve_callback_url branch March 31, 2025 09:58

Conversation

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

Uh oh!

Uh oh!

ibelar commented Jul 22, 2021

Uh oh!

Uh oh!

ibelar commented Jul 22, 2021

Uh oh!

ibelar commented Jul 22, 2021

Uh oh!

mvorisek commented Jul 22, 2021

Uh oh!

Uh oh!

mvorisek Jul 26, 2021

Choose a reason for hiding this comment

Uh oh!

ibelar commented Jul 26, 2021 via email

Uh oh!

mvorisek Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

ibelar Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

ibelar Jul 27, 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!

mvorisek Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

mvorisek Aug 19, 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!

ibelar commented Jul 27, 2021 via email

Uh oh!

mvorisek commented Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

mvorisek commented Jul 22, 2021 •

edited

Loading

ibelar Jul 27, 2021 •

edited

Loading

mvorisek Aug 19, 2021 •

edited

Loading

mvorisek commented Mar 31, 2025 •

edited

Loading