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

Schema diff keeps thinking custom mapping types have changed #2596

Closed
vicdelfant opened this issue Jan 11, 2017 · 19 comments
Closed

Schema diff keeps thinking custom mapping types have changed #2596

vicdelfant opened this issue Jan 11, 2017 · 19 comments

Comments

@vicdelfant
Copy link

I'm working on a relatively large project with a lot of custom mapping types (primarily value objects with UUIDs such as OrderId, InvoiceId etc.). For some reason, schema:update insists on re-creating all columns that contain such a custom type:

$ bin/console doctrine:schema:update --dump-sql
ALTER TABLE orders_order CHANGE order_id order_id CHAR(36) NOT NULL COMMENT '(DC2Type:orders.order_id)';
# ...

$ bin/console doctrine:schema:update --force
Updating database schema...
Database schema updated successfully! "33" queries were executed

$ bin/console doctrine:schema:update --dump-sql
ALTER TABLE orders_order CHANGE order_id order_id CHAR(36) NOT NULL COMMENT '(DC2Type:orders.order_id)';
# ...

Mapping types are properly registered, requiresSQLCommentHint is set to return true for all types, the DC2Type comment is correctly being added to the field definition (confirmed in the MySQL tables), storing and retrieving data works perfectly... everything but the schema comparison.

The types are registered through a dedicated service, but in a nutshell the process is as follows:

foreach ($types as $name => $class) {
    Type::addType($name, $class);

    $platform->registerDoctrineTypeMapping($name, $name);

    // Added explicit call to markDoctrineTypeCommented(), makes no difference
    $platform->markDoctrineTypeCommented($name);
}

AbstractPlatform::$doctrineTypeComments contains all types so they're being processed just fine. Re-creating the entire database doesn't help either, and this obviously also affects migrations and schema:validate.

What am I missing here?

I came across the following similar issues but none of them solved my issue:

@Ocramius
Copy link
Member

Possibly strictly related to #2594

@vicdelfant
Copy link
Author

I'll try to find some time to write a test case so you can reproduce the issue more easily.

@deeky666
Copy link
Member

@vicdelfant do you derive that particular custom type from a built-in concrete Doctrine type like StringType? We have some special comparisons for some types which could affect derived custom types. Maybe you can provide the code of that custom type? That would help a lot. Thanks.

@vicdelfant
Copy link
Author

It's not derived from a built-in type, but the type class does extend quite a few of our own types before extending Doctrine\DBAL\Types\Type. The simplified OrderId and OrderIdType classes can be found at https://github.com/vicdelfant/doctrine-issue-2596, so it's basically comparable to https://github.com/ramsey/uuid-doctrine.

I our actual project, however, the inheritance chain is as follows:

  1. ProjectName\OrderIdType
  2. ProjectName\AbstractGuidType
  3. ProjectName\AbstractStringType
  4. ProjectName\AbstractScalarType
  5. Doctrine\DBAL\Types\Type

As you can see, we love inheritance 😉 If you suspect inheritance to be the culprit then let me know and I'll update the test repo so you can get a complete picture.

@deeky666
Copy link
Member

deeky666 commented Jan 17, 2017

@vicdelfant I think your problem is this line. It won't match orders.order_id and thus not recognize that there is a custom type declaration in the comment. A workaround might be to choose another name for now. Not sure if we can widen the list of allowed characters. @Ocramius what do you think?

@vicdelfant
Copy link
Author

Ah! Adding a \. to that regex indeed solves the issue for all OrderIdType-like types.

It appears that there's a second issue with a custom type inheriting from DateTimeTzType though; the comment field does seem to get through to the database schema, even with requiresSQLCommentHint set to true. I guess that's what you meant in your earlier comment? The custom type doesn't do anything apart from casting it to a Carbon instance.

@deeky666
Copy link
Member

@vicdelfant I don't quite understand the second issue. Isn't it expected behaviour that a column comment is created in the database if requiresSQLCommentHint is set to true?

@vicdelfant
Copy link
Author

Yes, exactly, that's the weird thing... there's no DC2Type-comment, even with requiresSQLCommentHint set to true.

@deeky666
Copy link
Member

Maybe related to #2594?

@vicdelfant
Copy link
Author

I'll check against master and report back, thanks!

@Sharom
Copy link

Sharom commented Nov 17, 2017

I have the same problem when type is a child of StringType.

Type code example:

final class PhoneNumberType extends StringType
{
    const NAME = 'phone_number';

    public function getName()
    {
        return self::NAME;
    }

    public function getSQLDeclaration(array $fieldDeclaration, AbstractPlatform $platform)
    {
        return parent::getSQLDeclaration(
            array_merge(
                $fieldDeclaration,
                [
                    'length' => 16,
                    'fixed' => false
                ]
            ),
            $platform
        );
    }

    public function convertToDatabaseValue($value, AbstractPlatform $platform)
    {
        if ($value === null) {
            return $value;
        }

        if (!$value instanceof PhoneNumber) {
            throw new \InvalidArgumentException(sprintf(
                'Value must be of type "%s" but "%s" was given.',
                PhoneNumber::class,
                is_object($value) ? get_class($value) : gettype($value)
            ));
        }

        return $value->getRawValue();
    }

    public function convertToPHPValue($value, AbstractPlatform $platform)
    {
        if ($value === null) {
            return $value;
        }

        return new PhoneNumber($value);
    }

    public function requiresSQLCommentHint(AbstractPlatform $platform)
    {
        return true;
    }
}

lcobucci pushed a commit to jeremy-smith-maco/dbal that referenced this issue Nov 19, 2017
lcobucci pushed a commit to lcobucci/dbal that referenced this issue Nov 19, 2017
@lcobucci lcobucci self-assigned this Nov 19, 2017
@lcobucci lcobucci added this to the 2.6.3 milestone Nov 19, 2017
@NaGeL182
Copy link

I'm still encountering this problem with doctrine/dbal v2.8.0 and beberlei/DoctrineExtensions v1.1.4 with carbondatetime type...

@ragboyjr
Copy link

@NaGeL182 or anyone that is experiencing an issue similar to this with doctrine and symfony:

I was experiencing this issue as well for a custom type that extended the StringType. I didn't update the getName method of my custom type and it was writing the type as string into the sql comment which was what was causing the issue. Making sure my getName represented the name of my custom type (channel_enum in my case), doctrine's mapping tool worked perfectly.

@NaGeL182
Copy link

@ragboyjr thanks for the info but I still encounter it even if my getName method is updated!

    const MONEYTYPE = 'money';
    //...

    public function getName()
    {
        return self::MONEYTYPE;
    }

and still encounter this error!

@vicdelfant
Copy link
Author

@ragboyjr / @NaGeL182 : did you override the requiresSQLCommentHint to return true? This is what was causing part of the issue in my case.

@NaGeL182
Copy link

@vicdelfant
I just set this:

    public function getName()
    {
        return self::MONEYTYPE;
    }

    public function requiresSQLCommentHint(AbstractPlatform $platform)
    {
        return true;
    }

    public function canRequireSQLConversion()
    {
        return true;
    }

and it still does it.

@NaGeL182
Copy link

hey, this seems to be fixed in doctrine/orm v2.6.2!
I don't get those diffs anymore! dunno what fixed it tho because the dbal is still at 2.8.0...

@KartaviK
Copy link

@vicdelfant
I just set this:

    public function getName()
    {
        return self::MONEYTYPE;
    }

    public function requiresSQLCommentHint(AbstractPlatform $platform)
    {
        return true;
    }

    public function canRequireSQLConversion()
    {
        return true;
    }

and it still does it.

I made value object as dbal type, and every field in mapping file represents as different every time. This overrides helps me to prevent creating same migrations for diff when mapping files same every time. thx

@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 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants