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

dev/core#1863 Downgrade checkEnvironment level and skip non-prod checks #17807

Merged
merged 1 commit into from Jul 13, 2020

Conversation

colemanw
Copy link
Member

Overview

In non-production environments, CiviMail and other Cron-based jobs are disabled, yet the system checks for them still run. There's also a system check that reports non-production environments, and it uses an inappropriately high alert level.

Based on discussion in dev/core#1863

Before

  • checkEnvironment blasts its message at \Psr\Log\LogLevel::ALERT
  • CiviMail and Cron checks run and display alerts even though they are disabled in non-production environments.

After

  • checkEnvironment level downgraded to \Psr\Log\LogLevel::NOTICE
  • CiviMail and Cron checks skipped in non-production environments.

@civibot civibot bot added the master label Jul 13, 2020
@civibot
Copy link

civibot bot commented Jul 13, 2020

(Standard links)

@@ -142,7 +142,7 @@ public function checkDebug() {
ts('Warning: Debug is enabled in <a href="%1">system settings</a>. This should not be enabled on production servers.',
[1 => CRM_Utils_System::url('civicrm/admin/setting/debug', 'reset=1')]),
ts('Debug Mode Enabled'),
\Psr\Log\LogLevel::WARNING,
CRM_Core_Config::environment() == 'Production' ? \Psr\Log\LogLevel::WARNING : \Psr\Log\LogLevel::INFO,
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't get pop ups for info do you?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right

@eileenmcnaughton
Copy link
Contributor

This looks good - I have only given the code a light read but pending a better review we should merge IMHO

@colemanw
Copy link
Member Author

@kcristiano do you have time to review this one as well?

@kcristiano
Copy link
Member

@colemanw I did an r-run applying this on top of #17800 and reinstalling.

Works as expected.

@colemanw colemanw added the merge ready PR will be merged after a few days if there are no objections label Jul 13, 2020
@eileenmcnaughton eileenmcnaughton merged commit 10e1320 into civicrm:master Jul 13, 2020
@eileenmcnaughton eileenmcnaughton deleted the nonProdCronCheck branch July 13, 2020 19:53
@colemanw
Copy link
Member Author

Thanks @kcristiano

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
3 participants