[Cache] [PdoAdapter] Fix MySQL 1170 error (blob as primary key) #21323

Closed
wants to merge 2 commits into
from

Projects

None yet

4 participants

@akeeman
Contributor
akeeman commented Jan 17, 2017
Q A
Branch? 3.2
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? n/a
Fixed tickets n/a
License MIT
Doc PR n/a

When using a Doctrine\DBAL\Connection to initialize the PdoAdapter, PdoAdapter::createTable tries to create a table using a blob field as primary key. This is not an option for MySQL, resulting in an exeption: Syntax error or access violation: 1170 BLOB/TEXT column 'item_id' used in key specification without a key length. This commit supplies a way to act like the non-Connection way creates the table does.

@akeeman akeeman [Cache] [PdoAdapter] Fix MySQL 1170 error (blob as primary key)
Sync up with the non-Connection way of creating a table.
7575ee4
$schema = new Schema();
$table = $schema->createTable($this->table);
- $table->addColumn($this->idCol, 'blob', array('length' => 255));
+ $table->addColumn($this->idCol, $types[$this->driver], array('length' => 255));
@nicolas-grekas
nicolas-grekas Jan 17, 2017 Member

why not just 'mysql' === $this->driver ? 'binary' : 'blog'

@akeeman
akeeman Jan 17, 2017 Contributor

I indeed mentioned the MySQL case explicitly. However, the section below differs from the schema, in the sense that none of the statements there define the id column as blob (expecting you mean blob, not blog). Mapping it like this should result in an id column type that matches up with those below.

@akeeman
akeeman Jan 17, 2017 edited Contributor

I must say that I don't really know why sqlite uses text, but as you wrote it, I hope you do ;). In case this should be varchar too, then sqlite should also use 'string'. Then I would say that something like 'mysql' === $this->driver ? 'binary' : 'string' would be applicable, and I'll change the sqlite line too.

@nicolas-grekas
nicolas-grekas Jan 17, 2017 Member

I took inspiration from https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/PdoSessionHandler.php#L214

what matters is having a storage of 255 bytes, with collations/charset disabled

@akeeman
akeeman Jan 17, 2017 edited Contributor

Well, given that works I'd suggest to make the end result the same. Using the initially proposed mapping will result that, given doctrine's type mapping. Still I don't know why col type text is needed for sqlite, but probably I've got some catching up to do concerning that...

So, I now propose to keep it like my initial proposition. What about you?

@nicolas-grekas
nicolas-grekas Jan 17, 2017 Member

ok. could you please fix fabbot's failure? then we're done

@akeeman akeeman apply fabbot.io code style patch
f68d263
@fabpot
Member
fabpot commented Jan 17, 2017

Thank you @akeeman.

@fabpot fabpot added a commit that referenced this pull request Jan 17, 2017
@fabpot fabpot bug #21323 [Cache] [PdoAdapter] Fix MySQL 1170 error (blob as primary…
… key) (akeeman)

This PR was submitted for the master branch but it was merged into the 3.2 branch instead (closes #21323).

Discussion
----------

[Cache] [PdoAdapter] Fix MySQL 1170 error (blob as primary key)

| Q             | A
| ------------- | ---
| Branch?       | 3.2
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | n/a
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

When using a Doctrine\DBAL\Connection to initialize the PdoAdapter, PdoAdapter::createTable tries to create a table using a blob field as primary key. [This is not an option for MySQL](https://techjourney.net/mysql-error-1170-42000-blobtext-column-used-in-key-specification-without-a-key-length/), resulting in an exeption: `Syntax error or access violation: 1170 BLOB/TEXT column 'item_id' used in key specification without a key length`. This commit supplies a way to act like the non-Connection way creates the table does.

Commits
-------

08c6a65 [Cache] [PdoAdapter] Fix MySQL 1170 error (blob as primary key)
9d09059
@fabpot fabpot closed this Jan 17, 2017
@akeeman akeeman deleted the unknown repository branch Jan 18, 2017
@fabpot fabpot referenced this pull request Feb 6, 2017
Merged

Release v3.2.3 #21544

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