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#474 - Show missing log table warning only if logging is enabled #13010

Merged
merged 1 commit into from
Oct 26, 2018

Conversation

jitendrapurohit
Copy link
Contributor

Overview

Fix missing log table warning on the status page.

Before

Missing Log table warning is shown even if logging is disabled in the following scenario -

  • Logging is enabled on the site which generates a list of log tables in the database.
  • User now disables the logging functionality on the site which now stops generating any more log tables.
  • User create a custom set on the site which creates a new table in the database.
  • The system status page notices a missing log table for the above and displays a warning.

The last step should also check if the logging is enabled on the site even if it is able to find some core log tables already present in the db.

After

Missing log table warning only shown if logging is enabled.

Comments

Gitlab - https://lab.civicrm.org/dev/core/issues/474

@civibot
Copy link

civibot bot commented Oct 25, 2018

(Standard links)

@civibot civibot bot added the master label Oct 25, 2018

if ($missingLogTables) {
if ($config->logging && $missingLogTables) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not just CRM_Core_Config::singleton()->logging ? @eileenmcnaughton

Copy link
Contributor

Choose a reason for hiding this comment

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

@seamuslee001 I agree that would be better - but I see your CRM_Core_Config::singleton()->logging and raise you a \Civi::setings()->get('logging');

Copy link
Contributor

Choose a reason for hiding this comment

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

yeh setting is probably better / more straight forward

Copy link
Contributor

Choose a reason for hiding this comment

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

@seamuslee001 but you must be pre-coffee ATM - can we trust anything you say right now?

Copy link
Contributor

Choose a reason for hiding this comment

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

a bit yes but it also your suggestion you know

@eileenmcnaughton
Copy link
Contributor

We should definitely do this - @jitendrapurohit can you tweak the line to one of the other options just as part of discouraging the pattern of defining $config & then using it. My pref is the setting version....

@jitendrapurohit
Copy link
Contributor Author

Done 👍

@seamuslee001
Copy link
Contributor

Merging as per the tag

@seamuslee001 seamuslee001 merged commit 30b5e58 into civicrm:master Oct 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants