Settings #9420

eileenmcnaughton · 2016-11-21T02:56:46Z

No description provided.

totten · 2017-02-06T19:23:12Z

@eileenmcnaughton jfyi, this is crashing here:

ok 1459 - api_v3_SettingTest::testGetSetting
ok 1460 - api_v3_SettingTest::testGetExtensionSetting
PHP Fatal error:  Call to undefined method CRM_Core_BAO_Domain::setDomain() in /home/jenkins/buildkit/build/core-9420-5arlm/sites/all/modules/civicrm/tests/phpunit/api/v3/SettingTest.php on line 381

A bit of good news, though -- it appears that nothing else in the known universe references those functions:

me@localhost:~/buildkit/build/universe$ svngrep ::setDomain
civicrm-core/CRM/Core/BAO/Domain.php:   * counterpart to CRM_Core_BAO_Domain::setDomain.
civicrm-core/CRM/Core/BAO/Domain.php:   * @see CRM_Core_BAO_Domain::setDomain
civicrm-core/CRM/Core/BAO/Setting.php:        CRM_Core_BAO_Domain::setDomain($domainID);
civicrm-core/tests/phpunit/api/v3/SettingTest.php:    CRM_Core_BAO_Domain::setDomain($this->_domainID2);
me@localhost:~/buildkit/build/universe$ svngrep ::resetDomain
civicrm-core/CRM/Core/BAO/Setting.php:      CRM_Core_BAO_Domain::resetDomain();
civicrm-core/tests/phpunit/api/v3/SettingTest.php:    CRM_Core_BAO_Domain::resetDomain();

totten · 2017-02-06T19:30:21Z

CRM/Core/BAO/Domain.php

@@ -69,7 +69,7 @@ public static function retrieve(&$params, &$defaults) {
   *
   * @return CRM_Core_BAO_Domain|null
   */
-  public static function &getDomain($reset = NULL) {
+  public static function getDomain($reset = NULL) {


Saying this out loud to ensure we agree: changing this function signature is safe because:

There shouldn't be any subclasses (let alone folks overriding the function), and

The function returns an object which means that the & should have negligible impact behavior (for better or worse or mystery, both signatures allow the caller to mutate the domain object)

Yes & a third point is that I have learnt those & were ADDED to support php4

eileenmcnaughton · 2017-02-06T19:42:37Z

Ive just pushed a commit to remove that test

totten · 2017-02-06T19:45:01Z

CRM/Core/Error.php

@@ -600,7 +600,15 @@ public static function debug_log_message($message, $out = FALSE, $comp = '', $pr
    }
    $file_log->close();

-    if (!empty($config->userFrameworkLogging)) {
+    if (!isset(\Civi::$statics[__CLASS__]) || !isset(\Civi::$statics[__CLASS__]['userFrameworkLogging'])) {


(a) In my experience/environment, the first isset() hasn't been necessary. If it is necessary, then we probably need to cleanup a lot of others. (Also: curious to know what environment you have where it's necessary.)

(b) If it is necessary to explicitly check both, then you can probably combine the two isset()s:

if (!isset(\Civi::$statics[__CLASS__], \Civi::$statics[__CLASS__]['userFrameworkLogging']))

totten · 2017-02-06T20:38:16Z

Civi.php

@@ -95,6 +95,7 @@ public static function service($id) {
  public static function reset() {
    self::$statics = array();
    Civi\Core\Container::singleton();
+    \Civi::service('settings_manager')->useDefaults();


Agree that putting useDefaults() in boot() would have unintended consequences. IIRC, during dev, I'd originally applied defaults during boot() but had to move it. (Maybe to support edge cases around using hook_civicrm_config to manipulate $civicrm_setting data?)

FWIW, in my mind, there are two different notions of "reset":

Thin reset: Tear down running service objects... basically just flush statics. This was a new function, Civi::reset(). It leaves the system in an unbootstrapped state. (FWIW, in a design like "Symfony Standard Edition", that's a pretty much all you need to do. Much of the initialization is sequenced automatically/implicitly/on-demand when you request things from the container.)

Full reset: Tear down and restart the services. This was CRM_Core_Config::singleton(1,1). It leaves the system in a bootstrapped state. This was the preexisting functionality used by CiviUnitTestCase (and takes a lot of care to preserve sequence of steps).

FWIW, the current CiviUnitTestCase says this:

\Civi::reset(); $config = CRM_Core_Config::singleton(TRUE, TRUE); // ugh, performance

My gut says this patch is more intellectually consistent:

diff --git a/Civi/Test/CiviTestListener.php b/Civi/Test/CiviTestListener.php index 3bd6f0d..c87f60f 100644 --- a/Civi/Test/CiviTestListener.php +++ b/Civi/Test/CiviTestListener.php @@ -98,6 +98,7 @@ class CiviTestListener extends \PHPUnit_Framework_BaseTestListener { $config = \CRM_Core_Config::singleton(TRUE, TRUE); // ugh, performance \CRM_Utils_System::flushCache(); \Civi::reset(); + \CRM_Core_Config::singleton(TRUE, TRUE); \CRM_Core_Session::singleton()->set('userID', NULL); if (property_exists($config->userPermissionClass, 'permissions')) {

although (either way) we probably want to spot-check a couple other extensions to see that they still run tests correctly.

I find the fact that
\CRM_Core_Config::singleton(TRUE, TRUE);
instantiates the system desparately confusing!

does it need to be called twice above?

Agree it's really confusing. IMHO, the $force idiom is at least consistent with other stuff, but $loadFromDB==FALSE is really weird. (If I had to guess, it probably exists because some common utility function like ts() or escapeString() turns batty without CRM_Core_Config.)

When refactoring before, I figured we needed to retain it on general principle, but now I'm not so sure: grepping universe (rgrep CRM_Core_Config::singleton | grep -v 'singleton()'), there are only a handful of cases which use $loadFromDB==FALSE. Feel like there's 2/3 chance we could kill that mode with 1-2 days work.

Hmm, regarding CiviTestListener, it's interesting that CRM_Core_Config::singleton(TRUE,TRUE) is called 3 lines higher...

eileenmcnaughton · 2017-02-08T03:40:51Z

@totten I tried just moving that line in the same file & it worked for my test in the extended reports extension.

I tried running the mosaico & civivolunteer tests. Both failed with or without the patch - mosaico

Fatal error: Class 'Civi\FlexMailer\Listener\BaseListener' not found in /Users/emcnaughton/buildkit/build/dmaster/sites/default/files/civicrm/ext/uk.co.vedaconsulting.mosaico/CRM/Mosaico/UrlFilter.php on line 12

civivolunteer
Fatal error: require_once(): Failed opening required 'VolunteerTestAbstract.php'
with some hacking I got it to another error about a missing function.

I'm trying to run the mosaico one properly but I did get several tests in so it wasn't a complete miss.

I'm inclined to say we should merge this & add extended reports to CI - this will only affect tests, not any live code & by locking this much in then any fall out (unlikely) can be a step up the ladder

…ng once fix code style per Tim

CRM-19672 remove test that calls removed functions. The point of the test (& the function) was to cope with the settings stored in which no longer exist

I used buildkit to generate & run the extension test. The issue was that the settings were being loaded but later the \Civi::reset(); function was being called - From the testListener I think and it was flushing the settings bag so it no longer held default variables I did look at putting this in the boot function instead and only rejected that for fear of unintended consequences.

A recent commit for CRM-19672 changed the signature for `getDomain()`. It now returns a regular object (not a reference), but this means that you cannot pass the domain directly through to `replaceDomainTokens()`. This manifested as a test error when running `flexmailer` on my local PHP 5.6/7.0 system.

totten · 2017-03-13T19:29:46Z

@eileenmcnaughton The regression in running the flexmailer test turned out to be pretty much what the error reported -- the change to getDomain() in c9dbc47 meant that it no longer returned a reference, but it was passed directly into replaceDomainTokens() which expected a reference. I've added f2d1209 to your branch as a fix.

Based on grepping the known universe, I can't find anything else which is likely to be affected:

Grepping for ::getDomain( suggests all other references are assigned to an intermediate variable.
Grepping for ::replaceDomainTokens( suggests all other references consume an intermediate variable.

I'm not certain why this issue manifested in the test for flexmailer but not civicrm-core. The flexmailer tests are literally the core tests plus some setUp(). A couple theories:

It could be an environment difference (e.g. Jenkins@PHP5.3 and local@PHP5.6/7.0), except that my local@PHP5.6/7.0 showed the discrepancy: civicrm-core's test passed while flexmailer's test failed.
It could be that the extra setUp() process in flexmailer changes the result somehow? Maybe getDomain() returns different values (NULL vs non-NULL) depending on the setup logic?

eileenmcnaughton · 2017-03-13T21:27:39Z

It's passing with your fix & I checked for CRM_Core_BAO_Domain::getDomain & it is not passed into a function anywhere else - so I think this is good now?

eileenmcnaughton · 2017-03-13T21:31:01Z

As an aside, I think it's a bit silly that domain is passed into that function at all, since every calling function just loads it in preparation for that function

totten · 2017-03-14T05:09:13Z

Yeah, I had the same reaction about its silliness.

Anyway, no point in procrastinating more on a merge!

eileenmcnaughton · 2017-03-14T05:11:05Z

yay

agh1 · 2017-04-04T15:59:14Z

This is another PR I can't document. Stuff is removed, and it has to do with settings, and it sounds like a good thing? Could anyone take a stab at a sentence or two of explanation on this?

totten · 2017-04-04T18:36:51Z

From my recollection of the Mattermost conversations, the general impression was this: #9420 improves an edge-case observed when unit-testing Extended Reports, and it removes some obsolete functions from the Domain BAO.

The commits reference a couple JIRA issues. However, and I can't draw a straight-line between that recollection and the JIRA issues.

agh1 · 2017-04-04T20:25:59Z

Thanks @totten this helps. Those issues explain the changes to CRM/Core/Error.php that I saw, and I'm glad to hear that the other stuff really was just cleanup.

eileenmcnaughton · 2017-04-04T21:25:31Z

CRM-19672 has been recently reported by others, on MM - it affects Wordpress users in some edge cases having the page not load when the browser returns from a payment processor

eileenmcnaughton · 2017-04-04T21:26:16Z

The other one prevents an even-more edge-case code loop

Settings

civicrm-builder added the master label Nov 21, 2016

eileenmcnaughton force-pushed the settings branch 2 times, most recently from 5b489a8 to b3ed0aa Compare February 6, 2017 00:19

totten reviewed Feb 6, 2017

View reviewed changes

eileenmcnaughton force-pushed the settings branch 2 times, most recently from 9b501a5 to 70f6a7c Compare February 8, 2017 03:18

eileenmcnaughton force-pushed the settings branch 2 times, most recently from dbe9f29 to f8d9eed Compare February 28, 2017 07:30

CRM-19670 Avoid loop when logging by only attempting to look up setti…

21ce462

…ng once fix code style per Tim

eileenmcnaughton force-pushed the settings branch from f8d9eed to f1a0f01 Compare March 5, 2017 22:17

eileenmcnaughton and others added 2 commits March 6, 2017 11:20

CRM-19672 - fix Settings code wierdness in some circumstances

c9dbc47

CRM-19672 remove test that calls removed functions. The point of the test (& the function) was to cope with the settings stored in which no longer exist

eileenmcnaughton force-pushed the settings branch from f1a0f01 to 33fefd5 Compare March 5, 2017 22:20

totten merged commit 4d931c5 into civicrm:master Mar 14, 2017

eileenmcnaughton deleted the settings branch March 14, 2017 05:11

monishdeb pushed a commit to monishdeb/civicrm-core that referenced this pull request May 2, 2017

Merge pull request civicrm#9420 from eileenmcnaughton/settings

1307b65

Settings

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Settings #9420

Settings #9420

eileenmcnaughton commented Nov 21, 2016

totten commented Feb 6, 2017

totten Feb 6, 2017

eileenmcnaughton Feb 6, 2017

eileenmcnaughton commented Feb 6, 2017

totten Feb 6, 2017

totten Feb 6, 2017

eileenmcnaughton Feb 6, 2017

totten Feb 8, 2017

totten Feb 8, 2017

eileenmcnaughton commented Feb 8, 2017

totten commented Mar 13, 2017 •

edited

eileenmcnaughton commented Mar 13, 2017

eileenmcnaughton commented Mar 13, 2017

totten commented Mar 14, 2017

eileenmcnaughton commented Mar 14, 2017

agh1 commented Apr 4, 2017

totten commented Apr 4, 2017

agh1 commented Apr 4, 2017

eileenmcnaughton commented Apr 4, 2017

eileenmcnaughton commented Apr 4, 2017

Settings #9420

Settings #9420

Conversation

eileenmcnaughton commented Nov 21, 2016

totten commented Feb 6, 2017

totten Feb 6, 2017

Choose a reason for hiding this comment

eileenmcnaughton Feb 6, 2017

Choose a reason for hiding this comment

eileenmcnaughton commented Feb 6, 2017

totten Feb 6, 2017

Choose a reason for hiding this comment

totten Feb 6, 2017

Choose a reason for hiding this comment

eileenmcnaughton Feb 6, 2017

Choose a reason for hiding this comment

totten Feb 8, 2017

Choose a reason for hiding this comment

totten Feb 8, 2017

Choose a reason for hiding this comment

eileenmcnaughton commented Feb 8, 2017

totten commented Mar 13, 2017 • edited

eileenmcnaughton commented Mar 13, 2017

eileenmcnaughton commented Mar 13, 2017

totten commented Mar 14, 2017

eileenmcnaughton commented Mar 14, 2017

agh1 commented Apr 4, 2017

totten commented Apr 4, 2017

agh1 commented Apr 4, 2017

eileenmcnaughton commented Apr 4, 2017

eileenmcnaughton commented Apr 4, 2017

totten commented Mar 13, 2017 •

edited