CRM-21298 System check: bypass missing indices check until update indices works #11114
Conversation
|
Tim asked me to help with this PR by clarifying some of its context and looping in others who have worked on the relevant code. From what I can tell, this change is bypassing earlier changes made by @jitendrapurohit and merged by @eileenmcnaughton in #10908. Do either of you have thoughts about this PR, especially since it is made against the RC branch and thus requires action more quickly than other PRs? |
|
Thanks @seanmadsen for taking a look at it. The fact that 4.7.25 included #10908 was the only reason I even tried "Update Indices" because I knew that it failed so frequently before. Sadly, it still caused the DB error on 4.7.25. Seeing that CRM-20533 and CRM-20817 are still open, it seems like others are still seeing related problems. The problem here is that unlike most bugs in CiviCRM, this one has a message that pops up and directs you to the broken feature. The purpose of this PR is to suppress that message temporarily until CRM-20533 and CRM-20817 are closed. |
|
I get @agh1 comment but I wouldn't personally see this as meeting the bar for an rc merge because it is not a recent regression or newly identified critical issue |
|
I agree with the principle that @eileenmcnaughton has laid out. My intent in doing this was not to attempt to fix the problems with Update Indices but to quit pushing people to try it. I have no stake in this, though--in fact, the bug has been good for Civi911 business as people call and @tommybobo fixes things manually. I just think it's a bit embarrassing for the product. |
|
@seanmadsen i don't know what conclusion you and @totten have gotten from this discussion, but it would be good to get a decision so at the very least this can get in 4.7.28 and we don't have yet another release popping up messages telling people to run a feature that is broken. |
|
@totten @eileenmcnaughton @seanmadsen I know everyone's got a lot on their plates, but 4.7.27 was released and 4.7.28-rc was cut without ever letting me know this PR was going to be rejected. Would it be okay to open this against 4.7.28-rc, or would you rather this be proposed against 4.7.29. I hate to be a nag, but I really think the situation of telling people to run a process that crashes all the time is terribly embarrassing for the project. |
|
Hi @agh1 my point above was that in general I don't think this is a patch for an rc - it should go against master. Rc patches should only relate to recent regressions (or sometimes improvements to fixes already in the rc) and critical bug fixes |
|
I agree with the general principle @eileenmcnaughton--I had just thought this was a critical bug fix. (Or at least a way of papering over the critical bug until it's actually fixed.) It's all moot anyway because this PR is against a RC branch for a release that's already out. The problem here is that nobody made a decision to merge or close this, and now I need to know whether to submit this for |
|
Replaced by #11250. |
Overview
The "Performance warning: Missing indices" system check should be bypassed until the "Update Indices" task works reliably.
This is in the RC branch because the Update Indices problem is not resolved in 4.7.26
Before
Users of many sites (including as one example a demo site where 4.7.17 was installed and then upgraded to 4.7.25) are presented with a
WARNINGlevel message in the system check stating that they have missing indices. An Update Indices button is in the message. Clicking the button (for many sites) causes a database error as documented in CRM-20817.After
The system check will never produce the "Performance warning: Missing indices" message.
Technical Details
I left all the code in there to run the check and update the indices. The
checkIndices()function just returns an empty array before getting to that point. This allows for easy removal for testing and when the update tool is actually reliable.