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

Guard against passing empty values to CRM_Core_DAO::getFieldValue() #21998

Merged
merged 1 commit into from
Nov 7, 2021

Conversation

webmaster-cses-org-uk
Copy link
Contributor

Overview

This PR fixes a minor regression introduced in #18941 whereby empty values could be passed to CRM_Core_DAO::getFieldValue() which would throw an exception.

This change removes a potential "getFieldValue failed" error that can be thrown if an empty scalar value occurs in a multiple-contact reference custom field.

See https://lab.civicrm.org/dev/core/-/issues/2939

Replaces eileenmcnaughton#13 which was opened against the wrong repo.

Before

When submitting a record that includes a custom field of multiple contact reference type, if that field is empty (i.e. no contacts), an error message "getFieldValue failed" sometimes occurs and the screen freezes and needs to be reloaded. This would be due to the specific data present in the field (zero vs null vs empty array etc).

After

Defensive coding prevents empty values being passed to CRM_Core_DAO::getFieldValue(), so no exception occurs and record update proceeds as expected without any error.

Comments

Demonstrated working on live CiviCRM installation 5.37.0.

This change removes a potential "getFieldValue failed" error that can be thrown if an empty scalar value occurs in a multiple-contact reference custom field.

See https://lab.civicrm.org/dev/core/-/issues/2939

Replaces https://github.com/eileenmcnaughton/civicrm-core/pull/13/commits which was opened against the wrong repo.
@civibot
Copy link

civibot bot commented Nov 7, 2021

(Standard links)

@colemanw
Copy link
Member

colemanw commented Nov 7, 2021

This change makes sense to me. If tests pass then it's safe to merge.

@monishdeb monishdeb merged commit 455f605 into civicrm:master Nov 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants