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

Ensure 'type' is always set for query field data. #12488

Closed

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Cleanup metadata for query fields to ensure 'type' is always set

Before

Some manually defined fields don't have 'type' defined. No test

After

All defined fields have 'type' defined. Test

Technical Details

Historically 'type' was set on core fields but 'data_type' was set on custom fields.

For some time now we have set 'type' on custom fields as well so all the handling one
Export class that was written for them is actually bypassed. However, it seems there are a bunch
of manually defined fields that don't have a 'type' (note that both 'type' and 'title' are
ensured for any fields exposed via the api or DAO fields).

Some of these fields with no 'type' DO have a data_type - but the data_type code would pass right
through the loop that handles it as they use the 'type' constants & the handling
loop uses the custom value strings.

In theory, but not, IMHO in practice, this could result in a shorter varchar being used for fields
In https://issues.civicrm.org/jira/browse/CRM-19222 a field length of 512 was made possible for custom field strings. This would have been broken
for many months since the Custom fields no longer hit that loop by virtue of having a valid type.

I'm actually 99.99% sure I could just remove the deprecated switch statement - since I could not
find a single instance where 'type' is not set AND 'data_type' is valid. It would
give them 'varchar(255)' instead of text.

Comments

Separately, I'm not convinced we do ourself any favours by not setting all the
varchar fields to 512 length - it seems to me they take up no more space unless they need to,
in which case more space is better than a hard-fail

https://dev.mysql.com/doc/refman/8.0/en/char.html

@civibot
Copy link

civibot bot commented Jul 17, 2018

(Standard links)

Historically 'type' was set on core fields but 'data_type' was set on custom fields.

For some time now we have set 'type' on custom fields as well so all the handling one
Export class that was written for them is actually bypassed. However, it seems there are a bunch
of manually defined fields that don't have a 'type' (note that both 'type' and 'title' are
ensured for any fields exposed via the api or DAO fields).

These fields with no 'type' DO have a data_type - but the data_type code would pass right
through the loop that handles it as they use the 'type' constants & the handling
loop uses the custom value strings.

In theory, but not, IMHO in practice, this could result in a shorter varchar being used for fields
In CRM-19222 a field length of 512 was made possible for custom field strings. This would have been broken
for many months since the Custom fields no longer hit that loop by virtue of having a valid type.

I'm actually 99.99% sure I could just remove the deprecated switch statement - since I could not
find a single instance where 'type' is not set AND 'data_type' is valid. It would
give them 'varchar(255)' instead of text.

Separately, I'm not convinced we do ourself any favours by not setting all the
varchar fields to 512 length - it seems to me they take up no more space unless they need to,
in which case more space is better than a hard-fail

https://dev.mysql.com/doc/refman/8.0/en/char.html
@eileenmcnaughton
Copy link
Contributor Author

eileenmcnaughton commented Jul 19, 2018

I added a test to #12505 that very specifically identifies any changes made by this. I ran it locally & found...

Column Before this change After Comment
location_type text varchar(255)
im_provider text varchar(16) Pretty sure from my other tests this field doesn't work....
'groups text' longtext FIX BACK
tags text longtext FIX BACK
'notes text' longtext' FIX BACK
'contribution_note text longtext FIX BACK
'contribution_batch text varchar(255)
contribution_product_id varchar(255) varchar(16)
contribution_soft_credit_amount varchar(255) varchar(32) OK - 32 long enough for money IMHO
contribution_soft_credit_contact_id varchar(255) varchar(16)
contribution_soft_credit_contribution_id varchar(255) varchar(16)
participant_note text longtext
pledge_total_paid text varchar(32) OK - 32 long enough for money IMHO
pledge_next_pay_date text varchar(32) OK - 32 long enough for date IMHO
pledge_next_pay_amount text varchar(32) OK - 32 long enough for money IMHO
pledge_contribution_page_id varchar(255) varchar(16)
pledge_frequency_interval varchar(255) varchar(16)
pledge_balance_amount text varchar(32) OK - 32 long enough for money IMHO
pledge_payment_paid_amount text varchar(32) OK - 32 long enough for money IMHO
pledge_payment_paid_date text varchar(32) OK - 32 long enough for date IMHO

@eileenmcnaughton
Copy link
Contributor Author

I'm gonna close this - it's valid but WIP & I will track outside github

@eileenmcnaughton eileenmcnaughton deleted the queryFieldsType branch April 26, 2019 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants