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

Fix for failure (fatal error and silent failure) to clean up full text indices #20921

Merged
merged 1 commit into from
Aug 2, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Fix for failure to clean up full text indices

This builds on the test that fails on mariadb 10.4 locally #20920 and adds a fix that works for me locally based on

https://stackoverflow.com/questions/4107599/show-a-tables-fulltext-indexed-columns

Before

Hard fail when calling findActualFtsIndexNames on mariabd 10.4 locally. When I fix it to call the sql in the 'non mysql 8 way' to reflect the tables that are present I get an error on the new test assertion that checks the FULL TEXT indexes were found and deleted.

After

Locally this successfully indentifies and deletes the indices.

Technical Details

This is also much simpler - the question is whether it works on all sql versions in our matrix

Comments

@civibot
Copy link

civibot bot commented Jul 21, 2021

(Standard links)

@civibot civibot bot added the master label Jul 21, 2021
@eileenmcnaughton eileenmcnaughton changed the title Cleanup3 Fix for failure (fatal error and silent failure) to clean up full text indices Jul 21, 2021
@seamuslee001
Copy link
Contributor

@eileenmcnaughton I tried this on Max (5.7) and edge (mysql 8) on Max it worked but on edge it failed with

.
Installing build0test_4lhu9 database
EE                                                                 3 / 3 (100%)

Time: 4.78 seconds, Memory: 61.11 MB

There were 2 errors:

1) CRM_Core_InnoDBIndexerTest::testDisabled
Undefined property: CRM_Core_DAO::$index_name

/home/jenkins/bknix-edge/build/build-0/web/sites/all/modules/civicrm/CRM/Core/InnoDBIndexer.php:174
/home/jenkins/bknix-edge/build/build-0/web/sites/all/modules/civicrm/CRM/Core/InnoDBIndexer.php:210
/home/jenkins/bknix-edge/build/build-0/web/sites/all/modules/civicrm/CRM/Core/InnoDBIndexer.php:227
/home/jenkins/bknix-edge/build/build-0/web/sites/all/modules/civicrm/CRM/Core/InnoDBIndexer.php:123
/home/jenkins/bknix-edge/build/build-0/web/sites/all/modules/civicrm/tests/phpunit/CRM/Core/InnoDBIndexerTest.php:53
/home/jenkins/bknix-edge/build/build-0/web/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviUnitTestCase.php:254
/home/jenkins/bknix-edge/extern/phpunit8/phpunit8.phar:671

2) CRM_Core_InnoDBIndexerTest::testEnabled
Undefined property: CRM_Core_DAO::$index_name

/home/jenkins/bknix-edge/build/build-0/web/sites/all/modules/civicrm/CRM/Core/InnoDBIndexer.php:174
/home/jenkins/bknix-edge/build/build-0/web/sites/all/modules/civicrm/CRM/Core/InnoDBIndexer.php:210
/home/jenkins/bknix-edge/build/build-0/web/sites/all/modules/civicrm/CRM/Core/InnoDBIndexer.php:227
/home/jenkins/bknix-edge/build/build-0/web/sites/all/modules/civicrm/CRM/Core/InnoDBIndexer.php:123
/home/jenkins/bknix-edge/build/build-0/web/sites/all/modules/civicrm/tests/phpunit/CRM/Core/InnoDBIndexerTest.php:78
/home/jenkins/bknix-edge/build/build-0/web/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviUnitTestCase.php:254
/home/jenkins/bknix-edge/extern/phpunit8/phpunit8.phar:671

I then changed the Select in your query to have AS index_name on the end and that made the tests pass on MySQL 8

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 did you try with the alias against the other versions?

@eileenmcnaughton
Copy link
Contributor Author

I guess it's case?

@seamuslee001
Copy link
Contributor

Yeh MySQL8 seems to use the actual column name case rather than what you have in the query which is annoying

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 I've updated it

@seamuslee001
Copy link
Contributor

if this passes PR tests then we are good

@eileenmcnaughton eileenmcnaughton merged commit 67765b8 into civicrm:master Aug 2, 2021
@eileenmcnaughton eileenmcnaughton deleted the cleanup3 branch August 2, 2021 08:45
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.

2 participants