APIv4 - Setting api misc fixes & tests #20191
Conversation
|
(Standard links)
|
colemanw
force-pushed
the
apiSettingsFixes
branch
2 times, most recently
from
339ebc1 to
3dbebc6
Compare
| @@ -437,7 +437,7 @@ public static function geoProvider() { | |||
| * | |||
| * @throws \CRM_Core_Exception | |||
| */ | |||
| public function taxDisplayOptions() { | |||
| public static function taxDisplayOptions() { | |||
There was a problem hiding this comment.
@eileenmcnaughton the new unit test which checks settings pseudoconstants hit this (PHP error non-static method called statically).
| public function testGetFieldsGroupFilters($version) { | ||
| $this->_apiversion = $version; | ||
| public function testGetFieldsGroupFilters() { | ||
| $this->_apiversion = 3; |
There was a problem hiding this comment.
group is a deprecated property so APIv4 doesn't need to test it.
There was a problem hiding this comment.
Does apiv4 still permit filtering on it if set. We kinda deprecated it as a required key but kept the idea you could filter by it (e.g if you wanted to present all the settings defined by one extension on a page). I can't recall if it was reliable or not
There was a problem hiding this comment.
No. I was following the docs which state it is a deprecated property. I didn't want APIv4 returning stuff that's already deprecated. That means it wouldn't be able to filter on it either.
| @@ -69,9 +69,7 @@ public static function getMetadata($filters = [], $domainID = NULL, $loadOptions | |||
| } | |||
|
|
|||
| self::_filterSettingsSpecification($filters, $settingsMetadata); | |||
| if ($loadOptions) { | |||
There was a problem hiding this comment.
@colemanw so this seems to result in us iterating through every setting and doing noting if loadOptions is false - as opposed to just skipping. Why the change?
There was a problem hiding this comment.
Also - I note loadOptions is not cached so that doesn't mitigate the empty iteration
There was a problem hiding this comment.
You're right. I originally had this doing more but had to change it back due to a failing test.
There was a problem hiding this comment.
@colemanw is it going to do more again soon? If so I guess it's Ok to merge as is
There was a problem hiding this comment.
Probably not. I've replaced the conditional as before.
Civi/Core/SettingsMetadata.php
Outdated
| */ | ||
| protected static function loadOptions(&$settingSpec) { | ||
| protected static function fillOptions(&$settingSpec, $getOptions) { |
There was a problem hiding this comment.
So the thing I find kinda messy is that getOptions is really a 3 way option it
- NO
- YES
- do it as an array
I feel like is_array() to determine that last one is a bit opaque
There was a problem hiding this comment.
Well, yea but that's way outside the scope of this PR IMO.
There was a problem hiding this comment.
It's really an APIv4 documentation issue.
There was a problem hiding this comment.
@colemanw well maybe at the api level - but at the level of this function we could pass something pretty clear
There was a problem hiding this comment.
Unfortunately, I don't think we can. To maintain backward-compat it needs to be able to return a flat array if true and in order to correctly serve data to APIv4 it needs to be able to fetch name, label, etc if array.
There was a problem hiding this comment.
Sure I'll rename that param too.
There was a problem hiding this comment.
I renamed it but resisted the temptation to turn it into a boolean because eventually we'll add support for more return values.
There was a problem hiding this comment.
Hmm I feel like Civi is full of that sort of good intention code which makes less & less sense after the extra thing isn't done months later. I'd probably go with what actually makes sense with the code now as it's a protected function - but failing that beef up the comments explaining the rationale for the param in the docblock
There was a problem hiding this comment.
@eileenmcnaughton I renamed the variable & added a comment. I don't think this is one of those cases because it's not broken or half-functional. It works, this just leaves the door open for further enhancements.
There was a problem hiding this comment.
OK - it still feels a bit too much like magic to me but at least the docs cover it now
Fixes the setting api to correctly handle pseudoconstants & field suffixes
Overview
Fixes the setting api to correctly handle pseudoconstants & field suffixes
Before
Setting pseudoconstants not handled well.
After
Works better & covered by tests.