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#183 Remove references to and noisly deprecated CRM_Core_DAO::createTempTableName #15793

Merged

Conversation

seamuslee001
Copy link
Contributor

Overview

This removes the last references to CRM_Core_DAO::createTempTableName and also adds in some noisy deprecation

Before

Deprecated function used and not noisley deprecated

After

Deprecated function not used and noisely deprecated

This includes #15792

ping @eileenmcnaughton @totten @monishdeb

@civibot
Copy link

civibot bot commented Nov 9, 2019

(Standard links)

@civibot civibot bot added the master label Nov 9, 2019
@seamuslee001 seamuslee001 force-pushed the depreacete_dao_temp_table_name branch 4 times, most recently from 9d62b52 to 5013476 Compare November 10, 2019 19:38
@@ -4200,7 +4200,10 @@ public function relationship(&$values) {
$relationshipTempTable = NULL;
if (self::$_relType == 'reciprocal') {
$where = [];
self::$_relationshipTempTable = $relationshipTempTable = CRM_Core_DAO::createTempTableName('civicrm_rel');
self::$_relationshipTempTable = $relationshipTempTable = CRM_Utils_SQL_TempTable::build()
->setCategory('civicrm_rel')
Copy link
Member

@totten totten Nov 10, 2019

Choose a reason for hiding this comment

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

BG: createTempTableName($prefix) and tempTable->category are not interchangeable. The former yields a table name like civicrm_rel_temp_1234 while the latter would look like civicrm_tmp_e_civicrm_rel_1234. The category has tighter validation.

Comment: When I manually run TempTable::build() like this in cv, it throws an exception b/c the category has _. Assuming that this change is safe (e.g. there's no other code that relies on the /^civicrm_rel_/ prefix), then you could just do rel or drop the category completely.

@seamuslee001
Copy link
Contributor Author

Test failures relate here

CRM_Contact_BAO_QueryTest.testReciprocalRelationshipTargetGroupIsCorrectResults
CRM_Contact_BAO_QueryTest.testReciprocalRelationshipTargetGroupUsesTempTable

@seamuslee001 seamuslee001 force-pushed the depreacete_dao_temp_table_name branch from 5013476 to fe6bf8b Compare November 17, 2019 20:55
@eileenmcnaughton
Copy link
Contributor

@seamuslee001 just to build on @totten's note about the categories - it's more helpful when a category is NOT used -because then any tables like civicrm_tmp* can be excluded from replication. We miss that main benefit when using a category - although there are likely cases where it makes sense to do I'm not sure what they are. In general no category is better than category

@seamuslee001
Copy link
Contributor Author

@eileenmcnaughton category should not stop that because the category is added as the 3rd param so the table name would be civicrm_tmp_{e|d}_{category}_{rand}

@seamuslee001 seamuslee001 force-pushed the depreacete_dao_temp_table_name branch from fe6bf8b to 7a1734a Compare November 21, 2019 05:42
@seamuslee001
Copy link
Contributor Author

ok @eileenmcnaughton @totten removed the category and also neatened up some of the test changes, i think this is good now, The test failures have demonstrated we do have unit test coverage of the relationship change

@seamuslee001 seamuslee001 force-pushed the depreacete_dao_temp_table_name branch from 7a1734a to 000b20f Compare November 21, 2019 05:44
…bleName

Update unit test to match new temp table format
@eileenmcnaughton
Copy link
Contributor

Code seems logical & if this didn't work I would expect that relationship search would fatal - which it didn't....

@jitendrapurohit
Copy link
Contributor

Reciprocal relationship search with custom fields leads to an error on Advanced search after this change. Have raised a PR for the fix at #17132

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.

4 participants