[DBAL-324] SchemaManager should first look into comment instead of infer the type first. Created: 17/Aug/12  Updated: 03/Jan/14

Status: Open
Project: Doctrine DBAL
Component/s: None
Affects Version/s: None
Fix Version/s: None
Security Level: All

Type: Bug Priority: Major
Reporter: Guilherme Blanco Assignee: Guilherme Blanco
Resolution: Unresolved Votes: 0
Labels: None


 Description   

When using schema tool, Doctrine tries to infer the Doctrine type via the mapped types in Platform.
It should first try to read from the comment; if found, ignore the Database type inference.



 Comments   
Comment by Benjamin Eberlei [ 29/Aug/12 ]

Why is this a bug? Can you say some more about why we need to do this and what error occurs?

Comment by Guilherme Blanco [ 29/Aug/12 ]

This issue is strictly correlated to the commit I've done here: https://github.com/doctrine/dbal/commit/e25c774dde971dc4afd40648e9ccd0af53b34ce9

Mainly, we may have legacy database that we do know how Doctrine should operate. Under this circumstance, we may want to add a comment to the field defining the Doctrine DBAL type.
Even though after doing this, Doctrine still complains (in my situation, a SET column type) that it's unable to handle this column type.

That happens because MySQL Schema Manager (and others) first looks for the column type:
https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Schema/MySqlSchemaManager.php#L112
https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php#L312
which automatically fails.

But if we first try to look for the commented data type:
https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Schema/AbstractSchemaManager.php#L884
And forget about the rest if it finds one definition there, it would allow even unknown data types to be managed instead of forcing to define the DBAL Type compatible.

Comment by Steve Tauber [ 14/May/13 ]

This also applies for a type of yaml.
Example:

/** @Column(type="yaml") */
protected $data = array();

./scripts/doctrine orm:schema-tool:update --dump-sql
ALTER TABLE meta CHANGE data data LONGTEXT NOT NULL;

Very frustrating.... Might be related to http://www.doctrine-project.org/jira/browse/DBAL-42

Comment by Steve Müller [ 03/Jan/14 ]

Steve Tauber You have defined a custom data type. You need to override `Type::requiresSQLCommentHint()` to return true in your custom data type, otherwise Doctrine cannot infer the custom type mapping from the column comment and the comparator will always detect changes between Yaml and LONGTEXT. This is not directly related to this issue here I think.
Guilherme Blanco I don't really get the issue you describe here. Is it maybe fixed already? Otherwise can you maybe try to explain again so that I can fix that?





[DBAL-867] Doctrine\DBAL\Driver\Connection should be marked as an internal interface Created: 16/Apr/14  Updated: 26/Jun/14

Status: Reopened
Project: Doctrine DBAL
Component/s: None
Affects Version/s: None
Fix Version/s: None
Security Level: All

Type: Documentation Priority: Major
Reporter: Christophe Coevoet Assignee: Guilherme Blanco
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Currently, the main entry point Doctrine\DBAL\Connection is documented as a wrapper around Doctrine\DBAL\Driver\Connection.
This is very misleading and encourages other library to typehint against Doctrine\DBAL\Driver\Connection rather than Doctrine\DBAL\Connection. See https://github.com/symfony/symfony/pull/10720 for the original discussion.

However, the discussion in https://github.com/doctrine/dbal/pull/414#discussion_r7688765 implies that they should actually not be related together (but it cannot be fixed for BC reasons). The phpdoc should at least be changed



 Comments   
Comment by Guilherme Blanco [ 21/Apr/14 ]

This issue was fixed some time ago.

Commit reference: https://github.com/doctrine/dbal/commit/5fdedc4f8f304e8035580807bd36d59e97a87265

Comment by Steve Müller [ 22/Apr/14 ]

Guilherme Blanco How is your commit reference related to this issue?

Comment by Guilherme Blanco [ 22/Apr/14 ]

Steve Müller The suggestion about Doctrine\DBAL\Connection and Doctrine\DBAL\Driver\Connection started around the misleading support of ping and how to consistently support it across different drivers.
The commit reference I pointed out is Benjamin Eberlei's resolution to remove misleading phpdoc around ping support (which is also highlighted in the ticket as "phpdoc should at least be changed").
If there's anything else that is missing, I'm probably not seeing. All I've done is followed dbal's discussion. =\

Comment by Christophe Coevoet [ 22/Apr/14 ]

Your commit does not fix it at all. Benjamin Eberlei's comment was about the ping method only indeed. But he explained that Doctrine\DBAL\Connection should actually not be a Doctrine\DBAL\Driver\Connection except for legacy reasons, which is why makign it implement Doctrine\DBAL\Driver\PingableConnection was a bad idea even if it has a ping method.

My issue is related to the description of the class itself: https://github.com/doctrine/dbal/blob/aa2ed45ade6582a24e4f72f674f6989873d72112/lib/Doctrine/DBAL/Connection.php#L36
It still describes it as a wrapper around the driver connection, making other devs think that the right typehint in other libraries is the internal driver connection. See the discussion in the Symfony PR I linked

Comment by Guilherme Blanco [ 22/Apr/14 ]

So... it's reopened. I'll look into this later today.

Comment by Marco Pivetta [ 26/Jun/14 ]

Guilherme Blanco can you re-check this?





Generated at Thu Aug 28 23:25:08 UTC 2014 using JIRA 6.2.3#6260-sha1:63ef1d6dac3f4f4d7db4c1effd405ba38ccdc558.