fisharebest / webtrees

Online genealogy
https://webtrees.net
GNU General Public License v3.0
488 stars 301 forks source link

Module's boot method still can crash webtrees. #2686

Closed bmarwell closed 5 years ago

bmarwell commented 5 years ago

Hi Greg,

you removed that second try-catch from https://github.com/fisharebest/webtrees/issues/2657.

The effect is the following:

If a module will now throw an exception in boot(), this will happen, on just every page:

2019-10-23T164409-Bildschirmfoto


If you re-insert that same try-catch-block, it will look like this:

2019-10-23T164258-Bildschirmfoto

(the above version still has two flash messages and I18N was removed).

Please reconsider adding that try-catch to not crash the whole website because one module has a faulty boot method.

bmarwell commented 5 years ago

To explain a little bit further, I do not think this was your intention.

You wrote:

https://github.com/fisharebest/webtrees/pull/2658#issuecomment-545392041https://github.com/fisharebest/webtrees/pull/2658#issuecomment-545392041

But if the boot fails, then the rest of the module will probably also fail and we will just get another error further on...

The error further on will only break one functionality, e.g. a Tab or a custom List. But without the try-catch, the whole website is crashed and unusable. I do not believe that this was your intention.

This is how my faulty module looks like with the try-catch readded (take a look at the URL):

2019-10-23T165552-Bildschirmfoto

Without the try-catch, it will look like the first screenshot. Even the start page is unusuable.

fisharebest commented 5 years ago

If an exception occurs, I think the best course of action is to display the error and stop.

the error further on will only break one functionality

...or it might break the entire site, or it might cause a confusing error message.

If the module is faulty/incompatible, the only solution is to remove/upgrade it.

If the constructor fails, it is safe to continue without the module. But if any other method fails, then it is not safe to continue.

We should not allow/encourage the user to use the site with a broken module.

bmarwell commented 5 years ago

@fisharebest then the modules do not seem modular enough for me. Why stop the entire site from working only because a single list module does not work or a single (unused?) translation is buggy? Or in the case of JSON+LD or cousins, if a single tab has stopped working?

vytux-com commented 5 years ago

I can almost see benefit of your argument for very simple modules such as you described. However the system also supports much more complex ones which would act as a middleware layer or even modify webtrees base functionality. So at what point do you stop treating them as just simple add ins?

fisharebest commented 5 years ago

@vytux-com - thanks. This is my exact point.

A module author can always create code that will break your error handling ;-)

I have updated the error page to make it more user-friendly:

Here's an example:

Screen Shot 2019-10-28 at 12 52 50
bmarwell commented 5 years ago

That's a really elegant solution! Thanks!

fisharebest commented 5 years ago

FYI, I am going to change the signature of the boot() method.

No parameters will be passed in.

Auto-injecting parameters will encourage developers to use classes/objects that do not yet exist.

I'll update the examples as well.