I noticed this issue while triaging #732. I installed v2.5.1 of ClassifAI and was trying out different combinations of settings and then upgrading to v3.0.0 to see if I could reproduce the fatal error. I noticed that for the Classification Feature, if I unchecked the taxonomy options, when migrating to v3.0.0, those settings ended up being checked, which I didn't expect.
In debugging this, found out the way we store those values has changed in v3.0.0. In v2.5.1 and earlier, these taxonomy options store a null value for options that are unchecked. But in v3.0.0, we have a merge_settings method that merges existing settings with default settings and uses an isset check. This check doesn't work for null values so the default ends up getting set instead.
It seems like a workaround is to use array_key_exists instead, which is what this PR does. Now I am worried about this impacting other things though I haven't run into any issues in my testing. This also is a fairly minor issue that seems to only impact those migrating to v3.0.0 and can easily be fixed by validating the settings so maybe this can be closed out as a wontfix, open to thoughts there.
How to test the Change
Install v2.5.1 of ClassifAI
Set up the IBM Watson classification feature, turning off the Category option
Upgrade to v3.0.0 of ClassifAI
Go to the Classification Feature settings and see that the Category option is now checked
Checkout this PR and see that the Category option is no longer checked
Changelog Entry
Fixed - Ensure we properly account for null values when merging our saved settings with our default settings.
Description of the Change
I noticed this issue while triaging #732. I installed v2.5.1 of ClassifAI and was trying out different combinations of settings and then upgrading to v3.0.0 to see if I could reproduce the fatal error. I noticed that for the Classification Feature, if I unchecked the taxonomy options, when migrating to v3.0.0, those settings ended up being checked, which I didn't expect.
In debugging this, found out the way we store those values has changed in v3.0.0. In v2.5.1 and earlier, these taxonomy options store a
nullvalue for options that are unchecked. But in v3.0.0, we have amerge_settingsmethod that merges existing settings with default settings and uses anissetcheck. This check doesn't work fornullvalues so the default ends up getting set instead.It seems like a workaround is to use
array_key_existsinstead, which is what this PR does. Now I am worried about this impacting other things though I haven't run into any issues in my testing. This also is a fairly minor issue that seems to only impact those migrating to v3.0.0 and can easily be fixed by validating the settings so maybe this can be closed out as awontfix, open to thoughts there.How to test the Change
Changelog Entry
Credits
Props @dkotter
Checklist: