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

Diff ignores default length #3490

Closed
SunMar opened this issue Mar 12, 2019 · 7 comments
Closed

Diff ignores default length #3490

SunMar opened this issue Mar 12, 2019 · 7 comments
Assignees

Comments

@SunMar
Copy link

SunMar commented Mar 12, 2019

Hi,

For Doctrine I've created a custom type that extends Doctrine's StringType. This custom type overrides getDefaultLength() to provide a default length for the type. In my mapping I have not explicitly specified a length.

When I run a database migration (against PostgreSQL 10.4) using $schema->createTable('table')->addColumn('column', 'my_string_type'); this works, the column gets created with the proper length. However now when I run doctrine:migrations:diff it creates an alter again for that column (and running that migration doesn't change it, diff always creates an ALTER).

I've done some debugging and found out that the driver reads the mapping, and because the mapping doesn't explicitly mention a length the fieldMappings for the column also don't get a length property. Then we you run the diff it always generates a file with:

ALTER TABLE table ALTER column TYPE VARCHAR(xx) even though the column already is VARCHAR(xx).

It seems like there's a step missing that populates fieldMappings with type defaults (either for custom or built-in types).

@jwage jwage transferred this issue from doctrine/DoctrineMigrationsBundle Mar 17, 2019
@morozov
Copy link
Member

morozov commented Mar 17, 2019

Is this the getDefaultType() which was deprecated in #3256?

@SunMar
Copy link
Author

SunMar commented Mar 21, 2019

@morozov Yes, that one. Good to know that it's deprecated, I didn't know that.

It seems like getDefaultType() wasn't deprecated for StringType, but I assume that's an omission. It has a docblock with only {@inheritDoc}, however that breaks with the PHPDoc Standard (see the red "Important" block). As such PhpStorm did not auto-detect for me that getDefaultType() was deprecated, since I was extending from StringType and not from Type. Then my inspections did not throw any warnings about implementing a deprecated method.

Also the @deprecated tag is not inherited automatically, which makes sense, because just because a subclass has deprecated a method does not mean your class also deprecates the method.

@SunMar
Copy link
Author

SunMar commented Mar 21, 2019

Aside from the phpdoc, is there a way to specify bounds for types? Not just for strings, but also for numbers? It would be nice if I could specify default bounds for my custom types, and then when Doctrine generates the migrations it'll determine based on the platform what type should be used.

The point is that I would like to be as storage type agnostic as possible. If my custom type is an integer type with a max range between 0 and 200, then it would be nice if Doctrine could figure out it should use an TINYINT UNSIGNED for that in MySQL for example. The same goes for string types, I would just like to be able to specify the max variable length (or that it's a fixed length of X), and then Doctrine can figure out what the best storage type would be that fits that.

@SunMar
Copy link
Author

SunMar commented Mar 21, 2019

I just checked my code and aside from getDefaultLength() I have also implemented:

    public function getSQLDeclaration(array $fieldDeclaration, AbstractPlatform $platform): string
    {
        $fieldDeclaration['length'] = 50;

        return parent::getSQLDeclaration($fieldDeclaration, $platform);
    }

But I guess that won't work if the diff works based on the XML input, since it won't be looking at the SQL declaration. I also don't see a way to somehow specify default options like length in Type.

My use case is that my domain entities work with value objects. Each value object has a custom doctrine type associated with it, so that doctrine can convert to/from the value object to/from the database. A value object will also have bounds associated with it, so that inside the domain everything is consistent. If you have a value object A, its bounds will always be between X and Y. You can't have one entity put a range between X and Y, and then have another entity use the same value object but with a range between X and Z. Then you wouldn't be able to rely on the value object behaving in the same way everywhere. So basically in my domain I never work with primitives, always with value objects. And specifying a length every time value object A is used, then becomes boilerplate that I would prefer to keep out and just define in the custom type.

@morozov
Copy link
Member

morozov commented Mar 22, 2019

@SunMar thank you for the clarification on the phpDoc issue. Would you mind filing a separate one so that we could fix it across the codebase?

As for the types themselves, I'm not familiar with that area, so I personally can't help you. But I hope someone else will.

@SunMar
Copy link
Author

SunMar commented Mar 22, 2019

@morozov Thank you, I'll use some other support channels to see if I can get an answer for this, and if it's not possible create a new feature request without the noise of a deprecated method and phpdoc issues :).

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants