Doctrine DBAL
  1. Doctrine DBAL
  2. DBAL-647

MySqlPlatform's getCollationFieldDeclaration() looks like it has the wrong name

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Duplicate
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: Platforms
    • Security Level: All
    • Labels:
      None

      Description

      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.

        Activity

        Hide
        John Flatness added a comment -

        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.

        Show
        John Flatness added a comment - 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.
        Hide
        Steve Müller added a comment -

        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:

        https://github.com/doctrine/dbal/pull/274

        and

        https://github.com/doctrine/dbal/pull/245

        Partial support for SQL Server has already been merged in:

        https://github.com/doctrine/dbal/pull/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?

        Show
        Steve Müller added a comment - 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: https://github.com/doctrine/dbal/pull/274 and https://github.com/doctrine/dbal/pull/245 Partial support for SQL Server has already been merged in: https://github.com/doctrine/dbal/pull/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?
        Hide
        John Flatness added a comment -

        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.

        Show
        John Flatness added a comment - 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.
        Hide
        Steve Müller added a comment -

        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?

        Show
        Steve Müller added a comment - 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?
        Hide
        John Flatness added a comment -

        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.

        Show
        John Flatness added a comment - 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.
        Hide
        Steve Müller added a comment -

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

        Show
        Steve Müller added a comment - You are right it was not supposed to be AbstractPlatform but MySQLPlatform
        Hide
        Doctrine Bot added a comment -

        A related Github Pull-Request [GH-245] was closed:
        https://github.com/doctrine/dbal/pull/245

        Show
        Doctrine Bot added a comment - A related Github Pull-Request [GH-245] was closed: https://github.com/doctrine/dbal/pull/245

          People

          • Assignee:
            Benjamin Eberlei
            Reporter:
            John Flatness
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: