Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change how we check for settings to support null values #741

Merged
merged 2 commits into from
Mar 7, 2024

Conversation

dkotter
Copy link
Collaborator

@dkotter dkotter commented Mar 6, 2024

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 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

  1. Install v2.5.1 of ClassifAI
  2. Set up the IBM Watson classification feature, turning off the Category option
  3. Upgrade to v3.0.0 of ClassifAI
  4. Go to the Classification Feature settings and see that the Category option is now checked
  5. 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.

Credits

Props @dkotter

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@dkotter dkotter added this to the 3.0.1 milestone Mar 6, 2024
@dkotter dkotter self-assigned this Mar 6, 2024
@dkotter dkotter requested review from jeffpaul and a team as code owners March 6, 2024 16:41
@dkotter dkotter requested review from peterwilsoncc and removed request for a team and jeffpaul March 6, 2024 16:41
@github-actions github-actions bot added the needs:code-review This requires code review. label Mar 6, 2024
Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me and tests well

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.

Reviewing the docs for isset() vs array_key_exists() I can not see it causing any issues. It's something that's been done a few times in WordPress Core without any apparent illeffect.

I've approved the PR but will let you merge it in case you get other feedback that this could be a problem.

@dkotter dkotter merged commit 7ea7701 into develop Mar 7, 2024
13 checks passed
@dkotter dkotter deleted the fix/null-settings branch March 7, 2024 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:code-review This requires code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants