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

REF Allow for fields of type Blob or Mediumblob in Apiv4 #19196

Merged
merged 1 commit into from
Dec 13, 2020

Conversation

seamuslee001
Copy link
Contributor

@seamuslee001 seamuslee001 commented Dec 12, 2020

Overview

These are currently known data types but not supported by APIv4 https://github.com/civicrm/civicrm-core/blob/master/CRM/Utils/Type.php#L27 https://github.com/civicrm/civicrm-core/blob/master/CRM/Utils/Type.php#L34

Before

Cannot expose any column of type mediumblob or blob in APIv4 e.g. document column in civicrm_file

After

Can expose them

ping @colemanw @eileenmcnaughton

@civibot
Copy link

civibot bot commented Dec 12, 2020

(Standard links)

@civibot civibot bot added the master label Dec 12, 2020
@colemanw
Copy link
Member

colemanw commented Dec 13, 2020

Ok. Grep shows that these 2 field types are rarely used - in fact only once each.

  • civicrm_file has a blob column to hold file contents. But is this actually used? blob has a size limit of 64k, and AFAIK all files are stored as, well, files. I note that the column is very old (circa 1.5).
  • civicrm_case_type has a mediumblob field to store the serialized definition. Since it's a serialized field, the type as far as the api is concerned should be Array. However, the serialization isn't declared in the schema (and there isn't a serialization type XML).

@seamuslee001
Copy link
Contributor Author

Yeh so I'm not sure but at the same time, I had a custom extension that I am writing where a table currently uses a MediumBlob field and this broke it, I suppose I could convert the column but it feels like if the DAO is outputting it as a legit type shouldn't the API be supporting it?

@colemanw colemanw merged commit 5c7ad4a into civicrm:master Dec 13, 2020
@colemanw
Copy link
Member

Ok let's merge this and continue the conversation.

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