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

CRM-20452: Fatal Error on saving Organisation Info when geocoding is … #10192

Merged
merged 1 commit into from
May 3, 2017

Conversation

jitendrapurohit
Copy link
Contributor

@jitendrapurohit jitendrapurohit commented Apr 20, 2017

@@ -82,7 +82,7 @@ public static function format(&$values, $stateName = FALSE) {
$add .= ',+';
}

if (!empty($values['state_province']) || !empty($values['state_province_id'])) {
if (!empty($values['state_province']) || (!empty($values['state_province_id']) && $values['state_province_id'] != 'null')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jitendrapurohit do you know we have a special function to handle the weirdness that is CiviCRM thinking the four letters "null" actually is a special form of NULL?

See CRM_Utils_System::isEmpty()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I ran into this, I would want to know why state_province_id='null' before merging this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is explained in the Ticket I think it is when you deselect the state from an address

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. IMO that's incorrect behaviour of the form then - it shouldn't be sending 'null' as a string, surely? Weirdness! The state province ID should conform to an integer or literal null, not the string ... unless there's a good reason to tolerate the other. /opinion

Copy link
Contributor Author

@jitendrapurohit jitendrapurohit Apr 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reason for not using isNull - (we don't have isEmpty() :-(), is what I thought to check before, but that function only works if the key is actually present in the array and throws notices when it is not. Eg:

$var = array('key' => 'null');
if (!CRM_Utils_System::isNull($var['key'])) {
  CRM_Core_Error::Debug('Not Null', $var);
}

checks correctly and all seems to be working fine. Now, if we don't get this key:

$var = array();
if (!CRM_Utils_System::isNull($var['key'])) {
  CRM_Core_Error::Debug('Not Null', $var);
}

will throw a notice Notice: Undefined index: key since the key is not present and check of isset() is done after param is entered into isNull() function.

'null' value is being set at this point https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/BAO/Address.php#L319 to fix CRM-3393. Also, it is a very minor change needed, hence proceeded with a simple check other than manipulating 'null' to an empty string and check for all remaining scenarios.

@monishdeb
Copy link
Member

Replicated the issue, as per the steps mentioned in the issue description. The patch fixes the issue for Google and Yahoo gecode provider.

@monishdeb monishdeb merged commit 294ea01 into civicrm:master May 3, 2017
@jitendrapurohit jitendrapurohit deleted the CRM-20452 branch May 30, 2017 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants