[DBAL-120] MySql platform getAlterTableSQL does not quote existing column names Created: 13/May/11  Updated: 19/Jun/11  Resolved: 19/Jun/11

Status: Resolved
Project: Doctrine DBAL
Component/s: Platforms
Affects Version/s: 2.0.4
Fix Version/s: 2.0.4

Type: Bug Priority: Major
Reporter: Devon Weller Assignee: Benjamin Eberlei
Resolution: Duplicate Votes: 0
Labels: None

Attachments: Text File quote-mysql-change-colname.patch     Text File quote-mysql-change-keyname.patch    
Issue Links:
Duplicate
duplicates DBAL-96 Make approach towards identifier quot... Open

 Description   

When creating alter table SQL, the MySqlPlatform class does not quote names for columns of an existing table when building a CHANGE query. This can cause SQL errors.

It creates queries like this:

ALTER TABLE my_table CHANGE key `key` INTVARCHAR(255) DEFAULT '' NOT NULL';

Attached is a patch to fix this.

As an aside, is this the best way to contribute a patch? Or is a GitHub pull request better?



 Comments   
Comment by Devon Weller [ 13/May/11 ]

I also discovered a similar issue with key names. A second patch is attached.

Comment by Benjamin Eberlei [ 19/Jun/11 ]

This problem is a little more complicated, it will be fixed in DBAL-96 probably for 2.2





[DBAL-111] MySQL Driver possibly subject to sql injections with PDO::quote() Created: 18/Apr/11  Updated: 14/May/11  Resolved: 14/May/11

Status: Resolved
Project: Doctrine DBAL
Component/s: Drivers
Affects Version/s: 2.0.0-BETA2, 2.0.0-BETA3, 2.0.0-BETA4, 2.0.0-RC1-RC3, 2.0-RC4, 2.0-RC5, 2.0, 2.0.1, 2.0.2, 2.0.3, 2.0.4, 2.0.5, 2.1
Fix Version/s: 2.0.4, 2.0.5, 2.1

Type: Bug Priority: Critical
Reporter: Anthony Ferrara Assignee: Benjamin Eberlei
Resolution: Fixed Votes: 0
Labels: None
Environment:

MySQL Drivers



 Description   

Prior to 5.3.6, the MySQL PDO driver ignored the character set parameter to options. Due to MySQL's C api (and MySQLND), this is required for the proper function of mysql_real_escape_string() (the C API call). Since PDO uses the mres() C call for PDO::quote(), this means that the quoted string does not take into account the connection character set.

Starting with 5.3.6, that was fixed. So now if you pass the proper character set to PDO via driver options, sql injection is impossible while using the PDO::quote() api call.

PDO proof of concept
$dsn = 'mysql:dbname=INFORMATION_SCHEMA;host=127.0.0.1;charset=GBK;';
$pdo = new PDO($dsn, $user, $pass);
$pdo->exec('SET NAMES GBK');
$string = chr(0xbf) . chr(0x27) . ' OR 1 = 1; /*';
$sql = "SELECT TABLE_NAME
            FROM INFORMATION_SCHEMA.TABLES
            WHERE TABLE_NAME LIKE ".$pdo->quote($string)." LIMIT 1;";
$stmt = $pdo->query($sql);
var_dump($stmt->rowCount());

Expected Result: `int(0)`.
Actual Result: `int(1)`.

There are 2 issues to fix. First, the documentation does not indicate that you can pass the `charset` option to the MySQL Driver. This should be fixed so that users are given the proper option to set character sets.

Secondly, `Connection::setCharset()` should be modified for MySQL to throw an exception, since the character set is only safely setable using the DSN with PDO. This is a limitation of the driver and could be asked as a feature request for the PHP core. Either that, or a big warning should be put on the documentation of the API to indicate the unsafe character set change



 Comments   
Comment by Anthony Ferrara [ 19/Apr/11 ]

Note: issued same bug report for Doctrine1 as it's also affected: http://www.doctrine-project.org/jira/browse/DC-998

Comment by Anthony Ferrara [ 29/Apr/11 ]

Also note that prepared statements in PDO will suffer the same bug since PDO always emulates prepared statements for the mysql driver (even though it fully supports them in the source). See: http://bugs.php.net/bug.php?id=54638

Comment by Benjamin Eberlei [ 14/May/11 ]

Fixed, updated the docs





[DBAL-106] Doctrine\DBAL\Schema\Comparator false positives Created: 29/Mar/11  Updated: 06/Apr/11  Resolved: 06/Apr/11

