Skip to content
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

Closed

Conversation

mrxotey
Copy link

@mrxotey mrxotey commented Nov 20, 2017

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.

private function gatherColumnOptions(array &$options, array $mapping)
{
if (isset($mapping['options'])) {
$knownOptions = array('comment', 'unsigned', 'fixed', 'default');
Copy link
Contributor

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)
Copy link
Contributor

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"})
Copy link
Contributor

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.

@Tobion
Copy link
Contributor

Tobion commented Nov 20, 2017

Fixing it generically makes sense 👍

@Tobion
Copy link
Contributor

Tobion commented Nov 27, 2017

@mrxotey ping

@lcobucci
Copy link
Member

@mrxotey @Tobion as mentioned on our contributions guidelines we accept PRs only to master and we handle the backporting if needed (and when possible). With that said, please let's apply the fixes on #6830.

@lcobucci
Copy link
Member

I'll close this PR as invalid as per my comment.

@lcobucci lcobucci closed this Nov 27, 2017
@lcobucci lcobucci self-assigned this Nov 27, 2017
@mrxotey
Copy link
Author

mrxotey commented Dec 5, 2017

Sorry. Was busy. I'll create new PR with review fixes. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants