|
[
Permalink
| « Hide
]
Nico Kaiser added a comment - 08/Dec/09 04:49 PM
This patch implements columnDefinition into current Doctrine 2 trunk
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. 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) Updated patch with fixed XmlSchema and sqlite, and additional unit tests.
I think using columnDefinition people should be left with any consequences...
So do I. If you're using columnDefinition, you have to know what you do...
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?
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...
@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. Can you explain the Auto Increment Line in the Mysql Platform? I thought column definition was for that, defining a column definition completely
I am a bit worried about the following:
- $typeDecl = $field['type']->getSqlDeclaration($field, $this); + if (isset($field['columnDefinition'])) { + $typeDecl = $this->getCustomTypeDeclarationSql($field); + } else { + $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; } 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? 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? 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.
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...
@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.
OK. Talked in IRC. Release scheduled for coming friday and this issue is expected to be resolved until then.
Implemented - Can you test it and give some feedback Nico?
Btw, I found a way to pass the definition to the join column @Benjamin: Good work! Btw. you accidentily committed several *.php.orig files, presumably remainders from conflicts during merging.
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. Ah ok, this is indeed an issue that needs to be adressed.
Damn the .orig files, i'll remove them tonight. 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!
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Improvement
Resolved
Major
Support for "columnDefinition" in @Column