Status: Resolved
Project: Doctrine DBAL
Component/s: Schema Managers
Affects Version/s: 2.0.2
Fix Version/s: 2.0.4

Type: Bug Priority: Major
Reporter: Lee Feistel Assignee: Benjamin Eberlei
Resolution: Fixed Votes: 1
Labels: None
Environment:

Doctrine 2.0.3, PHP 5.3.5, MySQL 5.5.9



 Description   

I am using the model class annotations for my Doctrine schema definition and a MySQL database for my target. When testing code that uses Doctrine\DBAL\Schema\Comparator, either by using $schemaTool->updateSchema($classes) which is defined in the core functionality or by running doctrine migrations:diff from the Doctrine Migrations project, a set of bugs are revealed. The symptoms of this are that immediately after calling $schemaTool->createSchema($classes) any call that uses the Comparator class to calculate the diffs between the real database and the schema definition produce a large set of false positives. Most of these false positives can be traced back to the logic in Doctrine\DBAL\Schema\Comparator::diffColumn(Column $column1, Column $column2). There are two issues here I can see right away:

One is that if the model annotations rely on defaults rather than explicitly defining certain attributes, this function sees them as different. For example, if I have a column defined like this: @Column(type="decimal", scale=2, nullable=true) then the Comparator believes it is different from the real database because precision is 10 in the real data and 0 in my schema. If I change the line to this: if (($column1->getPrecision() ?: 10) != ($column2->getPrecision() ?: 10)) then it works, but I am sure a real solution needs to determine what the defaults actually are.

The second issue is when I use the 'columnDefinition' attribute with a custom type to declare an enum or set, for instance. Since this function does not actually check what the columnDefinition field is set to at all, it has no way to know if I have actually made a change to the list of values, or anything else about that columnDefinition. I have been reading that using the comments field might be another way to do this, but I am not yet clear on what the best practice really is here. I know that http://www.doctrine-project.org/jira/browse/DBAL-42 touches on this with regard to storing Array type metadata in the comments field. I need more direction on how this should work in custom field types.

In addition, I am getting this strange set of diffs on the index for a ManyToMany association's join table:

        $this->addSql("CREATE INDEX IDX_44B8C6D89AF7860 ON firm_firm (firm_id)");
        $this->addSql("DROP INDEX primary ON firm_firm");
        $this->addSql("CREATE UNIQUE INDEX primary ON firm_firm (firm_id, firm_id)");

I am not sure why this only happens on this one table and not others, but it seems to be another case of the Comparator getting a false positive for some reason this time in Comparator::diffIndex(Index $index1, Index $index2). The model annotations for this association are:

  /**
   * @var Firm
   * @ManyToMany(targetEntity="Firm")
   */
  protected $Children = array();

