CRM-20381 - ensure option to NOT geocode on imports is respected. #10181
Conversation
|
@totten one for you |
|
@totten - hey tim - just one more ping to get your opinion on this pull request. |
|
jenkins, test this please |
|
|
||
| // If foreign name is set, use that name (except with callback types because | ||
| // their second parameter is the object, not the foreign name). | ||
| $name = isset($this->map[$k][1]) && $type != 'callback' ? $this->map[$k][1] : $k; |
There was a problem hiding this comment.
Thanks for the nudge, @jmcclelland. This makes sense.
CRM/Core/Config/MagicMerge.php
Outdated
| @@ -222,7 +222,7 @@ public function __get($k) { | |||
| if (!isset($this->map[$k])) { | |||
| throw new \CRM_Core_Exception("Cannot read unrecognized property CRM_Core_Config::\${$k}."); | |||
| } | |||
| if (isset($this->cache[$k])) { | |||
| if (array_key_exists($k, $this->cache)) { | |||
There was a problem hiding this comment.
The main question would be: do we have situations where $this->cache[$k] uses a NULL to mean "temporary cache miss"? This is hard to determine. (Just as hard as determining whether any keys have semantically meaningful NULL values.)
Re-reading the code for __get(), one branch pops out:
case 'boot-svc':
$this->cache[$k] = \Civi\Core\Container::getBootService($name);
return $this->cache[$k];The getBootService($name) could theoretically return a temporary NULL if it was called during the middle of bootstrap. OTOH, doing so would probably produce warnings. And the boot-services are really central -- I'd expect it fail prominently if this change were problematic. The fact that tests are passing is a good sign.
Still, the conservative in me would prefer one of these changes:
- Include a unit-test for setting/getting
geocodeMethod. (If there is a regression where some other thing expects NULL, the test would ensure thatgeocodeMethodworks as expected.) - Keep the old handling of
isset($this->cache[$k]). Instead, find the code which says$config->geocodeMethod = NULL. Change it to use empty-string (''). (If you readCRM_Utils_Geocode::getProviderClass(), it seems to be saying that the correct form of emptiness is''.)
There was a problem hiding this comment.
Thanks @totten . I guess I never quite realized that isset returns true with an empty string and with FALSE, yet it returns false with NULL. I'll give it a shot. I did a quick scan and found that there are several tests in the code that use if (!empty($config->geocodeMethod)) which should work just as well with NULL or '' - but nothing that seems to expect NULL.
'' means empty, NULL means not set, which confuses MagicMerge.
|
Nice updates! Thank you, @jmcclelland . |
A somewhat terrifying change - we'll see how the tests do...