dev/core#2153 #REF Remove outdated updateCustomValues function

[seamuslee001]

Overview

As per #18912 (comment) This removes the outdated updateCustomValues function and replaces its current usage with updateValue

Before

Outdated function called from a couple of places

After

Outdated function removed

ping @yashodha @demeritcowboy @eileenmcnaughton @colemanw

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[seamuslee001]

Note this is just shifting the updateValue call from L487 to here to match how the code was originally

[demeritcowboy]

I'm thinking that was buggy before and having it down below is maybe better. For example suppose line 472 changes $customOption->value. Then if the update is here you've already updated existing ones to something that is different from what gets saved in civicrm_option_value.

Having it at the bottom as it is right now calls the function even on new fields, but could wrap in if ($customOption->id)

[seamuslee001]

ok have moved it back now @demeritcowboy

[demeritcowboy]

Did you guys discuss the cleanMoney part - updateValue doesn't clean the money - not sure if that really matters anywhere.

[seamuslee001]

@demeritcowboy we felt that cleanMoney is only really needed on the form layer

[demeritcowboy]

1. Calling updateValue when customOption->id is null is a little dangerous since it then thinks you're updating Phone communication preference (the DAO->find() just returns the first option value in the whole list). It's unlikely you have a custom field somewhere using that option group for its choices, but it would go and try to update value 1. The updateValue function should either fail hard if you give it null id, or just return.

2. It doesn't work for type money (clean or dirty). Maybe it never did for either function? It doesn't work if I do it from system settings - option groups either. Not sure why yet.

[demeritcowboy]

For the monies, I see it's because the custom db column is created as decimal(20,2), so if you have an option value of 10, with no decimals, the option value stores it as 10, but the custom table stores it as 10.00, so it doesn't match when it goes to update. So that's a separate issue - I'll make a ticket.

[demeritcowboy]

I think this should be self::updateValue($optionId, $value);

[eileenmcnaughton]

@demeritcowboy is this currently mergeable?

[demeritcowboy]

I don't think so. It doesn't pass the correct id for deletions, and calling it for new option values is dangerous unless updateValue is changed to ignore null ids. But can ignore the money stuff for now.

[eileenmcnaughton]

OK - in your court then @seamuslee001

[seamuslee001]

@demeritcowboy should be sorted now can you check

[demeritcowboy]

Looks good to me.

[yashodha]

@seamuslee001 looks good, thanks!