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 saving of State/Province Multi-select values #17737

Merged
merged 1 commit into from
Jul 2, 2020

Conversation

christianwach
Copy link
Member

Overview

Fixes this issue on Lab.

A Custom Field of type State/Province Multi-select fails to save values when attached to an Event and "Is this Field Searchable?" is set to false. Reproduced on WPMaster and DMaster demo sites for a CiviEvent. I haven't checked if this is exclusive to Events, but can see the solution and will open a PR in due course.

Reproduction steps

  1. Create a set of Custom Fields and attach to to "Event"
  2. Add a field of data type "State/Province" and field type "Select State/Province"
  3. Enable "Multi-Select" and disable "Is this Field Searchable?"
  4. Visit a "Configure Event" page and select some "State/Province" values
  5. Submit the form to save the event - State/Province" values are saved
  6. Submit the form again to save the event - State/Province" values are lost

Current behaviour

Following the above procedure (line refs are for 5.26.2) throws the following warnings:

[02-Jul-2020 12:59:29 Europe/London] PHP Warning:  explode() expects parameter 2 to be string, array given in /path/to/CRM/Core/BAO/CustomGroup.php on line 1383
[02-Jul-2020 12:59:29 Europe/London] PHP Warning:  Invalid argument supplied for foreach() in /path/to/CRM/Core/BAO/CustomGroup.php on line 1384

The WPMaster and DMaster logs will show similar entries.

Expected behaviour

The incoming data should be parsed in the same way that this commit implements. Doing so fixes the problem.

@civibot
Copy link

civibot bot commented Jul 2, 2020

(Standard links)

@civibot civibot bot added the master label Jul 2, 2020
@colemanw
Copy link
Member

colemanw commented Jul 2, 2020

Thanks for the fix.
This function is so overcomplicated and confusing already, I hate to add more bulk to it. Can you at least add some code comments about what's going on and why?

@christianwach
Copy link
Member Author

Added a couple of one-liners... not sure this PR is the right place to fully document.

@colemanw
Copy link
Member

colemanw commented Jul 2, 2020

Thanks. Looks good now.

@colemanw colemanw merged commit b0b075b into civicrm:master Jul 2, 2020
@christianwach christianwach deleted the lab-core-1851 branch July 18, 2020 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants