Skip to content

Conversation

@Majkl578
Copy link
Contributor

@Majkl578 Majkl578 commented Aug 17, 2018

Q A
Type improvement
BC Break yes

Summary

Not used by DBAL itself, only overridden by StringType. Some other types (DateIntervalType) hardcode limits in getSQLDeclaration() directly.

1st step towards #2841 (Refactoring type system).

@Majkl578 Majkl578 force-pushed the dev/removal/Type-getDefaultLength branch from 0aad067 to 1306c3d Compare August 18, 2018 19:34
@morozov morozov self-assigned this Aug 19, 2018
Copy link
Member

@morozov morozov left a comment

Choose a reason for hiding this comment

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

Makes sense.

*/
public function getDefaultLength(AbstractPlatform $platform)
{
return $platform->getVarcharDefaultLength();
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.

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

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

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

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

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.

@Ocramius Ocramius assigned Ocramius and unassigned morozov Aug 19, 2018
@Ocramius Ocramius added this to the 3.0.0 milestone Aug 19, 2018
@Ocramius Ocramius merged commit 40c4f38 into doctrine:develop Aug 19, 2018
@Majkl578 Majkl578 deleted the dev/removal/Type-getDefaultLength branch August 19, 2018 17:25
@morozov morozov mentioned this pull request Sep 21, 2020
@morozov morozov modified the milestones: 4.0.0, 3.0.0 Nov 17, 2020
beberlei added a commit to beberlei/dbal that referenced this pull request Mar 7, 2021
beberlei added a commit to beberlei/dbal that referenced this pull request Mar 7, 2021
beberlei added a commit to beberlei/dbal that referenced this pull request Mar 11, 2021
beberlei added a commit to beberlei/dbal that referenced this pull request Mar 11, 2021
beberlei added a commit to beberlei/dbal that referenced this pull request Mar 22, 2021
beberlei added a commit to beberlei/dbal that referenced this pull request Mar 27, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants