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

Reduce column size to fit utf8mb4 index #87

Open
wants to merge 1 commit into
base: 3.x
Choose a base branch
from

Conversation

foaly-nr1
Copy link
Contributor

As you set an index on the name and ownerId properties of your Entity, creation of the tables fails in an utf8mb4 database:

[Doctrine\DBAL\Exception\DriverException]                                                                                                                                                                                         
  An exception occurred while executing 'CREATE TABLE dmishh_settings (id INT AUTO_INCREMENT NOT NULL, name VARCHAR(255) NOT NULL, value LONGTEXT DEFAULT NULL, owner_id VARCHAR(255) DEFAULT NULL, INDEX name_owner_id_idx (name,  
   owner_id), PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci ENGINE = InnoDB':                                                                                                                           
  SQLSTATE[HY000]: General error: 1709 Index column size too large. The maximum column size is 767 bytes.

The reason can be found in the MySQL 5.7 upgrade notes:

InnoDB has a maximum index length of 767 bytes for tables that use a COMPACT or REDUNDANT row format, so for utf8 or utf8mb4 columns, you can index a maximum of 255 or 191 characters, respectively. If you currently have utf8 columns with indexes longer than 191 characters, you will need to index a smaller number of characters.

This pull request reduces the column sizes to support utf8mb4.

@rvanlaak
Copy link
Owner

I think Doctrine should be the one to manage length of such fields, @Nyholm can you validate that we can just remove the length parameter of the annotation?

@Nyholm
Copy link
Collaborator

Nyholm commented May 26, 2016

It is a huge BC break to reduce the size of the field. We can not merge the PR as it is.
When using a plain old MySQL database utf8 we can simply remove the length parameter because MySQL (not Doctrine) will default it to 255. I do not know the behavior with utf8mb4. @foaly-nr1 please try to remove it and see if that solves your problem. If so, we are happy to merge this PR.

For the record: I believe Symfony best practice for 3rd party bundles states that we should not use annotations when defining the database mapping. If we use XML or Yaml one could override our mapping with their own.

@foaly-nr1
Copy link
Contributor Author

Let me check, that'd be an ideal solution.

@Nyholm I don't believe that's the case:

Due to the way Doctrine works, it is not possible to override entity mapping of a bundle. However, if a bundle provides a mapped superclass (such as the User entity in the FOSUserBundle) one can override attributes and associations. Learn more about this feature and its limitations in the Doctrine documentation.

http://symfony.com/doc/current/cookbook/bundles/override.html

@Nyholm
Copy link
Collaborator

Nyholm commented May 26, 2016

I don't believe that's the case:

Hm, interesting. I thought one could just override the mapping configuration file like you normally would. I'll have to investigate in that further then.. Thank you for the resource.

@foaly-nr1
Copy link
Contributor Author

Unlucky, Doctrine doesn't handle it correctly. I will open a ticket with them, but doubt they will want to move away from a simple default value.

@Nyholm
Copy link
Collaborator

Nyholm commented May 26, 2016

I think it's more of an issue with Symfony. Symfony should make sure to use the use the config file in app/Resources/OtherBundle/config/doctrine/mapping.yml instead of OtherBundle/config/doctrine/mapping.yml

@rvanlaak
Copy link
Owner

Also had problems with overriding the length of a field in another bundle, which is not possible because the configs are not merged. See craue/CraueConfigBundle#20

@foaly-nr1
Copy link
Contributor Author

I know we're already at 2.0.0-beta4 so this might be too late. But generally, we could sneak a breaking change into this major release?

@foaly-nr1
Copy link
Contributor Author

foaly-nr1 commented Dec 8, 2016

Ping @Nyholm breaking change in 2.0.0?

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

Successfully merging this pull request may close these issues.

None yet

3 participants