-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Fix applying target column options on foreign key columns #6833
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix applying target column options on foreign key columns #6833
Conversation
private function gatherColumnOptions(array &$options, array $mapping) | ||
{ | ||
if (isset($mapping['options'])) { | ||
$knownOptions = array('comment', 'unsigned', 'fixed', 'default'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it makes sense to copy the column comment from the PK to the foreign key. So that should probably be excluded, but makes it kinda more ugly.
@@ -34,7 +34,7 @@ public function __construct(array $resultSet) | |||
* | |||
* @return array | |||
*/ | |||
public function fetchAll($fetchStyle = null, $columnIndex = null, array $ctorArgs = null) | |||
public function fetchAll($fetchStyle = null, $columnIndex = null, $ctorArgs = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why was all those unrelated stuff changed?
@@ -9,7 +9,7 @@ | |||
class ForumCategory | |||
{ | |||
/** | |||
* @Column(type="integer") | |||
* @Column(type="guid", options={"fixed":true, "collation":"latin1_bin", "foo":"bar"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
guid might not be a string so applying fixed or collation might not make sense in this case. maybe just use a string simply.
Fixing it generically makes sense 👍 |
@mrxotey ping |
@mrxotey @Tobion as mentioned on our contributions guidelines we accept PRs only to |
I'll close this PR as |
Sorry. Was busy. I'll create new PR with review fixes. Thanks. |
Fixes doctrine/dbal#2811
Similar PR: #6830
Tobion's PR fixes only applying of collation, which is not enough. This fix applies all options from target column to make type of source and target columns equal.
P.S.
Sorry for my terrible english.