Doctrine 1
  1. Doctrine 1
  2. DC-324

Quote identifier breaks multi-column index sql generation

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Critical Critical
    • Resolution: Invalid
    • Affects Version/s: 1.2.0
    • Fix Version/s: None
    • Component/s: Connection
    • Labels:
      None

      Description

      By enabling quote identifier:

      Doctrine_Manager::getInstance()->setAttribute(
      Doctrine::ATTR_QUOTE_IDENTIFIER,
      true
      );

      If table has multi-column index, results are:

      CREATE INDEX name ON table ("column1, column2")

      expected result

      CREATE INDEX name ON table ("column1", "column2")

      I created a patch for this bug, which extends quoteIdentifer() method in connection to detect comma separated fields and escape them separately. It also supports any amount of fields (not just two). It's done in this method, and not in getIndexFieldDeclarationList() in export classes, because it would require a major refactoring for a lot of classes and methods inside different classes. Please review this issue, so I can commit.

      1. connection2.patch
        0.9 kB
        Juozas Kaziukenas

        Activity

        Hide
        Jonathan H. Wage added a comment -

        I don't think it should be done this way. It should be done specifically where it is needed, in getIndexFieldDeclarationList()

        Show
        Jonathan H. Wage added a comment - I don't think it should be done this way. It should be done specifically where it is needed, in getIndexFieldDeclarationList()
        Hide
        Jonathan H. Wage added a comment -

        Hmm. After looking at this, it doesn't seem right. The method getIndexFieldDeclarationList() is already implemented this way and each field is quoted individually. Let me see your schema where you define the index..

        Show
        Jonathan H. Wage added a comment - Hmm. After looking at this, it doesn't seem right. The method getIndexFieldDeclarationList() is already implemented this way and each field is quoted individually. Let me see your schema where you define the index..
        Hide
        Juozas Kaziukenas added a comment - - edited

        Ok, it might have been not right, but I made it like this, because this data was tunelling from definition to getIndexFieldDeclarationList() and at that point fields are already as list (comma separated), so I guess my fix worked more like a hack.

        Model looks something like this:

        <?php

        abstract class Model extends Doctrine_Record
        {
        public function setTableDefinition()
        {
        $this->setTableName('table');
        $this->hasColumn('userid', 'integer', 4, array(
        'type' => 'integer',
        'length' => 4,
        'fixed' => false,
        'unsigned' => false,
        'notnull' => true,
        'primary' => false,
        ));
        $this->hasColumn('moduleid', 'integer', 4, array(
        'type' => 'integer',
        'length' => 4,
        'fixed' => false,
        'unsigned' => false,
        'notnull' => true,
        'primary' => false,
        ));
        $this->hasColumn('privilegeid', 'string', null, array(
        'type' => 'string',
        'fixed' => true,
        'unsigned' => false,
        'notnull' => true,
        'primary' => false,
        ));
        }

        public function setUp()
        {
        parent::setUp();

        $this->hasOne('Model_Privileges as Privileges', array(
        'local' => 'privilegeid, moduleid',
        'foreign' => 'id, moduleid'));
        }
        }

        It is generated by doctrine task generate-models-db, so it is a generator issue then?

        Show
        Juozas Kaziukenas added a comment - - edited Ok, it might have been not right, but I made it like this, because this data was tunelling from definition to getIndexFieldDeclarationList() and at that point fields are already as list (comma separated), so I guess my fix worked more like a hack. Model looks something like this: <?php abstract class Model extends Doctrine_Record { public function setTableDefinition() { $this->setTableName('table'); $this->hasColumn('userid', 'integer', 4, array( 'type' => 'integer', 'length' => 4, 'fixed' => false, 'unsigned' => false, 'notnull' => true, 'primary' => false, )); $this->hasColumn('moduleid', 'integer', 4, array( 'type' => 'integer', 'length' => 4, 'fixed' => false, 'unsigned' => false, 'notnull' => true, 'primary' => false, )); $this->hasColumn('privilegeid', 'string', null, array( 'type' => 'string', 'fixed' => true, 'unsigned' => false, 'notnull' => true, 'primary' => false, )); } public function setUp() { parent::setUp(); $this->hasOne('Model_Privileges as Privileges', array( 'local' => 'privilegeid, moduleid', 'foreign' => 'id, moduleid')); } } It is generated by doctrine task generate-models-db, so it is a generator issue then?
        Hide
        Jonathan H. Wage added a comment -

        What database type?

        $this->hasOne('Model_Privileges as Privileges', array(
        'local' => 'privilegeid, moduleid',
        'foreign' => 'id, moduleid'));
        }
        

        This part is wrong. the comma separated list of fields is not supported and Doctrine doesn't support composite foreign keys. So it is a problem with the importer for the database you're using. Is it mssql?

        Show
        Jonathan H. Wage added a comment - What database type? $ this ->hasOne('Model_Privileges as Privileges', array( 'local' => 'privilegeid, moduleid', 'foreign' => 'id, moduleid')); } This part is wrong. the comma separated list of fields is not supported and Doctrine doesn't support composite foreign keys. So it is a problem with the importer for the database you're using. Is it mssql?
        Hide
        Juozas Kaziukenas added a comment -

        I was not aware that comma separated fields were invalid. My patch is invalid them and I need to fix generator.

        I'm working with pgsql, and will review the problem, propose a pach.

        Show
        Juozas Kaziukenas added a comment - I was not aware that comma separated fields were invalid. My patch is invalid them and I need to fix generator. I'm working with pgsql, and will review the problem, propose a pach.
        Hide
        Jonathan H. Wage added a comment -

        If composite foreign keys were fully supported in Doctrine 1 it would be with an array:

        $this->hasOne('Model_Privileges as Privileges', array(
        'local' => array('privilegeid', 'moduleid'),
        'foreign' => array('id', 'moduleid')));
        }
        
        Show
        Jonathan H. Wage added a comment - If composite foreign keys were fully supported in Doctrine 1 it would be with an array: $ this ->hasOne('Model_Privileges as Privileges', array( 'local' => array('privilegeid', 'moduleid'), 'foreign' => array('id', 'moduleid'))); }
        Hide
        Jonathan H. Wage added a comment -

        If you define it that way, then everything is generated fine.

        Show
        Jonathan H. Wage added a comment - If you define it that way, then everything is generated fine.

          People

          • Assignee:
            Jonathan H. Wage
            Reporter:
            Juozas Kaziukenas
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: