Skip to content

Do not handle exception in UserAction executor#1328

Closed
mvorisek wants to merge 1 commit intodevelopfrom
do_not_handle_exception_in_useractions
Closed

Do not handle exception in UserAction executor#1328
mvorisek wants to merge 1 commit intodevelopfrom
do_not_handle_exception_in_useractions

Conversation

@mvorisek
Copy link
Copy Markdown
Member

@mvorisek mvorisek commented Jun 26, 2020 •
edited
Loading

Opening this, because:

  1. when modal is opened like with "Add xxx" button in CRUD for new record popup
  2. and error is thrown during the action execution like when

like when model to add is defined like:

class ZInboundParcel extends \AppAtk4\MyModel {
...
    public function init(): void {
        parent::init();

        ...
        $this->addField('xxx', ['ui' => ['form' => [TextArea::class]]]);
        ...
    }
}

to replicate this issue like descibed, you need:

  • unix OS (otherwise TextArea.php source will be loaded)
  • to not use/load Textarea class before it
  • maybe also no autoload optimalization in composer (not sure, it may normalize class names thus making it CI)

This issue is not about refactoring, it is about supressing great error trace with absolutely unhelpful text

Before:
image

After:
image

Also notice, that the form was previously rendered, but partly one - WE SHOULD NEVER ALLOW PARTIAL RENDER (and partial submit)

Need to be probably solved a little bit differently - handlers/steps should be reset, but exception handler/renderer from App should be used.

@mvorisek mvorisek added Clarity 🔍 Makes things easier to understand. BLOCKED waitingForReaction labels Jun 26, 2020
@ibelar
Copy link
Copy Markdown
Contributor

ibelar commented Jun 27, 2020

@mvorisek - The original idea was for the user to be able to step back and give him a chance to change the argument value if the error was caused by the argument. But I agree that it does not make sense when an error like this is thrown.
Maybe we can keep it but only for error that would make sense, i.e. that a user can correct with a step back. I am not really sure how this would work either.

Maybe @romaninsh can comment further as he is the one who asks for the functionality at first.

@mvorisek
Copy link
Copy Markdown
Member Author

mvorisek commented Jun 27, 2020 •
edited
Loading

@ibelar I also pointed this (to step back), ok, for @romaninsh

I think this is related with other bugs related with "user can not return back on error" and "popup cleared but not closed on error" I already experienced.

Unhandled errors needs to be probably solved in browser level (as server/response might fail in any state)

UPDATE:
I hit this bug for another case.

I think we have to never step forward without success action execution result, so then we do not have to roll ui back.

@mvorisek mvorisek added bug 🪲 and removed Clarity 🔍 Makes things easier to understand. labels Jun 27, 2020
@mvorisek mvorisek force-pushed the do_not_handle_exception_in_useractions branch from 98bdcd1 to 1a658c1 Compare July 7, 2020 22:56
@mvorisek mvorisek mentioned this pull request Aug 17, 2020
Merged
@mvorisek mvorisek mentioned this pull request Feb 24, 2023
Merged
@mvorisek
Copy link
Copy Markdown
Member Author

mvorisek commented Feb 24, 2023

merged in #2008

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@romaninsh romaninsh Awaiting requested review from romaninsh

@DarkSide666 DarkSide666 Awaiting requested review from DarkSide666

@ibelar ibelar Awaiting requested review from ibelar

@abbadon1334 abbadon1334 Awaiting requested review from abbadon1334

@georgehristov georgehristov Awaiting requested review from georgehristov

Assignees

@romaninsh romaninsh

Labels

bug 🪲

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mvorisek @ibelar @romaninsh