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

CRM-20838 - Add check for missing log tables on system status #10628

Merged
merged 2 commits into from
Aug 23, 2017

Conversation

jitendrapurohit
Copy link
Contributor

@jitendrapurohit jitendrapurohit commented Jul 10, 2017

If drupal and civicrm share same db, drupal modules don't create log tables which result into db error while using them. Check issue description for when this could happen.

Instead of handling this on update or installation hook within module, this PR checks for missing log table and displays a message + button (just like Update Indices) on system status page which calls System - updatelogtable api and creates missing log tables.


@jitendrapurohit jitendrapurohit changed the title CRM-20388 - Add check for missing log tables on system status CRM-20838 - Add check for missing log tables on system status Jul 10, 2017
@eileenmcnaughton
Copy link
Contributor

I think this is a good idea. The existing command that you have piggy backed onto also has some complexities to consider

  1. it will actually change the data in the log_conn_id field so that going forwards it will record a uniqueid() rather than a not-very-unique id which was the old schema
  2. it will convert the schema of the log tables to INNODB
  3. given the above it could be a very long operation

While you could work around those things with messaging etc I suspect it makes more sense just to add a new api action System.createmissinglogtables or similar

@jitendrapurohit
Copy link
Contributor Author

jitendrapurohit commented Aug 23, 2017

Added createmissinglogtables api. Also added a minor unit test to check if all works fine @eileenmcnaughton

@monishdeb
Copy link
Member

The patch works for me and addresses the points mentioned by Eileen. Also the added unit test cover the missing log table scenario well by restoring the missing log_civicrm_contact table via System.createmissinglogtables API.

@eileenmcnaughton should I merge it then?

@eileenmcnaughton
Copy link
Contributor

yep!

@eileenmcnaughton eileenmcnaughton merged commit 649b362 into civicrm:master Aug 23, 2017
@jitendrapurohit jitendrapurohit deleted the CRM-20838 branch August 24, 2017 04:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants