From 675e2573392ea986c5c407937738d3fcff337ecb Mon Sep 17 00:00:00 2001 From: Coleman Watts Date: Mon, 13 Jul 2020 20:29:06 -0400 Subject: [PATCH] SystemCheck: add ability to efficiently run only specified checks This changes the hook_civicrm_check() signature in a backward-compat fashion, adding 2 params that can be used by the hook subscriber to filter by name if specified. --- .../Incremental/sql/5.29.alpha1.mysql.tpl | 4 + CRM/Utils/Check.php | 49 ++++++++---- CRM/Utils/Check/Component.php | 48 +++++++++-- CRM/Utils/Check/Component/Env.php | 31 ++++--- CRM/Utils/Hook.php | 17 ++-- Civi/Api4/Action/System/Check.php | 31 ++++++- Civi/Api4/Generic/AbstractGetAction.php | 2 +- tests/phpunit/api/v4/Entity/SystemTest.php | 80 +++++++++++++++++++ 8 files changed, 220 insertions(+), 42 deletions(-) create mode 100644 tests/phpunit/api/v4/Entity/SystemTest.php diff --git a/CRM/Upgrade/Incremental/sql/5.29.alpha1.mysql.tpl b/CRM/Upgrade/Incremental/sql/5.29.alpha1.mysql.tpl index 7508dec80c..4d83f9ac4a 100644 --- a/CRM/Upgrade/Incremental/sql/5.29.alpha1.mysql.tpl +++ b/CRM/Upgrade/Incremental/sql/5.29.alpha1.mysql.tpl @@ -1,5 +1,9 @@ {* file to handle db changes in 5.29.alpha1 during upgrade *} +{* https://github.com/civicrm/civicrm-core/pull/17824 *} +UPDATE civicrm_status_pref SET name = 'checkExtensionsOk' WHERE name = 'extensionsOk'; +UPDATE civicrm_status_pref SET name = 'checkExtensionsUpdates' WHERE name = 'extensionUpdates'; + -- The RelationshipCache is a high-level index/cache for querying relationships. DROP TABLE IF EXISTS `civicrm_relationship_cache`; CREATE TABLE `civicrm_relationship_cache` ( diff --git a/CRM/Utils/Check.php b/CRM/Utils/Check.php index e92bb9ec7c..1e4d9f7df7 100644 --- a/CRM/Utils/Check.php +++ b/CRM/Utils/Check.php @@ -170,7 +170,7 @@ class CRM_Utils_Check { } /** - * Run all system checks. + * Run all enabled system checks. * * This functon is wrapped by the System.check api. * @@ -180,21 +180,10 @@ class CRM_Utils_Check { * @param bool $max * Whether to return just the maximum non-hushed severity * - * @return array - * Array of CRM_Utils_Check_Message objects + * @return CRM_Utils_Check_Message[] */ public static function checkAll($max = FALSE) { - $messages = []; - foreach (glob(__DIR__ . '/Check/Component/*.php') as $filePath) { - $className = 'CRM_Utils_Check_Component_' . basename($filePath, '.php'); - /* @var CRM_Utils_Check_Component $check */ - $check = new $className(); - if ($check->isEnabled()) { - $messages = array_merge($messages, $check->checkAll()); - } - } - - CRM_Utils_Hook::check($messages); + $messages = self::checkStatus(); uasort($messages, [__CLASS__, 'severitySort']); @@ -212,6 +201,38 @@ class CRM_Utils_Check { return ($max) ? $maxSeverity : $messages; } + /** + * @param array $statusNames + * Optionally specify the names of specific checks to run, or leave empty to run all + * @param bool $includeDisabled + * Run checks that have been explicitly disabled (default false) + * + * @return CRM_Utils_Check_Message[] + */ + public static function checkStatus($statusNames = [], $includeDisabled = FALSE) { + $messages = []; + $checksNeeded = $statusNames; + foreach (glob(__DIR__ . '/Check/Component/*.php') as $filePath) { + $className = 'CRM_Utils_Check_Component_' . basename($filePath, '.php'); + /* @var CRM_Utils_Check_Component $component */ + $component = new $className(); + if ($includeDisabled || $component->isEnabled()) { + $messages = array_merge($messages, $component->checkAll($statusNames, $includeDisabled)); + } + if ($statusNames) { + // Early return if we have already run (or skipped) all the requested checks. + $checksNeeded = array_diff($checksNeeded, $component->getAllChecks()); + if (!$checksNeeded) { + return $messages; + } + } + } + + CRM_Utils_Hook::check($messages, $statusNames, $includeDisabled); + + return $messages; + } + /** * @param int $level * @return string diff --git a/CRM/Utils/Check/Component.php b/CRM/Utils/Check/Component.php index 3ecf8bd347..5b5dcc7462 100644 --- a/CRM/Utils/Check/Component.php +++ b/CRM/Utils/Check/Component.php @@ -44,26 +44,60 @@ abstract class CRM_Utils_Check_Component { return TRUE; } + /** + * Get the names of all check functions in this class + * + * @return string[] + */ + public function getAllChecks() { + return array_filter(get_class_methods($this), function($method) { + return $method !== 'checkAll' && strpos($method, 'check') === 0; + }); + } + /** * Run all checks in this class. * - * @return array - * [CRM_Utils_Check_Message] + * @param array $requestedChecks + * Optionally specify the names of specific checks requested, or leave empty to run all + * @param bool $includeDisabled + * Run checks that have been explicitly disabled (default false) * - * @throws \API_Exception + * @return CRM_Utils_Check_Message[] + * + * @throws API_Exception * @throws \Civi\API\Exception\UnauthorizedException */ - public function checkAll() { + public function checkAll($requestedChecks = [], $includeDisabled = FALSE) { $messages = []; - foreach (get_class_methods($this) as $method) { + foreach ($this->getAllChecks() as $method) { // Note that we should check if the test is disabled BEFORE running it in case it's disabled for performance. - if ($method !== 'checkAll' && strpos($method, 'check') === 0 && !$this->isDisabled($method)) { - $messages = array_merge($messages, $this->$method()); + if ($this->isRequested($method, $requestedChecks) && ($includeDisabled || !$this->isDisabled($method))) { + $messages = array_merge($messages, $this->$method($includeDisabled)); } } return $messages; } + /** + * Is this check one of those requested + * + * @param string $method + * @param array $requestedChecks + * @return bool + */ + private function isRequested($method, $requestedChecks) { + if (!$requestedChecks) { + return TRUE; + } + foreach ($requestedChecks as $name) { + if (strpos($name, $method) === 0) { + return TRUE; + } + } + return FALSE; + } + /** * Is the specified check disabled. * diff --git a/CRM/Utils/Check/Component/Env.php b/CRM/Utils/Check/Component/Env.php index 76ae62ca5f..24b54a9473 100644 --- a/CRM/Utils/Check/Component/Env.php +++ b/CRM/Utils/Check/Component/Env.php @@ -157,13 +157,14 @@ class CRM_Utils_Check_Component_Env extends CRM_Utils_Check_Component { } /** + * @param bool $force * @return CRM_Utils_Check_Message[] */ - public function checkOutboundMail() { + public function checkOutboundMail($force = FALSE) { $messages = []; // CiviMail doesn't work in non-production environments; skip. - if (CRM_Core_Config::environment() != 'Production') { + if (!$force && CRM_Core_Config::environment() != 'Production') { return $messages; } @@ -188,13 +189,14 @@ class CRM_Utils_Check_Component_Env extends CRM_Utils_Check_Component { /** * Check that domain email and org name are set + * @param bool $force * @return CRM_Utils_Check_Message[] */ - public function checkDomainNameEmail() { + public function checkDomainNameEmail($force = FALSE) { $messages = []; // CiviMail doesn't work in non-production environments; skip. - if (CRM_Core_Config::environment() != 'Production') { + if (!$force && CRM_Core_Config::environment() != 'Production') { return $messages; } @@ -238,13 +240,14 @@ class CRM_Utils_Check_Component_Env extends CRM_Utils_Check_Component { /** * Checks if a default bounce handling mailbox is set up + * @param bool $force * @return CRM_Utils_Check_Message[] */ - public function checkDefaultMailbox() { + public function checkDefaultMailbox($force = FALSE) { $messages = []; // CiviMail doesn't work in non-production environments; skip. - if (CRM_Core_Config::environment() != 'Production') { + if (!$force && CRM_Core_Config::environment() != 'Production') { return $messages; } @@ -273,14 +276,15 @@ class CRM_Utils_Check_Component_Env extends CRM_Utils_Check_Component { /** * Checks if cron has run in the past hour (3600 seconds) + * @param bool $force * @return CRM_Utils_Check_Message[] * @throws CRM_Core_Exception */ - public function checkLastCron() { + public function checkLastCron($force = FALSE) { $messages = []; // Cron doesn't work in non-production environments; skip. - if (CRM_Core_Config::environment() != 'Production') { + if (!$force && CRM_Core_Config::environment() != 'Production') { return $messages; } @@ -655,7 +659,7 @@ class CRM_Utils_Check_Component_Env extends CRM_Utils_Check_Component { if (!$okextensions && !$updates && !$errors) { $messages[] = new CRM_Utils_Check_Message( - 'extensionsOk', + __FUNCTION__ . 'Ok', ts('No extensions installed. Browse available extensions.', [ 1 => 'href="' . CRM_Utils_System::url('civicrm/admin/extensions', 'reset=1') . '"', ]), @@ -677,7 +681,7 @@ class CRM_Utils_Check_Component_Env extends CRM_Utils_Check_Component { if ($updates) { $messages[] = new CRM_Utils_Check_Message( - 'extensionUpdates', + __FUNCTION__ . 'Updates', '', ts('Extension Update Available', ['plural' => '%count Extension Updates Available', 'count' => count($updates)]), \Psr\Log\LogLevel::WARNING, @@ -693,7 +697,7 @@ class CRM_Utils_Check_Component_Env extends CRM_Utils_Check_Component { $message = ts('All extensions are up-to-date:'); } $messages[] = new CRM_Utils_Check_Message( - 'extensionsOk', + __FUNCTION__ . 'Ok', $message . '', ts('Extensions'), \Psr\Log\LogLevel::INFO, @@ -820,13 +824,14 @@ class CRM_Utils_Check_Component_Env extends CRM_Utils_Check_Component { /** * Ensure reply id is set to any default value + * @param bool $force * @return CRM_Utils_Check_Message[] */ - public function checkReplyIdForMailing() { + public function checkReplyIdForMailing($force = FALSE) { $messages = []; // CiviMail doesn't work in non-production environments; skip. - if (CRM_Core_Config::environment() != 'Production') { + if (!$force && CRM_Core_Config::environment() != 'Production') { return $messages; } diff --git a/CRM/Utils/Hook.php b/CRM/Utils/Hook.php index 5a5e612c05..e59075fa00 100644 --- a/CRM/Utils/Hook.php +++ b/CRM/Utils/Hook.php @@ -2411,13 +2411,20 @@ abstract class CRM_Utils_Hook { /** * Check system status. * - * @param array $messages - * Array. A list of messages regarding system status. + * @param CRM_Utils_Check_Message[] $messages + * A list of messages regarding system status + * @param array $statusNames + * If specified, only these checks are being requested and others should be skipped + * @param bool $includeDisabled + * Run checks that have been explicitly disabled (default false) * @return mixed */ - public static function check(&$messages) { - return self::singleton() - ->invoke(['messages'], $messages, self::$_nullObject, self::$_nullObject, self::$_nullObject, self::$_nullObject, self::$_nullObject, 'civicrm_check'); + public static function check(&$messages, $statusNames = [], $includeDisabled = FALSE) { + return self::singleton()->invoke(['messages'], + $messages, $statusNames, $includeDisabled, + self::$_nullObject, self::$_nullObject, self::$_nullObject, + 'civicrm_check' + ); } /** diff --git a/Civi/Api4/Action/System/Check.php b/Civi/Api4/Action/System/Check.php index 9080f60a8d..19df2a2a11 100644 --- a/Civi/Api4/Action/System/Check.php +++ b/Civi/Api4/Action/System/Check.php @@ -14,12 +14,39 @@ namespace Civi\Api4\Action\System; /** * Retrieve system notices, warnings, errors, etc. + * @method bool getIncludeDisabled() */ class Check extends \Civi\Api4\Generic\BasicGetAction { + /** + * Run checks that have been explicitly disabled (default false) + * @var bool + */ + protected $includeDisabled = FALSE; + + /** + * @param bool $includeDisabled + * @return Check + */ + public function setIncludeDisabled(bool $includeDisabled): Check { + $this->includeDisabled = $includeDisabled; + return $this; + } + protected function getRecords() { - $messages = []; - foreach (\CRM_Utils_Check::checkAll() as $message) { + $messages = $names = []; + + // Filtering by name relies on the component check rather than the api arrayQuery + // @see \CRM_Utils_Check_Component::isCheckable + foreach ($this->where as $i => $clause) { + if ($clause[0] == 'name' && !empty($clause[2]) && in_array($clause[1], ['=', 'IN'], TRUE)) { + $names = (array) $clause[2]; + unset($this->where[$i]); + break; + } + } + + foreach (\CRM_Utils_Check::checkStatus($names, $this->includeDisabled) as $message) { $messages[] = $message->toArray(); } return $messages; diff --git a/Civi/Api4/Generic/AbstractGetAction.php b/Civi/Api4/Generic/AbstractGetAction.php index aaea4ca3c2..153f2e60a0 100644 --- a/Civi/Api4/Generic/AbstractGetAction.php +++ b/Civi/Api4/Generic/AbstractGetAction.php @@ -105,7 +105,7 @@ abstract class AbstractGetAction extends AbstractQueryAction { protected function _itemsToGet($field) { foreach ($this->where as $clause) { // Look for exact-match operators (=, IN, or LIKE with no wildcard) - if ($clause[0] == $field && (in_array($clause[1], ['=', 'IN']) || ($clause[1] == 'LIKE' && !(is_string($clause[2]) && strpos($clause[2], '%') !== FALSE)))) { + if ($clause[0] == $field && (in_array($clause[1], ['=', 'IN'], TRUE) || ($clause[1] == 'LIKE' && !(is_string($clause[2]) && strpos($clause[2], '%') !== FALSE)))) { return (array) $clause[2]; } } diff --git a/tests/phpunit/api/v4/Entity/SystemTest.php b/tests/phpunit/api/v4/Entity/SystemTest.php new file mode 100644 index 0000000000..11203e70ec --- /dev/null +++ b/tests/phpunit/api/v4/Entity/SystemTest.php @@ -0,0 +1,80 @@ +setHook('civicrm_check', [$this, 'hook_civicrm_check']); + + // Test on non-prod site + Setting::set()->addValue('environment', 'Development')->setCheckPermissions(FALSE)->execute(); + + StatusPreference::delete()->setCheckPermissions(FALSE)->addWhere('name', '=', 'checkLastCron')->execute(); + + // Won't run on non-prod site without $includeDisabled + $check = System::check()->addWhere('name', '=', 'checkLastCron')->execute(); + // Will have skipped our hook because name matched a core check + $this->assertCount(0, $check); + + // This should only run the php check + $check = System::check()->addWhere('name', '=', 'checkPhpVersion')->setIncludeDisabled(TRUE)->execute(); + // Hook should have been skipped because name clause was fulfilled + $this->assertCount(1, $check); + + // Ensure cron check has not run + $this->assertCount(0, StatusPreference::get()->setCheckPermissions(FALSE)->addWhere('name', '=', 'checkLastCron')->execute()); + + // Will run on non-prod site with $includeDisabled. + // Giving a more-specific name will run all checks with less-specific names too + $check = System::check()->addWhere('name', '=', 'checkLastCronAbc')->setIncludeDisabled(TRUE)->execute()->indexBy('name'); + // Will have run our hook too because name wasn't an exact match + $this->assertCount(2, $check); + $this->assertEquals('Ok', $check['hook_civicrm_check']['title']); + + // We know the cron check has run because it would have left a record marked 'new' + $record = StatusPreference::get()->setCheckPermissions(FALSE)->addWhere('name', '=', 'checkLastCron')->execute()->first(); + $this->assertEquals('new', $record['prefs']); + + // Restore env + Setting::set()->addValue('environment', $origEnv)->setCheckPermissions(FALSE)->execute(); + $hooks->reset(); + } + + public function hook_civicrm_check(&$messages, $statusNames, $includeDisabled) { + $messages[] = new \CRM_Utils_Check_Message( + __FUNCTION__, + 'Hook running', + 'Ok', + \Psr\Log\LogLevel::DEBUG + ); + } + +} -- 2.25.1