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

Introduce the usage of Module user-displayable exceptions to handle module errors (part 2) #14239

Merged
merged 2 commits into from Jun 18, 2019

Conversation

matks
Copy link
Contributor

@matks matks commented Jun 17, 2019

Questions Answers
Branch? 1.7.6.x
Description? Allow hooked modules to throw a ModuleErrorException or ModuleErrorInterface to display error messages to end-user
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #13490 last issue: "How to handle errors happening inside module (translation + display to user)"
How to test? See below

How to test

Attached is a demonstration module.
module_for_pr_hook_error.zip

If you install it and test this PR,

  • on Add form, if you save you'll see an error message "An unexpected error occurred. [RuntimeException code 0]"
  • on Edit form, if you save you'll see an error message "This is a nice display message"

PR notes

Background

This PR solves 2 issues:

  • How to display error messages from a module hooked on a Add/Edit form process (see Issues when creating new module for PS 1.7.6 #13490)
  • It also handles the usecase "module does throw an uncatched exception". In the Core, we do not catch the generic \Exception because we consider our duty to handle all exception paths and make sure we display the right error message for each, however we cannot guarantee Modules will do the same. So we catch \Exception in order to avoid the "blank page" behavior.

This change is Reviewable

@matks matks force-pushed the workaround-for-error-in-modules-3 branch from ec0216f to 16c0b01 Compare June 17, 2019 09:09
* If an exception that implements this ModuleErrorInterface
* is thrown, its message will be displayed to the end-user
*/
interface ModuleErrorInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering is it ok to have an empty interface? I don't mind it's just I wasn't sure it was possible

I had thought of a single getErrorMessage method which would allow to "customize" the exception message a bit (the simplest implementation being to simply return the exception message). This would allow to separate the exception message and the error message which might be different:

  • exception message => technical message with (potentially) sensitive info (db infos, path infos, ...)
  • error message => user message more "feature oriented" and safe to display

Besides by using ModuleErrorInterface you restrict its use to module only, maybe we could call it FlashErrorInterface which would allow us to use it with core exceptions as well this could simplify our internal getErrorMessages methods (yet it doesn't make this PR more complicated), wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

Ideally it should extend Throwable, then it wouldn't be empty. But it's not possible. It's just a marker interface so that you have more flexibility to create your own exceptions in this specific case.

From a DX standpoint, it's easier to use the exception message. Regarding FlashErrorInterface it could be integrated in the core later and be made a parent to this one.

Copy link
Member

Choose a reason for hiding this comment

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

Ideally it should extend Throwable, then it wouldn't be empty. But it's not possible. It's just a marker interface so that you have more flexibility to create your own exceptions in this specific case.

From a DX standpoint, it's easier to use the exception message. Regarding FlashErrorInterface it could be integrated in the core later and be made a parent to this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok let's do that for now then :)

Copy link
Contributor

@jolelievre jolelievre left a comment

Choose a reason for hiding this comment

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

Thanks @matks

@matks
Copy link
Contributor Author

matks commented Jun 18, 2019

I fixed the empty demo module, QA can proceed 🚀

@matks matks added the Waiting for QA Status: action required, waiting for test feedback label Jun 18, 2019
@sarahdib sarahdib added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Jun 18, 2019
@matks
Copy link
Contributor Author

matks commented Jun 18, 2019

Thank you for review and QA 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.7.6.x Branch Improvement Type: Improvement QA ✔️ Status: check done, code approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants