CRM-21466 - Fix (obscure) enotice when updating greeting for contact,… #11310

eileenmcnaughton · 2017-11-22T04:20:17Z

Overview

If I try to update the greeting template on an existing contact I get an e-notice when trying to call

$contact = $this->callAPISuccess('Contact', 'create', array(
{{  'id' => $contact['id'], }}
 'postal_greeting_id' => 2,
));

Before

e-notice

After

resolved. Tests added and caching issues that are blockers to tests fixed

Technical Details

The issue occurs when calling
{{CRM_Core_PseudoConstant::greeting($filter); }}
with a filter for contact_type.

Contact type is unknown at this point. Further passing it in makes the code LESS efficient rather than MORE efficient. The code loads possible values to a static index. If it returns too many options no harm is done because the calling code is only looking for the one option for which it has an ID. If, however, it does filter it then it means it stores one static var for household, one for individual & one for organisation, resulting in marginally more queries running. (I think the performance is marginal in both scenarios but just trying to explore what reason, if any, there is for the filter.)

eileenmcnaughton · 2017-11-22T23:14:05Z

CRM/Core/BAO/OptionValue.php

@@ -234,7 +234,7 @@ public static function add(&$params, $ids = array()) {

    $optionValue->id = CRM_Utils_Array::value('optionValue', $ids);
    $optionValue->save();
-    CRM_Core_PseudoConstant::flush();
+    CRM_Core_OptionGroup::flushAll();


Calling CRM_Core_PseudoConstant::flush() with no params simply results in CRM_Core_OptionGroup::flushAll() being called -so this is for clarity

No, that's not all it does. It also clears the CRM_Core_PseudoConstant::$cache variable.

It looks like that is not a real property though. I can't see anywhere that self::cache is set - it seems to me that either you call the function with the implicit param 'cache' and it does nothing in the first bit because CRM_Core_PseudoConstant::cache does not exist AND it does an OptionGroup flush in the second part. Or you pass in a parameter to flush & it clears that and not the optionGroup

/** * Flush given pseudoconstant so it can be reread from db. * nex time it's requested. * * * @param bool|string $name pseudoconstant to be flushed */ public static function flush($name = 'cache') { if (isset(self::$$name)) { self::$$name = NULL; } if ($name == 'cache') { CRM_Core_OptionGroup::flushAll(); } }

(It's called from 3 places outside unit tests - including this one)

One of those 3 is CRM_Utils_PseudoConstant::flushAll(); - which DOES flush all properties that are set. The last is OptionValue::delete

The variable is used by CRM_Core_PseudoConstant::get() so almost all pseudoconstant lookups rely on it.

OK - I can see it now - I'll flip that back

eileenmcnaughton · 2017-11-29T21:09:27Z

api/v3/Contact.php

@@ -618,7 +618,6 @@ function _civicrm_api3_greeting_format_params($params) {

    $nullValue = FALSE;
    $filter = array(
-      'contact_type' => $params['contact_type'],


enotice fix when not set + this is not really the place to worry about filtering

eileenmcnaughton · 2017-11-29T21:09:50Z

CRM/Utils/Token.php

-    }
+    // Refresh contact tokens in case they have changed. There is heavy caching
+    // in exportable fields so there is no benefit in doing this conditionally.
+    self::$_tokens['contact'] = array_merge(


ditch static var which makes tests hard & does not acheive much

twomice · 2017-12-02T20:33:12Z

CRM/Core/PseudoConstant.php

@@ -1650,6 +1650,10 @@ public static function countryIDForStateID($stateID) {
   *   array reference of all greetings.
   */
  public static function greeting($filter, $columnName = 'label') {
+    if (!isset(Civi::$statics[__CLASS__]['greeting'])) {


All the changes in CRM/Core/PseudoConstant.php are already merged, apparently in PR #11313. I think that explains the "merge conflict" error in the Jenkins checks.

This is the only thing that stands out to me, @eileenmcnaughton . Other changes look sensible, I can't think of any way to improve them.

But oddly, I'm not seeing the E_NOTICE you mention, using code like you provided, on master:

public function testNotice() { // This will cause a failing test because of the NOTICE-level error: // trigger_error('Notice.', E_USER_NOTICE); // But with that line commented out, the test passes just fine. $contact_id = $this->individualCreate(); $contact = $this->callAPISuccess('Contact', 'create', array( 'contact_type' => 'Individual', 'id' => $contact_id, 'postal_greeting_id' => 2, )); }

… add test

eileenmcnaughton · 2017-12-02T21:05:15Z

I've rebased it - I actually have a few more related tidy ups on this code locally - which I will put up once this is merged - I think they tie in quite well with your goals

colemanw · 2017-12-03T15:31:12Z

CRM/Core/BAO/OptionValue.php

@@ -234,7 +234,7 @@ public static function add(&$params, $ids = array()) {

    $optionValue->id = CRM_Utils_Array::value('optionValue', $ids);
    $optionValue->save();
-    CRM_Core_PseudoConstant::flush();
+    CRM_Core_OptionGroup::flushAll();


No, that's not all it does. It also clears the CRM_Core_PseudoConstant::$cache variable.

colemanw · 2017-12-03T15:32:58Z

CRM/Core/OptionGroup.php

+    if (isset(Civi::$statics['CRM_Core_PseudoConstant']['greeting'])) {
+      unset(Civi::$statics['CRM_Core_PseudoConstant']['greeting']);
+    }
+  }


This is a strange oddball. Why is this cache variable being stored in Civi::$statics instead of in CRM_Core_PseudoConstant::$cache? If it was in the latter then no special function would be needed and CRM_Core_PseudoConstant::flush() would take care of it.

We've been consistently moving towards storing statics in Civi::Statics - I can go a different way if you want - but there are a lot of places where that has been done. Note that CiviStatics IS flushed between test runs - but there is sometimes a need to flush a value once it has been altered. We don't normally flush all PseudoConstants in order to achieve that since there is still a performance benefit in keeping most of our cached variables.

I still think putting it in CRM_Core_PseudoConstant::$cache would be better because we then wouldn't need this function. If we followed the pattern you're establishing here of a clear function for every variable, this file would get quite long & unwieldy.

I removed this function because I added in to unset Civi::statics[CLASS] in the CRM_Core_PseudoConstant function. There are already a few functions in that class using the Civi::statics.

…ulated. This involves fixing the caching to be flushed during testing

colemanw · 2017-12-04T02:51:00Z

Cool. We should probably also move CRM_Core_PseudoConstant::$cache over to Civi::$statics. Not necessarily in this PR.

eileenmcnaughton · 2017-12-04T02:54:05Z

yep - that feels like what we are working towards. Is this merge-on-pass now?

CRM-21466 - Fix (obscure) enotice when updating greeting for contact,…

2 commits from civicrm#11310 CRM-21466 - Fix (obscure) enotice when updating greeting for contact, add test CRM-21466 follow up, add unit test to ensure custom fields can be populated. This involves fixing the caching to be flushed during testing Change-Id: Ic296f2b37a8ea169b93df314b95d2502d2e2b3f1

eileenmcnaughton force-pushed the greet branch from bd823c2 to fcff378 Compare November 22, 2017 04:41

eileenmcnaughton commented Nov 22, 2017

View reviewed changes

eileenmcnaughton force-pushed the greet branch 2 times, most recently from 04f2d35 to e5a7272 Compare November 23, 2017 01:31

eileenmcnaughton commented Nov 29, 2017

View reviewed changes

eileenmcnaughton mentioned this pull request Dec 2, 2017

WIP: CRM-21180: Inline changes to custom fields aren't reflected in custom greetings #11095

Closed

twomice reviewed Dec 2, 2017

View reviewed changes

CRM-21466 - Fix (obscure) enotice when updating greeting for contact,…

0db8cd8

… add test

eileenmcnaughton force-pushed the greet branch from e5a7272 to b7ee47d Compare December 2, 2017 21:03

colemanw requested changes Dec 3, 2017

View reviewed changes

eileenmcnaughton force-pushed the greet branch from b7ee47d to e876924 Compare December 4, 2017 02:37

CRM-21466 follow up, add unit test to ensure custom fields can be pop…

7c99061

…ulated. This involves fixing the caching to be flushed during testing

eileenmcnaughton force-pushed the greet branch from e876924 to 7c99061 Compare December 4, 2017 02:47

colemanw approved these changes Dec 4, 2017

View reviewed changes

colemanw added the merge on pass label Dec 4, 2017

eileenmcnaughton merged commit fa8ef27 into civicrm:master Dec 4, 2017

eileenmcnaughton deleted the greet branch December 4, 2017 04:00

sluc23 pushed a commit to ixiam/civicrm-core that referenced this pull request Jan 10, 2018

Merge pull request civicrm#11310 from eileenmcnaughton/greet

6da7d04

CRM-21466 - Fix (obscure) enotice when updating greeting for contact,…

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM-21466 - Fix (obscure) enotice when updating greeting for contact,… #11310

CRM-21466 - Fix (obscure) enotice when updating greeting for contact,… #11310

eileenmcnaughton commented Nov 22, 2017 •

edited

Loading

eileenmcnaughton Nov 22, 2017

colemanw Dec 3, 2017

eileenmcnaughton Dec 4, 2017

eileenmcnaughton Dec 4, 2017

colemanw Dec 4, 2017

eileenmcnaughton Dec 4, 2017

eileenmcnaughton Nov 29, 2017

eileenmcnaughton Nov 29, 2017

colemanw Dec 3, 2017

twomice Dec 2, 2017

twomice Dec 2, 2017

eileenmcnaughton commented Dec 2, 2017

colemanw Dec 3, 2017

colemanw Dec 3, 2017

eileenmcnaughton Dec 4, 2017

colemanw Dec 4, 2017

eileenmcnaughton Dec 4, 2017

colemanw commented Dec 4, 2017

eileenmcnaughton commented Dec 4, 2017

CRM-21466 - Fix (obscure) enotice when updating greeting for contact,… #11310

CRM-21466 - Fix (obscure) enotice when updating greeting for contact,… #11310

Conversation

eileenmcnaughton commented Nov 22, 2017 • edited Loading

Overview

Before

After

Technical Details

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Dec 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colemanw commented Dec 4, 2017

eileenmcnaughton commented Dec 4, 2017

eileenmcnaughton commented Nov 22, 2017 •

edited

Loading