Partial fix Buggy add-on can stop mass toggling add-ons

[Jon W]

Not entirely sure if this is a suggestion or a bug, but I suspect bug.

If an add-on throws/returns an error when an attempt is made to disable it, this could halt the mass toggle add-ons process half way, having only disabled the add-ons that appear in the list before this buggy add-on.

This is made worse by refreshing the page, which causes the process to re-run using the current list of enabled add-ons, which means the original cached list of enabled add-ons is now gone.

There might also be legitimate reasons for an add-on to show a warning or throw/return an error if it is disabled. For example, it might be that an add-on stops itself from being disabled if it knows that disabling itself would cause serious errors. For example (although it doesn't actually do this), an add-on like NoForo by Waindigo (which deletes core database tables) might stop itself from being disabled.

Perhaps the mass toggle code could be included in a try/catch block and/or the XenForo_DataWriter::ERROR_SILENT error handler could be enabled.

Thanks.

 

[Mike]

I've adjusted this as far as possible. There's a separate preSave loop and the save is wrapped in a transaction.

However, I would note that there isn't really an expectation that toggling the active state of an add-on is something that a) an add-on would involve itself with, or b) that an add-on can prevent. If disabling an add-on would cause serious errors, that add-on really shouldn't be approached like that. If other add-ons depend on that add-on and it's not available, they should behave "defensively" and ensure that they get don't errors (by checking for the other add-on in the add-on cache).

I'm tagging this as a partial fix because it's certainly possible for an add-on to do something that will break the defensive code we have added (especially if it's executing DDL queries), but I would suggest if that happens, the add-on should be reapproached.