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

Doctrine SQL comments not always generated #4845

Closed
Mediagone opened this issue Oct 3, 2021 · 7 comments
Closed

Doctrine SQL comments not always generated #4845

Mediagone opened this issue Oct 3, 2021 · 7 comments

Comments

@Mediagone
Copy link

Bug Report

Q A
BC Break probably yes
Version ??

Summary

Despite implementing requiresSQLCommentHint() (and returning true) in custom type classes, the (DC2Type:) comment is not always added to the db.

Missing comments are causing problems like endless diff generation, etc.

How to reproduce

It happens when the type is declared with a different name from the one returned by the type's getName method.
eg.

final class CustomType extends Type
{
    public function getName() : string
    {
        return 'custom_type';
    }
    
    // other methods...
}

Type::addType('app_customtype', CustomType::class);

We can see the types are registered in the TypeRegistry using the supplied custom name:

    public function register(string $name, Type $type): void
    {
        if (isset($this->instances[$name])) {
            throw Exception::typeExists($name);
        }

        if ($this->findTypeName($type) !== null) {
            throw Exception::typeAlreadyRegistered($type);
        }

        $this->instances[$name] = $type;
    }

When looking in the AbstractPlatform, we can see that type's name (the one supplied to Type::register) is stored when the custom type requires a comment.

    protected function initializeCommentedDoctrineTypes()
    {
        $this->doctrineTypeComments = [];

        foreach (Type::getTypesMap() as $typeName => $className) {
            $type = Type::getType($typeName);

            if (! $type->requiresSQLCommentHint($this)) {
                continue;
            }

            $this->doctrineTypeComments[] = $typeName;
        }
    }

However, in the next method, we can see that the getName()'s name is used to check if the type requires an SQL comment, resulting in the class being not detected as commented :

    public function isCommentedDoctrineType(Type $doctrineType)
    {
        if ($this->doctrineTypeComments === null) {
            $this->initializeCommentedDoctrineTypes();
        }

        assert(is_array($this->doctrineTypeComments));

        return in_array($doctrineType->getName(), $this->doctrineTypeComments);
    }

I don't know enough Doctrine code to be sure, but maybe registering the type's class name instead of the type name would solve the issue.

Also, what are the purposes of this getName method if we can define the type name elsewhere ?

@morozov
Copy link
Member

morozov commented Oct 8, 2021

@Mediagone could you provide a script that reproduces the issue?

@Mediagone
Copy link
Author

Hi Morozov,

I created this small Symfony project to illustrate the bug : https://github.com/Mediagone/doctrine-custom-types-comment-bug

I created two custom Doctrine types, which are declared in config/doctrine.yaml :

  • SameType is declared with an identical name ("same") as returned in SameType::getName() method.
  • OtherType is declared with a different name ("different") as returned in OtherType::getName() method ("other").

The src/Entity/TestEntity declares properties of both types.

I generated a migration (with php bin/console doctrine:migrations:diff) that shows the problem : migrations/Version20211008083115.php (I added some notes inside to explain the result)

@morozov
Copy link
Member

morozov commented Oct 8, 2021

@Mediagone if you believe it's a bug in the DBAL, it should be reproducible without any dependencies. Otherwise, the problem can be anywhere else. You may want to seek help from others to convert your Symfony project to a reproducer.

@Mediagone
Copy link
Author

Mediagone commented Oct 10, 2021

Using symfony was just simpler since it enables quick type mapping, but it uses Doctrine's Type::register underneath (and related methods), to where I tracked down the problem (cf. my first post) !

It was also a way to integrate the doctrine-migrations package for SQL generation.

@morozov
Copy link
Member

morozov commented Dec 12, 2021

It looks like this issue duplicates #4983 and was fixed by #5077,

@Mediagone, please check on 3.2.x-dev.

@Mediagone
Copy link
Author

Indeed, the bug seems to be solved in 3.2.x-dev and SQL comments to be correctly added 👍

@morozov morozov added this to the 3.2.1 milestone Dec 19, 2021
@morozov morozov closed this as completed Dec 19, 2021
@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 21, 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

3 participants