Skip to content

Remove Doctrine\DBAL\Types\Type::getDefaultLength() #3255

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 19, 2018
Merged

Remove Doctrine\DBAL\Types\Type::getDefaultLength() #3255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions UPGRADE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# Upgrade to 3.0

## BC BREAK: `Doctrine\DBAL\Types\Type::getDefaultLength()` removed

The `Doctrine\DBAL\Types\Type::getDefaultLength()` method has been removed as it served no purpose.

## BC BREAK: `Doctrine\DBAL\Types\Type::__toString()` removed

Relying on string representation was discouraged and has been removed.
Expand Down
8 changes: 0 additions & 8 deletions lib/Doctrine/DBAL/Types/StringType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,6 @@ public function getSQLDeclaration(array $fieldDeclaration, AbstractPlatform $pla
return $platform->getVarcharTypeDeclarationSQL($fieldDeclaration);
}

/**
* {@inheritdoc}
*/
public function getDefaultLength(AbstractPlatform $platform)
{
return $platform->getVarcharDefaultLength();
Copy link
Member

@morozov morozov Aug 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Speaking of which, why do we need to know the default type length on the platform level at all? E.g.:

dbal/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

Lines 298 to 300 in caf55ee

if ( !isset($field['length'])) {
$field['length'] = $this->getVarcharDefaultLength();
}

We're taking our hard-coded value and passing it explicitly to the field declaraion. Would it make sense to leave the length unspecified if the developer doesn't care? Otherwise, it leads to the pointless expctations like e2e0de2#diff-21fd51266275832f478e50a5b1f22748L219.

Copy link
Member

@Ocramius Ocramius Aug 19, 2018

Choose a reason for hiding this comment

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

Completely unspecified length is probably better: should be explicitly specified by developers anyway.

Copy link
Contributor Author

@Majkl578 Majkl578 Aug 19, 2018

Choose a reason for hiding this comment

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

How portalbe is that? From what I remember MySQL requires maximum length when declaring VARCHAR columns while PostgreSQL doesn't.

Copy link
Member

@morozov morozov Aug 19, 2018

Choose a reason for hiding this comment

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

There are two points here:

  1. If a platform doesn't require max length (PostgreSQL), DBAL shouldn't require max length either but it shouldn't provide any hard-coded length in DDL. It should omit it.
  2. If a platform does require MySQL, DBAL should require it in the column definition but it shouldn't provide any hard-coded length in DDL. It should fail.

This way, if you use only one platform (PostgreSQL) and don't care about portability, you can omit the length and use DBAL w/o any artificial restrictions. Otherwise, you have to explicitly specify the length. It's not the DBAL's job to specify it for you.

}

/**
* {@inheritdoc}
*/
Expand Down
14 changes: 0 additions & 14 deletions lib/Doctrine/DBAL/Types/Type.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,20 +122,6 @@ public function convertToPHPValue($value, AbstractPlatform $platform)
return $value;
}

/**
* Gets the default length of this type.
*
* @param \Doctrine\DBAL\Platforms\AbstractPlatform $platform
*
* @return int|null
*
* @todo Needed?
*/
public function getDefaultLength(AbstractPlatform $platform)
{
return null;
}

/**
* Gets the SQL declaration snippet for a field of this type.
*
Expand Down
5 changes: 0 additions & 5 deletions tests/Doctrine/Tests/DBAL/Types/StringTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,6 @@ public function testReturnsSqlDeclarationFromPlatformVarchar()
self::assertEquals("DUMMYVARCHAR()", $this->_type->getSqlDeclaration(array(), $this->_platform));
}

public function testReturnsDefaultLengthFromPlatformVarchar()
{
self::assertEquals(255, $this->_type->getDefaultLength($this->_platform));
}

public function testConvertToPHPValue()
{
self::assertInternalType("string", $this->_type->convertToPHPValue("foo", $this->_platform));
Expand Down