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

EnumDropCommentCommand does not work on MySQL #197

Open
AlexandruGG opened this issue Jun 30, 2021 · 4 comments
Open

EnumDropCommentCommand does not work on MySQL #197

AlexandruGG opened this issue Jun 30, 2021 · 4 comments
Assignees
Labels

Comments

@AlexandruGG
Copy link
Contributor

Hello!

I'm trying the new addition command to drop comments mentioned here: https://github.com/fre5h/DoctrineEnumBundle/blob/main/Resources/docs/hook_for_doctrine_migrations.md#console-command-for-dropping-comments. However, it doesn't look like the SQL generated is valid for MySQL (I'm using 5.7 but I suspect the same holds true for 8.0).

[ERROR] An exception occurred while executing 'COMMENT ON COLUMN table_name.column_name IS ''':

         SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual
         that corresponds to your MySQL server version for the right syntax to use near 'COMMENT ON COLUMN
         table_name.column_name IS ''' at line 1

I can see this SQL is generated by Doctrine here so it's a bit unclear to me why the correct statement is not used. The correct statement would be something on the lines of:

ALTER TABLE table_name MODIFY COLUMN ENUM(...[current values]) NOT NULL COMMENT ''

Is there something wrong in my setup or what could be the issue here?

@fre5h fre5h self-assigned this Jul 30, 2021
@fre5h fre5h added the question label Jul 30, 2021
@fre5h
Copy link
Owner

fre5h commented Jul 30, 2021

Hi. Sorry for the late answer. Looks like it is a bug in Doctrine.

My console command gets platform from the config

  $connection = $this->em->getConnection();
  
  $platform = $connection->getDatabasePlatform();
  if (!$platform instanceof AbstractPlatform) {
      throw new RuntimeException('Missing database platform for connection.', 3);
  }

So the SQL statement comes from Doctrine

And then

  $sql = $platform->getCommentOnColumnSQL($tableName, $fieldMappingDetails['columnName'], null);
  $connection->executeQuery($sql);

@AlexandruGG
Copy link
Contributor Author

AlexandruGG commented Aug 5, 2021

@fre5h hey, I've looked into this some more.

I can see the statement comes from Doctrine - I also mention this in the description.

The issue here is that I don't think it's correct to use AbstractPlatform::getCommentOnColumnSQL for MySQL. If you look at where it's used you'll notice MySQLPlatform is not in there:

Screenshot 2021-08-05 at 16 59 29

Even though getCommentOnColumnSQL is used for other databases, like in PostgreSqlPlatform, for MySQL it's not used and the protected AbstractPlatform::getColumnComment is used instead: https://github.com/doctrine/dbal/blob/3.1.x/src/Platforms/MySQLPlatform.php#L552.

It's not really your fault, looks like this is just another inconsistency in Doctrine 🤷‍♂️ .
But I think we should mention in the documentation that the doctrine:enum:drop-comment command will not work on MySQL.

Maybe the command could also have a check for the platform and fail early with an error message if used with MySQL. If you agree with this I can open a PR.

@craigh
Copy link

craigh commented Apr 26, 2022

I just discovered this bug for MariaDB also (which I know is essentially MySQL). but thought it was worth the mention. Any workaround?

@AlexandruGG
Copy link
Contributor Author

I just discovered this bug for MariaDB also (which I know is essentially MySQL). but thought it was worth the mention. Any workaround?

My workaround was to just write the migration query manually whenever an enum's values needed to change (which should not happen often ideally!).

For example if we have an enum BasketballPositionType: ENUM('PG', 'SG', 'SF', 'PF') and we want to add the C value:

ALTER TABLE players CHANGE position position ENUM('PG', 'SG', 'SF', 'PF', 'C') NOT NULL COMMENT '(DC2Type:BasketballPositionType)'

My other recommendation would be to get on PHP 8.1 and Doctrine >= 2.11 because it supports the new native PHP Enum type: https://www.doctrine-project.org/2022/01/11/orm-2.11.html.

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

No branches or pull requests

3 participants