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
Introduce BinaryType #452
Introduce BinaryType #452
Conversation
Hello, thank you for creating this pull request. I have automatically opened an issue http://www.doctrine-project.org/jira/browse/DBAL-714 We use Jira to track the state of pull requests and the versions they got |
public function testListTableWithBinary() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not put that one on the SchemaManagerFunctionalTestCase
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@beberlei It is but it works differently on Drizzle platform as drizzle does not differenciate between fixed and varying binary columns. Therefore the assertion in SchemaManagerFunctionalTestCase
will fail as it tests both cases per default. Same goes for Oracle.
rebased |
rebased |
*/ | ||
public function getBinaryMaxLength() | ||
{ | ||
return 4000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deeky666 can we add a comment on why this is 4000?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol awesome picture ;). I don't know why this is the default in AbstractPlatform
guess it's legacy. It's wrongly used throughout the platforms (adopted it from varchar implementation). 4000 is the default for Oracle maybe that's the reason ^^
This PR introduces a
BinaryType
as an equivalent toStringType
for binary strings of varying and fixed length as an alternative toBlobType
for smaller data requirements. Most of the vendors have native support forBINARY
andVARBINARY
types which get mapped to this type. CurrentlySqlite
andPostgreSQL
don't support them and mapping falls back to their nativeBLOB
types whenBinaryType
is used in the application.Please not that the added methods
AbstractPlatform::getBinaryMaxLength()
andAbstractPlatform::getBinaryDefaultLength()
are not used consistently throughout the platforms. I decided to adopt each platform's behaviour concerning default and max lengths evaluation if favour of implementing it the "correct" way. The reason for this is that those values are not properly defined and used throughout the platform and I guess changing this behaviour would be a BC break.An example to illustrate what I mean:
Most of the vendors use a default length for
VARCHAR
/CHAR
/VARBINARY
/BINARY
of1
when you don't provide a length specifier in the declaration. However most of the platforms use a default of255
and there are even many places where this value is hardcoded. At least this is my understanding of the default value.