Improve callbacks url generation 2

[mvorisek]

as per #1646 (comment)

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:

• 1st time http://localhost/dequ_ui_vendor/atk4/ui/demos/_unit-test/crud.php?__atk_cbtarget=_035c5ce8278dcf7e6c79c50288172d00__lexecutor_loader_callback&__atk_cb__035c5ce8278dcf7e6c79c50288172d00__lexecutor_loader_callback=callback&atk_layout_maestro_crud_useraction_modalexecutor=1&step=fields
• 2nd time http://localhost/dequ_ui_vendor/atk4/ui/demos/_unit-test/crud.php?atk_layout_maestro_crud_view_view_paginator=1&__atk_cbtarget=_035c5ce8278dcf7e6c79c50288172d00__lexecutor_loader_callback&__atk_cb__035c5ce8278dcf7e6c79c50288172d00__lexecutor_loader_callback=callback&atk_layout_maestro_crud_useraction_modalexecutor=1&step=fields

related with #2070

[ibelar]

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

[ibelar]

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]

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

[mvorisek]

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.

[mvorisek]

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]

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]

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

[ibelar]

@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.

[ibelar]

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.

[mvorisek]

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

any idea how to solve it?

[mvorisek]

@ibelar the issue is here:

ui/src/JsReload.php

Line 58 in 8014b6c

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

1. in Improve callbacks url generation #1646 you have removed mergeStickyArgsFromChildView which allowed to delegate sticky args from it's children.
2. in this PR, sticky args are collected from the render tree, but the URL must be obtained thru terminating callback
3. 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:

1. 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
2. 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]

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.