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

Fix MultiSelect when empty selection is given #6768

Merged
merged 1 commit into from Apr 4, 2024

Conversation

martin-rueegg
Copy link
Contributor

@martin-rueegg martin-rueegg commented Dec 20, 2023

Currently, the BasePicker and the MultiSelect widgets try to get the value from the module, if $config['selection'] is empty (which includes an empty array!).

To avoid such a lookup by the widget, one would expect to provide an empty array (which may actually constitute the current selection). However, this will be ignored due to the above.

This PR does treat an empty array as a valid selection list. Thus, avoiding looking up the value.

Additionally, if the field value is an ArrayAccess instance, rather than an array, the current code fails with a fatal error. Using ArrayHelper::keyExists() rather than in_array() solves this issue.

PR Admin

What kind of change does this PR introduce?

  • Bugfix

Does this PR introduce a breaking change?

  • Maybe Yes
  • Probably No

If yes, please describe the impact and migration path for existing applications:

Not sure if there are use cases, where and empty array has been provided as selection and was expected to be replaced by the attribute value of the model or some other source. To me this would not make sense. Because if you expect the selection to be filled by the field/widget, then why would you provide an empty array, rather then just leaving the config alone?

That's why I currently filed the PR against master.

The PR fulfills these requirements

  • It's submitted to the develop branch, not the master branch if no hotfix
  • All tests tests are passing
  • New/updated tests are included
  • Changelog was modified

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

@luke-
Copy link
Contributor

luke- commented Dec 20, 2023

@martin-rueegg Thanks for the fix.

I would prefer to merge such fixes into the develop branch. As long as it has no immediate impact on productive installations.

Please let me know if you absolutely need it (or if the function is affected in any way), then we can merge it into master branch.

@luke- luke- changed the base branch from master to develop April 4, 2024 14:53
@luke- luke- merged commit dd2d230 into humhub:develop Apr 4, 2024
6 checks passed
@marc-farre
Copy link
Collaborator

@luke- I've added a comment here about an issue introduced by this PR: dd2d230#r141253611

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants