dev/core#144 getCustomFieldID switch to API, add caching, add full string return option

[mattwire]

Overview

In most of my extensions I implement something like this: https://github.com/mattwire/uk.co.mjwconsult.recurmaster/blob/master/CRM/Recurmaster/Utils.php#L14

Because I want the lookup to be cached so I can use it multiple times without a performance issue (eg. within hooks). It is also useful to be able to return the full string equivalent (ie. custom_123 instead of 123) as it can then be used directly in parameter arrays like $key = getCustomFieldID() without doing: $key = 'custom_' . getCustomFieldID()

This is a proposal to update the core function getCustomFieldID so that it is more useful. It should also help towards #109

Before

No caching, requires a database lookup every time it is called.

After

Result cached as static. Performance improvement if called multiple times, can also return result directly as full string for use in parameter arrays.

@reneolivo Could you review this towards https://lab.civicrm.org/dev/core/issues/109

[eileenmcnaughton]

test this please

[eileenmcnaughton]

test this please

[eileenmcnaughton]

I agree we should improve the caching in this function. I'm uncomfortable with the suggestion it is done so that extensions can use it (if it's for extension then perhaps something in Civix makes more sense). The issue with extensions is that they should not feel like there is any support, implied or otherwise, for calling non-api functions & that we may change those functions again.

(On that ticket I think the suggestion of using apiv4 was used which is a good idea)

[mattwire]

I'm uncomfortable with the suggestion it is done so that extensions can use it

@eileenmcnaughton I agree, and I wouldn't expect extensions to use it directly, but I was previously unaware of that function existing at all and since I'd already implemented a version in my extensions that did caching I thought I'd propose it here for core. A follow-up to this would be to look at accessing this function via the customvalue API and adding a unit test for that.

[eileenmcnaughton]

Ok @mattwire - that makes sense - thanks!

[eileenmcnaughton]

Hi @mattwire I added a unit test to ensure this doesn't change behaviour and it failed - because it DOES change default behaviour

#12283

[eileenmcnaughton]

test this please

[mattwire]

The default for $fullString should be FALSE - tests should now pass

[eileenmcnaughton]

Thanks @mattwire - makes sense & we have test coverage of this. Merging

[eileenmcnaughton]

I wonder how many notices we would hit if we made it so it only checks label if name fails & gives a deprecated warning if it has to use the fall back...