Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Presenter::$onAfterSignal event #309

Open
wants to merge 3 commits into
base: v3.1
Choose a base branch
from
Open

Presenter::$onAfterSignal event #309

wants to merge 3 commits into from

Conversation

mabar
Copy link
Contributor

@mabar mabar commented Oct 18, 2022

  • new feature
  • BC break? no

I am currently working on a Tracy panel which tracks submitted forms and shows errors from these forms.

This is useful for debugging manually rendered forms which don't render all form errors. e.g. datagrids

Problem is it is currently hooked via onStartup event. That works only for components that are created via createComponent*(), have all required parameters for creation available during startup and are not created manually - e.g. via $this['form'] = new Form(); in action*() method. It also changes order of code execution.

Having an onAfterSignal event would solve all of these issues and would also allow me to handle successfully submitted forms (which throw AbortException for redirect) and other signals

Current (bad) solution:

If you agree on proposed solution @dg I will write some tests for it.

$signalException = null;
try {
$this->processSignal();
} catch (Throwable $signalException) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the catch can be omitted

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is used to assign Throwable to $signalException

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove it too. Just try ... finally

@dg
Copy link
Member

dg commented Oct 18, 2022

Seem legit.

For consistency, it might be a good idea to add $onSignal too.

@dg dg force-pushed the v3.1 branch 9 times, most recently from 94b5e36 to 32db4c2 Compare October 9, 2023 01:32
@dg dg force-pushed the v3.1 branch 4 times, most recently from ae29a56 to e5e0e3b Compare November 2, 2023 00:09
@dg dg force-pushed the v3.1 branch 2 times, most recently from bfe61e7 to e697d90 Compare November 13, 2023 12:26
@dg dg force-pushed the v3.1 branch 8 times, most recently from 3e12237 to b170b62 Compare December 15, 2023 00:29
@dg dg force-pushed the v3.1 branch 7 times, most recently from 855889b to f503641 Compare May 12, 2024 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants