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 empty plugin settings are saved correctly #22885

Merged
merged 2 commits into from
Dec 19, 2024

Conversation

michalkleiner
Copy link
Contributor

Description:

When an array setting was empty, it didn't get sent to the API for processing. We are using a custom empty value to signal that and the API to handle that here.

For Matomo 6 we may consider using json_encoded array instead though that would be a breaking change for now.

You can check that the integration test fails without the JS and PHP changes, and the original issue can also be reproduced by trying to unselect and save all the browsers in the ExampleSettingsPlugin.

Fixes #22877
Ref. DEV-18796

Review

@michalkleiner michalkleiner requested a review from a team December 19, 2024 15:11
@michalkleiner michalkleiner added c: Usability For issues that let users achieve a defined goal more effectively or efficiently. c: Design / UI For issues that impact Matomo's user interface or the design overall. c: APIs For bugs and features in the Matomo HTTP and plugin APIs. javascript PRs that update Javascript code Needs Review PRs that need a code review labels Dec 19, 2024
@michalkleiner michalkleiner added this to the 5.2.1 milestone Dec 19, 2024
@michalkleiner
Copy link
Contributor Author

This should actually target 5.2.x-dev. When it's available I can rebase that.

@michalkleiner michalkleiner force-pushed the dev-18796-plugin-settings-empty-array branch from 3d7c0c6 to 6614fd0 Compare December 19, 2024 15:14
@michalkleiner michalkleiner changed the base branch from 5.x-dev to 5.2.x-dev December 19, 2024 15:14
@michalkleiner
Copy link
Contributor Author

I probably confused GH by the force-push and target branch change, so triggered the tests manually here — https://github.com/matomo-org/matomo/actions/runs/12415416589

@michalkleiner michalkleiner force-pushed the dev-18796-plugin-settings-empty-array branch from 6614fd0 to 248b11c Compare December 19, 2024 20:02
@michalkleiner
Copy link
Contributor Author

Force-pushed to re-trigger the tests.

Copy link
Contributor

@caddoo caddoo left a comment

Choose a reason for hiding this comment

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

Tested and works ✅

@caddoo caddoo merged commit c51668c into 5.2.x-dev Dec 19, 2024
24 of 26 checks passed
@caddoo caddoo deleted the dev-18796-plugin-settings-empty-array branch December 19, 2024 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: APIs For bugs and features in the Matomo HTTP and plugin APIs. c: Design / UI For issues that impact Matomo's user interface or the design overall. c: Usability For issues that let users achieve a defined goal more effectively or efficiently. javascript PRs that update Javascript code Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] SystemSetting not saving the settings for FieldConfig::TYPE_ARRAY if all options are unselected
2 participants