I will add more to this as I learn more details about the issues, but hopefully this is enough to start grokking the issues in Comparator. Getting the Comparator class in the core Doctrine\DBAL library working properly for calculating diffs will go a long way toward making well defined migrations a reality for Doctrine2 projects running in production. Hopefully, someone will also be able to address this minor issue in the Doctrine Migrations (http://www.doctrine-project.org/jira/browse/DMIG-21) so that it can be deployed without needing a patch.

Thanks to everyone for Doctrine2, and I am looking forward to being among the first to put this new library into production on a real world project in a few months.



 Comments   
Comment by Benjamin Eberlei [ 06/Apr/11 ]

Can you tell me how the indexes on the join table look like?

Comment by Lee Feistel [ 06/Apr/11 ]

Now that I look at this schema, I think I need to apologize for my oversight because I didn't explain the problem properly. The real issue is that this self-referential ManyToMany association needs to have the id fields explicitly defined or it will not work. Doctrine2 is generating this table by apparently assuming both id fields have the same name, and so the second index that is created gets a randomly generated name assigned to it. There is no way this association is actually working properly at all, so I'll talk to the programmer that wrote this. I think we can fix this part of the problem ourselves, but perhaps some error checking here would be helpful. The first half of the problem (in diffColumn) remains. Thanks again for looking into this.

CREATE TABLE IF NOT EXISTS `firm_firm` (
`firm_id` int(11) NOT NULL,
PRIMARY KEY (`firm_id`),
KEY `IDX_44B8C6D89AF7860` (`firm_id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8;

ALTER TABLE `firm_firm`
ADD CONSTRAINT `firm_firm_ibfk_1` FOREIGN KEY (`firm_id`) REFERENCES `firm` (`id`) ON DELETE CASCADE;

Comment by Benjamin Eberlei [ 06/Apr/11 ]

Fixed the precision issue, please open a new ticket for the many to many.

Comment by Lee Feistel [ 06/Apr/11 ]

Thanks. At least the precision issue was easy, so glad to have that one out of the way. Is there any hope for a way to handle the use of columnDefiniton? I guess the alter statements being generated are just changing it to the same value, so it might not actually be broken in and of itself. Those statements could take a small amount of time to execute what is essentially a noop. Maybe we just have to manually go through and clean out the unwanted alter statements for the time being. It might be workable.

Comment by Benjamin Eberlei [ 06/Apr/11 ]

Updated version

Comment by Benjamin Eberlei [ 06/Apr/11 ]

columnDefinition always comes up in the diff. There is just no way around it. The problem is that there is no generic way to create a "columnDefinition" from the existing databse, so no good value to compare it too. If you can think of a solution for this problem i would be very happy, but we havent come up with one yet.





[DBAL-101] Undefined index "length" in SqliteSchemaManager Created: 17/Mar/11  Updated: 06/Apr/11  Resolved: 06/Apr/11

Status: Resolved
Project: Doctrine DBAL
Component/s: None
Affects Version/s: 2.1
Fix Version/s: 2.0.4

Type: Bug Priority: Major
Reporter: Karsten Dambekalns Assignee: Benjamin Eberlei
Resolution: Fixed Votes: 0
Labels: None

Attachments: Text File 0002-BUGFIX-Fix-undefined-index-error-on-SqliteSchemaMana.patch    

 Description   

If not giving length for numeric columns when using SQLite, I got an undefined index error around line 155 in SqliteSchemaManager. I fixed it with this:

             case 'numeric':
-                list($precision, $scale) = array_map('trim', explode(', ', $tableColumn['length']));
+                if (array_key_exists('length', $tableColumn)) {
+                    list($precision, $scale) = array_map('trim', explode(', ', $tableColumn['length']));
+                } else {
+                    $precision = $scale = null;
+                }
                 $length = null;
                 break;
 

Git patch attached.



 Comments   
Comment by Benjamin Eberlei [ 06/Apr/11 ]

Fixed





[DBAL-105] Schema Comparator produces invalid SQL Created: 29/Mar/11  Updated: 06/Apr/11  Resolved: 06/Apr/11

Status: Resolved
Project: Doctrine DBAL
Component/s: Schema Managers
Affects Version/s: 2.0.2
Fix Version/s: 2.0.4

Type: Bug Priority: Major
Reporter: Johannes Schmitt Assignee: Benjamin Eberlei
Resolution: Fixed Votes: 1
Labels: None
Environment:

Windows7, PHP 5.3.3



 Description   

Before:

/**
 * @orm:Entity
 * @orm:Table(name="twitter_users")
 */
class TwitterUser implements UserInterface
{
    const ROLE_DEFAULT = 'ROLE_TWITTER_USER';

    /**
     * @orm:Id
     * @orm:GeneratedValue(strategy="AUTO")
     * @orm:Column(type="integer")
     */
    private $id;

    /**
     * @orm:Column(type="integer", unique=true, nullable=false)
     */
    private $twitterId;

    /**
     * @orm:Column(type="string", unique=true, nullable=false)
     */
    private $displayName;
}

After:

/**
 * @orm:Entity
 * @orm:Table(name="twitter_users")
 */
class TwitterUser implements UserInterface
{
    const ROLE_DEFAULT = 'ROLE_TWITTER_USER';

    /**
     * @orm:Id
     * @orm:GeneratedValue(strategy="AUTO")
     * @orm:Column(type="integer")
     */
    private $id;

    /**
     * @orm:Column(type="integer", unique=true, nullable=false, name="twitter_id")
     */
    private $twitterId;

    /**
     * @orm:Column(type="string", unique=true, nullable=false, name="display_name")
     */
    private $displayName;

    /**
     * @orm:Column(type="datetime", name="last_logged_in_at", nullable=true)
     */
    private $lastLoggedInAt;
}

Bugs in the generated SQL:

  • last_logged_in_at column is not added
  • displayName is dropped, and at the same time the column name "displayName" is changed to "display_name"
  • twitterId same as above


 Comments   
Comment by Benjamin Eberlei [ 06/Apr/11 ]

Fixed





Generated at Wed Oct 01 04:15:14 UTC 2014 using JIRA 6.2.3#6260-sha1:63ef1d6dac3f4f4d7db4c1effd405ba38ccdc558.