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.

Sign up for GitHub

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

Use "text" type mysql field when exporting any id-type field #12598

Closed
wants to merge 2 commits into from
Closed

Use "text" type mysql field when exporting any id-type field #12598

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ad6f6e4
Use text type field when exporting any id-type field that might get c…
adixon Jul 30, 2018
c53004b
fix special case of exporting the civicrm primary id
adixon Jul 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 13 additions & 21 deletions CRM/Export/BAO/ExportProcessor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,32 +515,22 @@ public function getValidLocationFields() {
*/
public function getSqlColumnDefinition($field) {
$fieldName = $this->getMungedFieldName($field);

// early exit for master_id, CRM-12100
// in the DB it is an ID, but in the export, we retrive the display_name of the master record
// also for current_employer, CRM-16939
if ($fieldName == 'master_id' || $fieldName == 'current_employer') {
return "$fieldName varchar(128)";
}

if (substr($fieldName, -11) == 'campaign_id') {
// CRM-14398
return "$fieldName varchar(128)";
}

$queryFields = $this->getQueryFields();
$lookUp = ['prefix_id', 'suffix_id'];
// special case: civicirm_primary_id exports the contact id and is an index field
if ($fieldName == 'civicrm_primary_id') {
return "$fieldName varchar(16)";
Copy link
Member

Choose a reason for hiding this comment

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

Why would we use varchar to store an integer?

Copy link
Contributor

Choose a reason for hiding this comment

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

So this change will need to be tested on servers with significant data sets for performance - we have a good theory as to what will work well but it needs some concerted testing. It's also possible we can & should eliminate the temp table per my comment above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@colemanw : in an export, in general, a civi field stored as an integer might actually end up getting a long string value (e.g. the gender field, which is what triggerred this PR).

Copy link
Member

Choose a reason for hiding this comment

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

Ok but in the case of civicrm_primary_id we know the temp table is storing an integer, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, so you're asking why varchar(16) for the civicrm_primary_id instead of an integer. The short answer is, I'm not sure, but we're generating the export table and perhaps there's a reason they should all be text fields so we can process them into a csv file more easily? The existing code is converting everything to text fields as well, so that issue is kind of outside the scope of the PR (though it's a reasonable question).

}
// set the sql columns
if (isset($queryFields[$field]['type'])) {
switch ($queryFields[$field]['type']) {
case CRM_Utils_Type::T_INT:
// Integer fields may be references that are retrieved for export as text of arbitrary length.
// We use "text" instead of "varchar(xyz)" to avoid row width limitations of mysql.
// This solution replaces a previous collection of ad-hoc partial solutions.
return "$fieldName text";

case CRM_Utils_Type::T_BOOLEAN:
if (in_array($field, $lookUp)) {
return "$fieldName varchar(255)";
}
else {
return "$fieldName varchar(16)";
}
return "$fieldName varchar(16)";

case CRM_Utils_Type::T_STRING:
if (isset($queryFields[$field]['maxlength'])) {
Expand Down Expand Up @@ -570,8 +560,10 @@ public function getSqlColumnDefinition($field) {
}
}
else {
// handle fields for which we have no 'type' configured (yet!)
if (substr($fieldName, -3, 3) == '_id') {
return "$fieldName varchar(255)";
// Will likely get expanded into text of arbitrary length.
return "$fieldName text";
}
elseif (substr($fieldName, -5, 5) == '_note') {
return "$fieldName text";
Expand Down