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

Text supports up to 16 megabytes, not 2 #6368

Merged
merged 1 commit into from
Feb 14, 2017

Conversation

anselmdk
Copy link
Contributor

@anselmdk anselmdk commented Dec 6, 2016

See the requireField method that defines it as mediumtext.

See the `requireField` method that defines it as `mediumtext`.
@helpfulrobot
Copy link

@anselmdk, thanks for your PR! By analyzing the blame information on this pull request, I identified @chillu and @sminnee to be potential reviewers

Copy link
Member

@chillu chillu left a comment

Choose a reason for hiding this comment

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

Can you please check if that's the case in the other supported databases as well? Postgres, SQLite, MSSQL

@tractorcow
Copy link
Contributor

I'd rather remove the size constraint at all; It's not correct to declare db-level storage limitations at the orm level.

@assertchris
Copy link
Contributor

Agree with @tractorcow - removing these constraints (yet keeping the ability to constrain) seems like a good idea to me.

@anselmdk
Copy link
Contributor Author

anselmdk commented Dec 7, 2016

I agree that making them configurable would be the best option. Maybe just through config.yml.

@sminnee
Copy link
Member

sminnee commented Feb 14, 2017

This discussion is completely unrelated to the PR which is correcting some docs.

Removing these constraints requires a patch to MySQL.

I've confirmed that the limit is 16mb on MSSQL. I can't get clarity on PostgreSQL's limits; as best I can see it's unrestricted. SQLite has a 1gb limit.

I'm merging this and closing the PR.

@sminnee sminnee merged commit 449ed8d into silverstripe:master Feb 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants