Plugin settings bug(s) if no version 2 plugins enabled

[lgedgar]

• Version of CORE? upstream/master

• Issue with Office, Lane, or both? Office

• Is this [mostly] a bug report, feature request, or question? bug report

With a fresh install, InstallPluginsPage.php does not behave correctly for version 2 plugin settings, if no version 2 plugins are enabled. The main page loop emits a warning when loading / organizing the existing settings, then at the end it tries to write settings back to the DB, but fails.

I wasn't sure how you wanted this to work or else might have submitted a PR instead. But problems I see are:

• $dbSettings is defined before the main loop, and is used to write back to DB, but its contents are replaced within the loop, for the current plugin only.
• writing contents of $dbSettings back to DB then implies "all" plugin settings, but should it skip the disabled ones?

Assuming that CalendarPlugin is the only version 2 so far, reproducing should be as simple as disabling that plugin I think... Here are the warnings I saw:

[2021-09-11 19:42:17] fannie.WARNING: Undefined index: CalendarDatabase Line 248, /srv/corepos/upstream/fannie/IS4C/fannie/install/InstallPluginsPage.php [] []
[2021-09-11 19:42:17] fannie.WARNING: Failed Query on /upstream/install/InstallPluginsPage.php INSERT INTO PluginSettings (name, setting) VALUES (?, ?) Parameters: CalendarPlugin.CalendarDatabase Column 'setting' cannot be null  [] []

[gohanman]

I did indeed get the same warning after disabling the calendar plugin. This should solve those two warnings plus an additional one I discovered when disabling and enabling the plugin a few times.

[lgedgar]

Thanks! But now am wondering if maybe it should still save settings for disabled v2 plugins to the DB. If I enable the plugin and define its settings, then disable it momentarily e.g. to test something, when I re-enable all the settings will have reverted to default values.

[gohanman]

I would lean toward "no". The v1 plugins do revert to defaults, and having inconsistent behavior would be more confusing than helpful. At least with v2 you have the option of consulting a database backup.

[lgedgar]

Fair enough. I don't think this lacks anything else...