Skip to content

Loading…

DBAL-647: MySqlPlatform's getCollationFieldDeclaration() looks like it has the wrong name #1865

Closed
doctrinebot opened this Issue · 10 comments

2 participants

@doctrinebot

Jira issue originally created by user jflatnes:

The MySqlPlatform has a method getCollationFieldDeclaration(). As far as I can tell, that method's not actually used for anything.

However, looking at the other Platforms and the AbstractPlatform, it looks like this method is what is elsewhere called getColumnCollationDeclarationSQL().

The AbstractPlatform calls getColumnCollationDeclarationSQL() when getting the SQL for a column with a "collation" set; for MySQL, this currently causes no effect since the default, blank implementation from the AbstractPlatform is being used.

It looks like the name of this method should be changed.

@doctrinebot

Comment created by jflatnes:

The current name of the method looks like it dates back to the initial refactorings for Doctrine 2. No other methods there still use the "Field" terminology in the method name, and all the others that return snippets of SQL uniformly use "DeclarationSQL" instead of just "Declaration".

That combined with the different name for seemingly the same method in the AbstractPlatform and SQLServerPlatform makes it seem like this one just got left behind.

@doctrinebot

Comment created by @deeky666:

The reason for this diverge in implementation and terminology is that there still is an open PR that enables support for column collation declaration on capable platforms which is not yet merged:

#274

and

#245

Partial support for SQL Server has already been merged in:

#282

This feature is nearly finished and about to be merged. AbstractPlatform::getCollationFieldDeclaration() will be deprecated then to ensure BC. Does that answer your question? =) If so, can this ticket be closed?

@doctrinebot

Comment created by jflatnes:

I believe that does answer my question.

One little thing: when you say AbstractPlatform::getCollationFieldDeclaration() will be deprecated, you mean the one on MysqlPlatform, right? I believe there is no such method on the AbstractPlatform.

@doctrinebot

Comment created by @deeky666:

Yes it will be deprecated in MySQLPlatform. See the PR.
Can you tell me what is still unclear? Or what you'd expect of this ticket?

@doctrinebot

Comment created by jflatnes:

Okay, then I think it was just a typo in your first comment.

PR #245 on Github seems like it covers this squarely, so I suppose this issue doesn't stand for much on its own.

@doctrinebot

Issue was closed with resolution "Duplicate"

@doctrinebot

Comment created by @deeky666:

You are right it was not supposed to be AbstractPlatform but MySQLPlatform :)

@doctrinebot

Comment created by @doctrinebot:

A related Github Pull-Request [GH-245] was closed:
#245

@doctrinebot

Comment created by @doctrinebot:

A related Github Pull-Request [GH-274] was closed:
#274

@doctrinebot

Comment created by @doctrinebot:

A related Github Pull-Request [GH-274] was assigned:
#274

@doctrinebot doctrinebot added the Bug label
@beberlei beberlei was assigned by doctrinebot
@doctrinebot doctrinebot closed this
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.