Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7575ee4
[Cache] [PdoAdapter] Fix MySQL 1170 error (blob as primary key)
akeeman Jan 17, 2017
f68d263
apply fabbot.io code style patch
akeeman Jan 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion src/Symfony/Component/Cache/Adapter/PdoAdapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,20 @@ public function createTable()
$conn = $this->getConnection();

if ($conn instanceof Connection) {
$types = array(
'mysql' => 'binary',
'sqlite' => 'text',
'pgsql' => 'string',
'oci' => 'string',
'sqlsrv' => 'string',
);
if (!isset($types[$this->driver])) {
throw new \DomainException(sprintf('Creating the cache table is currently not implemented for PDO driver "%s".', $this->driver));
}

$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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@akeeman akeeman Jan 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@akeeman akeeman Jan 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

$table->addColumn($this->dataCol, 'blob', array('length' => 16777215));
$table->addColumn($this->lifetimeCol, 'integer', array('unsigned' => true, 'notnull' => false));
$table->addColumn($this->timeCol, 'integer', array('unsigned' => true, 'foo' => 'bar'));
Expand Down