Doctrine 1
  1. Doctrine 1
  2. DC-708

Wrong definition for MySQL string primary column

    Details

    • Type: Bug Bug
    • Status: 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

        Hide
        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
        Hide
        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
        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
        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
        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
        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
        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
        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?

          People

          • Assignee:
            Jonathan H. Wage
            Reporter:
            Claudio Nicora
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated: