Skip to content

DDC-200: Support for "columnDefinition" in @Column #2677

Closed
doctrinebot opened this Issue Dec 8, 2009 · 26 comments

2 participants

@doctrinebot

Jira issue originally created by user nicokaiser:

Support for columnDefinition (The SQL fragment that is used when generating the DDL for the column).

     /****
      * @Column(name="name", type="string", columnDefinition="CHAR(32)")
      */
     public $name;

See this mail for details:
http://groups.google.com/group/doctrine-user/browse_thread/thread/2cf813c72e4f15a8

@doctrinebot

Comment created by nicokaiser:

This patch implements columnDefinition into current Doctrine 2 trunk

@doctrinebot

Comment created by romanb:

That patch looks pretty good, except the (array) cast in the XmlDriver and that there are no tests :)

Would be great if you could enhance the patch with some tests.

Either way I'm pretty sure this will be included at some point before beta.

@doctrinebot

Comment created by nicokaiser:

There is one glitch - if you actually use this on a primary key, this may break associations with auto-generated foreign keys (as Doctrine does not know what type the PK acutally is (it may be an Integer for Doctrine, but a "mediumint(8)" in the columnDefinition), the foreign key gets the wrong type, which throws a PDOException...

The question is, how to handle these cases. Do you have a hint how to easily pass the type to to-be-created foreign keys (I think this is done by ALTER TABLEs later)? Or would this get too complicated and we should just leave this as it is?

(The JPA specification is no too detailed about that)

@doctrinebot

Comment created by nicokaiser:

Updated patch with fixed XmlSchema and sqlite, and additional unit tests.

@doctrinebot

Comment created by @beberlei:

I think using columnDefinition people should be left with any consequences...

@doctrinebot

Comment created by nicokaiser:

So do I. If you're using columnDefinition, you have to know what you do...

@doctrinebot

Comment created by romanb:

The foreign keys (JoinColumns) get the same type as the column they reference anyway or not? In that case could they not also just be given the same column definition if columnDefinition is used in the targeted column?

@doctrinebot

Comment created by nicokaiser:

Yes, I think this would solve the problem. However I did not yet find a way to implement this, as I did not dig far enough into the internals of UnitOfWork etc...

@doctrinebot

Comment created by romanb:

@Benjamin: You can merge this in. This mainly affects DBAL components so you will have an easier time resolving conflicts :)
Can you also look at whether the columnDefinition can not be simply taken over to the join columns if it is defined? If thats not easily possible it doesnt matter.

@doctrinebot

Comment created by @beberlei:

Can you explain the Auto Increment Line in the Mysql Platform? I thought column definition was for that, defining a column definition completely :-)

@doctrinebot

Comment created by @beberlei:

I am a bit worried about the following:

-        $typeDecl = $field['type']->getSqlDeclaration($field, $this);
<ins>        if (isset($field['columnDefinition'])) {
</ins>            $typeDecl = $this->getCustomTypeDeclarationSql($field);
<ins>        } else {
</ins>            $typeDecl = $field['type']->getSqlDeclaration($field, $this);
+        }

         return $name . ' ' . $typeDecl . $charset . $default . $notnull . $unique . $check . $collation;

The columnDefinition only allows to set $typeDecl in this case. Wouldn't it be better to make this:

        if (isset($field['columnDefinition'])) {
            $typeDecl = $this->getCustomTypeDeclarationSql($field);
             return $name . ' ' .$typeDecl;
        } else {
            $typeDecl = $field['type']->getSqlDeclaration($field, $this);
             return $name . ' ' . $typeDecl . $charset . $default . $notnull . $unique . $check . $collation;
        }
@doctrinebot

Comment created by nicokaiser:

Hm, I'm not sure about this - the Auto Increment line and the code you mention still allow the user to use autoincrement, notnull, etc. declarations from annotations and still use a customDefinition. I.e. you can use something like

@Column(name="name", type="string", columnDefinition="CHAR(32)")

and Doctrine will add a "NOT NULL" - as it is the default in Doctrine and it would be also added if there was no columnDefinition.

I did not find more details in the JPA specification or in its various implementation on what exactly the columnDefinition is used for - in my example only the type declaration would be replaced, in your example the whole definition would be replaced.

Not needing to specify things like auto increment in the custom columnDefinition would make it easier to automatically create foreign keys: e.g. if an ID field is created as "MEDIUMINT", this declaration can be copied to foreign key definitions that reference this field (see my second comment). If the ID's columnDefinition field includes AUTO_INCREMENT, this would be much harder...

What do you think?

@doctrinebot

Comment created by romanb:

I think it would indeed be better and easier if columnDefinition replaced the complete column definition in the DDL, not just the type part.

But what we need additionally is the columnDefinition on @JoinColumn. Indeed this is even in the spec. So it is the responsibility of the user to use the same columnDefinition in a @JoinColumn as in the referenced @Column in the case of foreign keys.

I think this would be the easiest, most transparent and least error-prone solution. So columnDefinition for both, @Column and @JoinColumn and it always replaces the complete definition.

Your thoughts?

@doctrinebot

Comment created by @beberlei:

sounds like a good solution, however I have to look at the code but it could be easy to implement that column passes the definition to join column automatically without any problems.

@doctrinebot

Comment created by nicokaiser:

Sounds good! My concerns for the complete column defintion was that it would be hard to define join columns then. But if this can be easily implemented, I don't see a problem here. And as I said, I did not find an exact specification about this, so I guess it's ok to hand the full responsibility to the user of columnDefinition...

@doctrinebot

Comment created by romanb:

@Benjamin Should I postpone this to BETA1? ALPHA4 will roll out this week, so if you cant find the time in the next days we can just push this back to the next release.

@doctrinebot

Comment created by romanb:

OK. Talked in IRC. Release scheduled for coming friday and this issue is expected to be resolved until then.

@doctrinebot

Comment created by nicokaiser:

Great, thanks!

@doctrinebot

Comment created by @beberlei:

Implemented - Can you test it and give some feedback Nico?

Btw, I found a way to pass the definition to the join column

@doctrinebot

Issue was closed with resolution "Fixed"

@doctrinebot

Comment created by romanb:

@Benjamin: Good work! Btw. you accidentily committed several *.php.orig files, presumably remainders from conflicts during merging.

@doctrinebot

Comment created by romanb:

I see that you left out columnDefinition from @JoinColumn. Is it not needed anymore? I think it would still be very useful. Even more, looking at your testcase, one common issue with the current behavior of taking the definition over is NOT NULL. Join columns / foreign keys are often supposed to be nullable, contrary to the primary key they are referring to.

So I think columnDefinition for @JoinColumn is needed.

@doctrinebot

Comment created by @beberlei:

Ah ok, this is indeed an issue that needs to be adressed.

Damn the .orig files, i'll remove them tonight.

@doctrinebot

Comment created by nicokaiser:

Looks great! I successfully ran our tests and even changed an Integer primarykey to "mediumint", so passing the definition to the join column seems to work! Thanks for integrating!

@beberlei beberlei was assigned by doctrinebot Dec 6, 2015
@doctrinebot doctrinebot added this to the 2.0-ALPHA4 milestone Dec 6, 2015
@doctrinebot doctrinebot closed this Dec 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.