Details
-
Type:
Bug
-
Status: Open
-
Priority:
Minor
-
Resolution: Unresolved
-
Affects Version/s: 4.7.20
-
Fix Version/s: 4.7.23
-
Component/s: Core CiviCRM
-
Labels:
-
Versioning Impact:Patch (backwards-compatible bug fixes)
-
Documentation Required?:None
-
Funding Source:Needs Funding
-
Verified?:No
Description
I have been hitting an edge case in a testing environment where it fails in the DAO because
$_DB_DATAOBJECT['CONNECTIONS'][$this->_database_dsn_md5]
is null
This happens during unit tests outside civi core tests and it's hard to see how it could happen outside tests. What happens is test 1 calls civicrm_initialize which populates $_DB_DATAOBJECT['CONNECTIONS'], however, on tearDown it is unpopulated.
In test 2 it is NOT populated as civicrm_initialize returns early as the static variable is set to TRUE. Any function that instantiates a DAO, including calling CRM_Core_DAO::executeQuery will cause 'CONNECTIONS' to be populated. However, if the very first db action is an api create call then the 'BEGIN' mysql query will be the first one called. This bypasses executeQuery & uses an obscure alternative ->query() - which does NOT cause the connection to be populated. Because it is using a cached dao object it does not instantiate a new one, meaning that path is skipped too. I have simply done what executeQuery does and added a call to instantiate a DAO if connections does not exist
Failed asserting that 'Trying to get property of non-objectArray
(
[trace] => #0 /Users/emcnaughton/buildkit/build/d46/civicrm/CRM/Core/DAO.php(347): PHPUnit_Util_ErrorHandler::handleError(8, 'Trying to get p...', '/Users/emcnaugh...', 347, Array)
#1 /Users/emcnaughton/buildkit/build/d46/civicrm/Civi/Core/Transaction/Frame.php(132): CRM_Core_DAO->query('BEGIN')
#2 /Users/emcnaughton/buildkit/build/d46/civicrm/Civi/Core/Transaction/Manager.php(87): Civi\Core\Transaction\Frame->begin()
#3 /Users/emcnaughton/buildkit/build/d46/civicrm/CRM/Core/Transaction.php(125): Civi\Core\Transaction\Manager->inc(false)
#4 /Users/emcnaughton/buildkit/build/d46/civicrm/Civi/API/Subscriber/TransactionSubscriber.php(139): CRM_Core_Transaction->__construct(false)
#5 [internal function]: Civi\API\Subscriber\TransactionSubscriber->onApiPrepare(Object(Civi\API\Event\PrepareEvent), 'api.prepare', Object(Symfony\Component\EventDispatcher\ContainerAwareEventDispatcher))
#6 /Users/emcnaughton/buildkit/build/d46/civicrm/vendor/symfony/event-dispatcher/Symfony/Component/EventDispatcher/EventDispatcher.php(164): call_user_func(Array, Object(Civi\API\Event\PrepareEvent), 'api.prepare', Object(Symfony\Component\EventDispatcher\ContainerAwareEventDispatcher))
#7 /Users/emcnaughton/buildkit/build/d46/civicrm/vendor/symfony/event-dispatcher/Symfony/Component/EventDispatcher/EventDispatcher.php(53): Symfony\Component\EventDispatcher\EventDispatcher->doDispatch(Array, 'api.prepare', Object(Civi\API\Event\PrepareEvent))
#8 /Users/emcnaughton/buildkit/build/d46/civicrm/vendor/symfony/event-dispatcher/Symfony/Component/EventDispatcher/ContainerAwareEventDispatcher.php(170): Symfony\Component\EventDispatcher\EventDispatcher->dispatch('api.prepare', Object(Civi\API\Event\PrepareEvent))
#9 /Users/emcnaughton/buildkit/build/d46/civicrm/Civi/API/Kernel.php(248): Symfony\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('api.prepare', Object(Civi\API\Event\PrepareEvent))
#10 /Users/emcnaughton/buildkit/build/d46/civicrm/Civi/API/Kernel.php(160): Civi\API\Kernel->prepare(Object(Civi\API\Provider\MagicFunctionProvider), Array)
#11 /Users/emcnaughton/buildkit/build/d46/civicrm/Civi/API/Kernel.php(92): Civi\API\Kernel->runRequest(Array)
#12 /Users/emcnaughton/buildkit/build/d46/civicrm/api/api.php(43): Civi\API\Kernel->runSafe('Contact', 'create', Array)
#13 /Users/emcnaughton/buildkit/build/d46/sites/all/modules/wmf_common/tests/includes/BaseWmfDrupalPhpUnitTestCase.php(83): civicrm_api3('Contact', 'create', Array)
#14 /Users/emcnaughton/buildkit/build/d46/sites/all/modules/large_donation/tests/LargeDonationTest.php(30): BaseWmfDrupalPhpUnitTestCase->callAPISuccess('Contact', 'create', Array)
#15 phar:///Users/emcnaughton/buildkit/bin/phpunit4/phpunit/Framework/TestCase.php(764): LargeDonationTest->setUp()
#16 phar:///Users/emcnaughton/buildkit/bin/phpunit4/phpunit/Framework/TestResult.php(612): PHPUnit_Framework_TestCase->runBare()
Attachments
Issue Links
Activity
After sleeping a bit... it sounds like the test-class is calling "Civi::reset()" and "civicrm_initialize();" – either within the "setUp()" or the "testFoo()" function. This usage wouldn't have occurred to me, and I'll try to verbalize why it seems weird.
That scenario bootstraps two large, intertwined systems (the CMS and CRM), but it only manages setup/teardown for one of them. If you reset one system without resetting the other system, then the state can get out-of-sync. (In this example, the "static $initialized" in Drupal-land gets out-of-sync with the "CRM_Core_Config::singleton()" state in Civi-land. But it's fairly easy to imagine other Drupal modules retaining old state that references Civi.)
Safer patterns would be:
- Bootstrap CRM+CMS one time – when PHPUnit starts up. (You would never call Civi::reset().)
- This is how the "E2E" tests work.
- Bootstrap/reset the CRM+CMS in every test. (You would call Civi::reset() and any analogous functions in the CMS.)
- You could say that "Headless" tests (eg "CiviUnitTestCase") work this way, but resetting the dummy/headless CMS is trivially easy.
- Some CMSs/frameworks might not require any special work to reset. (Symfony SE comes to mind.)
- For Drupal... Yyou might call "Civi::reset()" and "drupal_static_reset()" at the same time. (Then fix any errant variables like "static $initialized" that don't participate in the scheme.)
The commit currently in for this looks like for each instance of
$dao = new CRM_Core_DAO();
$dao->query('SELECT 1', FALSE);
it will initialise two CRM_Core_DAO(), and run initialize() etc for both of them. If there's any overhead to that at all, this may affect performance. (I don't know if such a change would appear on the radar in current reviewing, so I'm raising it here.)
- Chat link with more details: https://chat.civicrm.org/civicrm/pl/ykdserk4r7g53jjqc6sirpd5er
- The change I'm asking about: https://github.com/civicrm/civicrm-core/pull/10320/commits/96f346f8aa3524884ceb72f6ffe04a332b6bea34
- Resulting _connect() fired twice: https://github.com/civicrm/civicrm-packages/blob/master/DB/DataObject.php#L2177
Suggestion for Eileen: in your test class, try calling `drupal_static_reset()`, then apply https://github.com/civicrm/civicrm-drupal/pull/447 and revert 10320. It might address Chris's concerns, and it should address some undiscovered/asymptomatic issues.
I think I may have misread the change above. I didn't catch the conditional wrapping that double-initialise.
It's weird and smells bad that we'd need to re-initialize the connection, or have to add code like this to frequently used functions just to make tests behave, and that we inspect deep into a global to see if we need to, but on reflection this likely isn't the problem I thought it was.
Yeah, this seems like a valid defensive measure.
It's a little bit weird that it's necessary. My expectation is that you'd have the system bootstrap before running the API call (e.g. `CRM_Core_Config::singleton(); civicrm_api3('Mytest',...)`), and the connection should be initialized during bootstrap. (Lots of things are initialized during bootstrap.) I'm mildly curious about whether other elements of bootstrap are haywire in this scenario.
However, the extra check in #10320 seems safe/innocuous on its own.