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

New feature: added mage_run_installed_exception event when uncatched exception is thrown #3613

Merged
merged 5 commits into from
May 1, 2024

Conversation

pquerner
Copy link
Contributor

Description (*)

This will add a new event called mage_installed_exception.
It will be thrown when OpenMage is installed and an uncatched exception is handled in the Mage::run method.

Using this new event it is possible to attach a exception facility, such as (Sentry)[https://sentry.io/].

Related Pull Requests

None

Fixed Issues (if relevant)

None, it got converted to a discussion: #3510

Manual testing scenarios (*)

  1. Throw exception anywhere thats unhandled by itself. For instance in an controller.
Mage::throwException('Dummy exception');
  1. It will finally be catched in Mage::run
  2. Listen to the new event, and do whatever you want with the exception. A use case has been discussed here: New event: mage_installed_exception (to i.e. forward exceptions to a 3rd party error logger, such as Sentry) #3510

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

@github-actions github-actions bot added the Mage.php Relates to app/Mage.php label Oct 26, 2023
@fballiano fballiano changed the title feat: add event when installed and a uncatched exception is thrown New feature: added event when installed and a uncatched exception is thrown Feb 16, 2024
app/Mage.php Outdated Show resolved Hide resolved
@fballiano
Copy link
Contributor

@pquerner what do you think about this pr and its relation with #3957?

@pquerner
Copy link
Contributor Author

pquerner commented Apr 26, 2024

I think having 1 event as shown here is enough to even implement this new Error library or any library for that matter. I don't really see the need to have more than that event.

Also it makes sure that the exception caught can only be from cgi context, as the ::printException can be called from different places, which ought to be handled either in the core or at the listener place (which I currently dont see in the 1 module implementing)

@fballiano
Copy link
Contributor

not really because there's still the need to override the default error handler and that needs to be in the initEnvironment, so at least 2 events are needed.

I'll check if the event added in this pr could be used also in the context of Ignition and get back here.

@empiricompany

@fballiano
Copy link
Contributor

@empiricompany could you test your ignition module listening to mage_run_exception and mage_installed_exception (i've to say i don't love the name, although technically correct) instead of the ones added in #3957?

@pquerner
Copy link
Contributor Author

pquerner commented Apr 26, 2024

My sentry implementation uses controller_action_predispatch to change the default error handler (cgi) and default for cli. (cron)

The new event core_app_init_environment_after would make it more visible for cgi, I agree. But for cron this should not get executed if I read that code correctly.

@fballiano
Copy link
Contributor

we'll test the ignition module with a mix of your other PR and this one and see if it works, probably the mix of the two will be the best implementation ;-)

docs/EVENTS.md Outdated Show resolved Hide resolved
@fballiano
Copy link
Contributor

i'd try to decide a name for this event and merge it asap so that we can test the Ignition thing with this new even and see if that's enough

pquerner and others added 2 commits April 30, 2024 15:27
Co-authored-by: Fabrizio Balliano <fabrizio.balliano@gmail.com>
Co-authored-by: Fabrizio Balliano <fabrizio.balliano@gmail.com>
@fballiano
Copy link
Contributor

since we kinda already had consensus, if nothing comes up I'll merge this tomorrow

@fballiano fballiano changed the title New feature: added event when installed and a uncatched exception is thrown New feature: added mage_run_installed_exception event when uncatched exception is thrown May 1, 2024
@fballiano fballiano merged commit f215834 into OpenMage:main May 1, 2024
17 checks passed
@pquerner pquerner deleted the feature/mageExceptionEvent branch May 21, 2024 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mage.php Relates to app/Mage.php
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants