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

Ensure both arguments passed to merge_settings are an array #733

Merged
merged 5 commits into from
Mar 7, 2024

Conversation

dkotter
Copy link
Collaborator

@dkotter dkotter commented Mar 2, 2024

Description of the Change

We have a merge_settings method that takes in stored settings as well as the default settings and merges those together. This method expects both of these values to be an array, so if non-array data is passed in, a PHP error occurs.

This method is called recursively to merge settings that contain array data. This was throwing a fatal error in #732 because the passed in settings were not an array but the default settings were.

This PR adds additional checks here to ensure we only ever call merge_settings if both the stored settings and default settings are an array. For instances were the stored settings aren't an array but the defaults are, we override the stored settings with the defaults as it seems the only time this happens is when data has been stored improperly (or not migrated properly in v3.0.0).

In addition, some followup in #732 pointed to some additional issues in the descriptive text generator feature where it expects array data but is instead passed in non-array data. In theory this should never happen but again, in scenarios where data is stored improperly or migrated wrong, I've added additional checks to avoid PHP errors and warnings.

Closes #732

How to test the Change

There are good details in #732 but I've not been able to replicate the problem yet without manually manipulating data.

Here's the steps I followed:

  1. Install the 2.5.1 version of ClassifAI
  2. Configure settings as seen in the screenshots from Update from 2.5.1 to 3.0.0 broke my website #732
  3. Deactivate the plugin and install and activate the 3.0.0 version of the plugin
  4. Go to the ClassifAI settings screens and note the settings that have been migrated over and should see no errors
  5. Manually change the classifai_feature_descriptive_text_generator option in the database. Change the descriptive_text_fields item to be the value 1 instead of an array
  6. Go back to the settings you should now see the same errors as reported in Update from 2.5.1 to 3.0.0 broke my website #732
  7. Check out this branch and refresh, you should no longer see the errors

Changelog Entry

Fixed - Ensure we only pass in array data to our merge_settings method
Fixed - Handle scenario where non-array data is passed into our descriptive text generator feature

Credits

Props @ajaxthemestudios, @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.

@github-actions github-actions bot added this to the 3.0.1 milestone Mar 2, 2024
…aved setting isn't. In those scenarios, not only should we not run the merge_settings method again but let's use our default setting since the saved setting is incorrect
@dkotter dkotter marked this pull request as ready for review March 6, 2024 17:34
@dkotter dkotter requested review from jeffpaul and a team as code owners March 6, 2024 17:34
@github-actions github-actions bot added the needs:code-review This requires code review. label Mar 6, 2024
@jeffpaul jeffpaul requested review from peterwilsoncc and removed request for a team and jeffpaul March 6, 2024 17:34
@dkotter
Copy link
Collaborator Author

dkotter commented Mar 6, 2024

I believe I've finally figured out proper reproduction steps here. The ones outlined in the PR description still work (manually changing data) but I'm pretty sure I've figured out that actual use case.

In #374, we changed how the descriptive text generator feature works. Previously it was either on or off and if on, it would save the text in the alt text field. In that PR we changed this so you can choose which field(s) this text gets saved to.

But this also means the settings we saved changed from being a string to being an array. This PR went out in the v2.0.0 release. So if someone were using ClassifAI prior to that release and they never re-saved the descriptive text generator settings, they would still have a string value saved there instead of an array value. And when updating to v3.0.0 and the changes introduced there (as well as stricter type checks) this ends up causing the problem.

So to reproduce:

  1. Install the 1.8.1 version of ClassifAI (last version prior to v2.0.0)
  2. Set things up to match the screenshot in Update from 2.5.1 to 3.0.0 broke my website #732 but also ensure to turn on the Generate alt text setting
  3. Update to v3.0.0
  4. Notice PHP errors happen
  5. Checkout out this PR and ensure the errors go away

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.

LGTM and tests well.

@peterwilsoncc peterwilsoncc merged commit d42fdee into develop Mar 7, 2024
13 checks passed
@peterwilsoncc peterwilsoncc deleted the fix/732 branch March 7, 2024 00:30
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.

Update from 2.5.1 to 3.0.0 broke my website
2 participants