-
-
Notifications
You must be signed in to change notification settings - Fork 825
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-20774 - Add check for existing key index in table #10566
Conversation
5cff2b6
to
f4f835e
Compare
@jitendrapurohit looks like you got some test failures buddy https://test.civicrm.org/job/CiviCRM-Core-PR/15987/testReport/(root)/api_v3_SystemCheckTest/testSystemCheckBasic/ |
Oh, I thought we'd fixed "Update Incices" but I see it in the screenshot above. Make sure you haven't brought back "Incices" instead of "Indices"! (I don't think you have by looking at the changes, but ...) |
No, the screenshot is from one of our dev site(inz) which is on 4.7.19 and the typo fix was done in 4.7.20. I've checked on local(master) and the button looks correct there. |
This looks good to me, my only thoughts is perhaps there should be a message about reporting it to your administrator if you cannot alter the database or do not know now? ping @eileenmcnaughton |
I'm inclined to think this is good to merge & we could cover the messaging as a follow up? |
I would be ok with that but do you think that would be a good text addition? |
yep |
Happy for this to be merged then |
Thanks all. |
getMissingIndices()
retrieves the indices based on the value and not on the key. So there is a possibility that it'll also fetch the value -For eg.
As
is_selected
is not in the table,updateindices
would attempt to re-create the index which will lead to duplicateindex_all
key error.This PR displays the existing keys on system status page and ask the user to drop the index so that civi creates the correct one with all columns again.
Test added.