October: Syntax errors in plugin registration files should halt execution

[multiwebinc]

As it is right now, if there is a syntax error in a Plugin.php file, that plugin will simply be silently ignored. When viewing the list of plugins in the settings, it will indicate that the plugin has been removed from the filesystem. This is really an unexpected way to handle this, and I would expect a syntax error in any loaded file to instead cause an error/exception indicating the particular file and line number and then die, which is standard PHP behavior.

See: System\Classes\PluginManager::loadPlugin(). What about re-throwing $e after the error is logged?

[bennothommo]

@multiwebinc This is a blast from the past - it was one of my first contributions to October!

The justification is that a failing plugin should not bring down a website - especially if it is one sourced from the marketplace via a Project ID that is inserted into a production website. October can mostly gracefully handle a failed plugin and work around it so that all other functions work as expected, and once optional components are implemented, it should work even better around missing or failing plugins.

The failure of the plugin is logged in the error logs that the plugin could not be instantiated, so most developers should be able to find the issue quickly if need be. Given that failures with the plugins could theoretically be introduced by an update as well, it was safer to let sites run without the failing plugin as opposed to harshly stopping execution with an error.

[multiwebinc]

@bennothommo That makes sense. We wouldn't want the site to be totally inaccessible because of a plugin update. Then again, a missing plugin could prevent vital functionality for the website without the website owner even knowing about it.

What about doing something like this in the catch block?

\Flash::error('Plugin ' . $className . ' could not be instantiated. The error was: ' . $e->getMessage() . ' in ' . $e->getFile() . ' on line ' . $e->getLine() . '.');

This way the website owner will know something is wrong with the plugin in the backend, but I think this shouldn't affect the frontend.

I had this error today, but I didn't think to check the log file, so it took me way longer than it should have to diagnose why it was telling me the plugin had been removed.

[Samuell1]

How you do flash when backend cant load because of broken plugin? Thats why website owner should look at error logs after every update to be 100% sure everything is fine.

[multiwebinc]

@Samuell1 I meant to do that instead of rethrowing the Exception. The backend will still work fine; you just get that flash message on every page load.

[Samuell1]

Maybe it should work when debug is enabled then it should throw error page?

[multiwebinc]

@Samuell1 Keep in mind that this could happen from a plugin update from the marketplace, in which case the site owner is more likely to have debug mode disabled if the site is live.

[36864]

Seems to me that just changing the message displayed in the plugin list to "This plugin could not be initialized, check the system log for details" would be sufficient.

[bennothommo]

That's a great idea, @36864.

[bennothommo]

@multiwebinc I'm personally not a fan of a "nag" flash message appearing every page load because a plugin has failed. For example, I may already know that the plugin doesn't work and know that it won't impede on the website at that point in time. Also, if it were to happen on a production website which a client accesses, they may be frustrated by a constant flash message appearing.

[github-actions[bot]]

This issue will be closed and archived in 3 days, as there has been no activity in the last 30 days. If this issue is still relevant or you would like to see action on it, please respond and we will get the ball rolling.