Doctrine 1

Wrong definition for MySQL string primary column

Details

  • Type: Bug Bug
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: 1.2.2
  • Fix Version/s: 1.2.2
  • Component/s: Import/Export
  • Labels:
    None
  • Environment:
    PHP 5.3.1, WinXP-SP3, Doctrine 1.2.2, MySQL 5.1.41

Description

If you define a primary column, the attribute notnull is removed from the column definition because Doctrine assumes that primary columns are always not null.

Now suppose you have a schema like this, with a string primary column.

Client:
  columns:
    serial:    { type: string(50), notnull: true, primary: true }
    name:      { type: string(36), notnull: true }

That's fine, but causes problems with MySQL where the column is created with a default value of "" (empty string) and not <none>.

CREATE TABLE client (serial VARCHAR(50), name VARCHAR(36) NOT NULL, PRIMARY KEY(serial)) ENGINE = INNODB;

Note that the 2nd column is well defined and has <none> as default value (as seen from phpMyAdmin).

I attached a quick-workaround to disable the code which removes the notnull attribute from column definition.

After that the SQL code is like this:

CREATE TABLE client (serial VARCHAR(50) NOT NULL, name VARCHAR(36) NOT NULL, PRIMARY KEY(serial)) ENGINE = INNODB;

Activity

Descending order - Click to sort in ascending order
Hide
Permalink
Claudio Nicora added a comment - - edited

Hi, I attached a patch against Export/Mysql.php instead of the previous against Import/Builder.php.
It works on my side; can you please test it with your test suite?

Show
Claudio Nicora added a comment - - edited Hi, I attached a patch against Export/Mysql.php instead of the previous against Import/Builder.php. It works on my side; can you please test it with your test suite?
Hide
Permalink
Jonathan H. Wage added a comment -

Hi, if you want to provide a patch that fixes your situation I can test it against our test suite and see if we can include it. You can also run your changes against the test suite to see if it causes any problems.

Show
Jonathan H. Wage added a comment - Hi, if you want to provide a patch that fixes your situation I can test it against our test suite and see if we can include it. You can also run your changes against the test suite to see if it causes any problems.
Hide
Permalink
Claudio Nicora added a comment -

That's what I was afraid of.
Maybe you should change the code that generates the SQL for MySQL to make it include the "NOT NULL" clause to primary keys.

Show
Claudio Nicora added a comment - That's what I was afraid of. Maybe you should change the code that generates the SQL for MySQL to make it include the "NOT NULL" clause to primary keys.
Hide
Permalink
Jonathan H. Wage added a comment -

We can't just remove the code. It will change the behavior of the builder drastically which breaks backwards compatibility.

Show
Jonathan H. Wage added a comment - We can't just remove the code. It will change the behavior of the builder drastically which breaks backwards compatibility.
Hide
Permalink
Claudio Nicora added a comment -

My patch only removed the effect, but it's not surely the best solution.

I've no sufficient knowledge on Doctrine to say that commenting out (or removing) that line will not affect other parts; that's why I only commented out the code instead of removing it (both on my side and on the attached patch).

If you think I'm not adding new bugs, I agree that removing the offending code is the cleanest way.

Show
Claudio Nicora added a comment - My patch only removed the effect, but it's not surely the best solution. I've no sufficient knowledge on Doctrine to say that commenting out (or removing) that line will not affect other parts; that's why I only commented out the code instead of removing it (both on my side and on the attached patch). If you think I'm not adding new bugs, I agree that removing the offending code is the cleanest way.
Hide
Permalink
Jonathan H. Wage added a comment -

The patch I see currently just comments out the offending code. Is that intended? It cannot be committed if so

Show
Jonathan H. Wage added a comment - The patch I see currently just comments out the offending code. Is that intended? It cannot be committed if so
Hide
Permalink
Claudio Nicora added a comment - - edited

The same behaviour happens even for integer columns, so the notnull attribute should never be removed, not only for string columns. New patch attached, old one removed.

Show
Claudio Nicora added a comment - - edited The same behaviour happens even for integer columns, so the notnull attribute should never be removed, not only for string columns. New patch attached, old one removed.
Hide
Permalink
Claudio Nicora added a comment - - edited

Attached a better patch where the notnull attribute is removed only if the primary column type is string

Show
Claudio Nicora added a comment - - edited Attached a better patch where the notnull attribute is removed only if the primary column type is string

People

  • Assignee:
    Jonathan H. Wage
    Reporter:
    Claudio Nicora
Vote (0)
Watch (1)

Dates

  • Created:
    Updated: