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

[adixon]

Use text type field when exporting any id-type field that might get converted into text of arbitrary length

Overview

Exports of fields that contain "id" type values may contain text of arbitrary length (e.g. values from option groups).

1. Fix the code so that the previous collection of ad-hoc fixes are replaced by a more comprehensive solution.
2. Replace the use of varchar(xyz) fields with text type fields to reduce the chance of the row length maximum being reached.

Before

Add a gender type "I am not sure and neither are you" (i.e. anything more than 16 characters). You will be unable to export contacts if you include gender in your export fields: you'll get the unhelpful "DB Error Unknown error" fatal error.

After

Export now works.

Technical Details

1. This PR as per the discussion here: https://lab.civicrm.org/dev/core/issues/181
2. Useful documentation about row length contributions of different field types (i.e. why we're using text instead of varchar) is here: https://dev.mysql.com/doc/refman/8.0/en/column-count-limit.html#row-size-limits.
3. Replaces the code fixes of CRM-12100, CRM-16939, CRM-14398 (at least).

Comments

Testing the effects of the change in a large export would be worthwhile, i.e the performance and disk-usage implications.

---

• CRM-12100: On export participants, fatal error if shared address belongs to contact with very long name
• CRM-16939: Current Employer field is cut off at 64 characters on export
• CRM-14398: Fatal error exporting contributions with campaigns, if campaign name > 16 chars

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

@adixon for now the test fails relate - https://test.civicrm.org/job/CiviCRM-Core-PR/21897/testReport/junit/(root)/CRM_Export_BAO_ExportTest__testGetSQLColumns/testGetSQLColumns_with_data_set__0/ but I agree that performance testing is the key here - we should make sure we get some exports done of ~60000+ rows & comparitive times on it

[adixon]

So there's one special case - the primary civicrm id really is exported as an integer number, and it's used as an index on the table, so we need to give it a varchar type. I just updated my PR.

[eileenmcnaughton]

Once I've got the rest of the cleanup on export merged to an rc I'll test this + export in general on some larger exports & get some stats

[eileenmcnaughton]

As as aside @adixon the more I dig into the export the less sure I am that the temp table is used in a way that it improves performance. We actually iterate through every row & write it to the temp table & then pull it out & write to csv. I think the way the merge household & merge address was written is reliant on that process but at least in the case of merge household I'm sure that it could be done at least as efficiently without the temp table

[davidjosephhayes]

Is there anything I can do to help get this pull requested merged?

[colemanw]

Why would we use varchar to store an integer?

[eileenmcnaughton]

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

[adixon]

@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).

[colemanw]

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

[adixon]

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).

[eileenmcnaughton]

I'm closing this out because it's stale - I actually think the answer, after a lot of digging in that code, is not to write the translated fields to a temp table. We only actually need the temp table when merging same addresses

[adixon]

Are you proposing an alternative fix? This is a bug that a client experienced and the patch does resolve it (and also cleans up a few other past hacks), so if you're suggesting an alternative way of resolving it then a few more details might be reasonable to request?

[eileenmcnaughton]

The code has changed quite a bit so the patch doesn't apply anymore - but basically I think we need to review what we are writing to the temp table and why - there really isn't much reason for the temp table in most cases as we iterate through the entire php array anyway it doesn't help with performance and we could just write straight to the csv and skip the temp table.

The one blocker to skipping the temp table is checking how we'd handle merge to address