Skip to content

Commit

Permalink
feature #36655 Automatically provide Messenger Doctrine schema to "di…
Browse files Browse the repository at this point in the history
…ff" (weaverryan)

This PR was squashed before being merged into the 5.1-dev branch.

Discussion
----------

Automatically provide Messenger Doctrine schema to "diff"

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | Alternative to #36629
| License       | MIT
| Doc PR        | TODO - WILL be needed

This follows this conversation: symfony/symfony#36629 (comment) - it automatically adds SQL to Doctrine's migration/diff system when features are added the require a database table:

The new feature works for:

### A) Messenger Doctrine transport
**FULL support**
Works perfectly: configure a doctrine transport and run `make:migration`

**Note**: There is no current way to disable this. So if you have `auto_setup` ON and you
run `make:migration` before trying Messenger, it will generate the table SQL. Adding a
flag to disable it might be very complicated, because we need to know (in DoctrineBundle, at compile time) whether or not this feature is enabled/disabled so that we can decide *not* to add `messenger_messages` to the `schema_filter`.

### B) `PdoAdapter` from Cache
**FULL support**
Works perfectly: configure a doctrine transport and run `make:migration`

### C) `PdoStore` from Lock
**PARTIAL support**
I added `PdoStore::configureSchema()` but did NOT add a listener. While `PdoStore` *does* accept a DBAL `Connection`, I don't think it's possible via the `framework.lock` config to create a `PdoStore` that is passed a `Connection`. In other words: if we added a listener that called `PdoStore::configureSchema` if the user configured a `pdo` lock, that service will *never* have a `Connection` object... so it's kind of worthless.

**NEED**: A proper way to inject a DBAL `Connection` into `PdoStore` via `framework.lock` config.

### D) `PdoSessionHandler`
**NO support**

This class doesn't accept a DBAL `Connection` object. And so, we can't reliably create a listener to add the schema because (if there are multiple connections) we wouldn't know which Connection to use.

We could compare (`===`) the `PDO` instance inside `PdoSessionHandler` to the wrapped `PDO` connection in Doctrine. That would only work if the user has configured their `PdoSessionHandler` to re-use the Doctrine PDO connection.

The `PdoSessionHandler` *already* has a `createTable()` method on it to help with manual migration. But... it's not easy to call from a migration because you would need to fetch the `PdoSessionHandler` service from the container. Adding something

**NEED**: Either:

A) A way for `PdoSessionHandler` to use a DBAL Connection
or
B) We try to hack this feature by comparing the `PDO` instances in the event subscriber
or
C) We add an easier way to access the `createTable()` method from inside a migration.

TODOs

* [X] Determine service injection XML needed for getting all PdoAdapter pools
* [ ] Finish DoctrineBundle PR: doctrine/DoctrineBundle#1163

Commits
-------

2dd9c3c3c8 Automatically provide Messenger Doctrine schema to "diff"
  • Loading branch information
fabpot committed May 5, 2020
2 parents dc0f55f + b8beb3e commit 1b73c7b
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 6 deletions.
33 changes: 27 additions & 6 deletions Store/PdoStore.php
Expand Up @@ -146,7 +146,7 @@ public function save(Key $key)
public function putOffExpiration(Key $key, float $ttl)
{
if ($ttl < 1) {
throw new InvalidTtlException(sprintf('"%s()" expects a TTL greater or equals to 1 second. Got %s.', __METHOD__, $ttl));
throw new InvalidTtlException(sprintf('"%s()" expects a TTL greater or equals to 1 second. Got "%s".', __METHOD__, $ttl));
}

$key->reduceLifetime($ttl);
Expand Down Expand Up @@ -249,11 +249,7 @@ public function createTable(): void

if ($conn instanceof Connection) {
$schema = new Schema();
$table = $schema->createTable($this->table);
$table->addColumn($this->idCol, 'string', ['length' => 64]);
$table->addColumn($this->tokenCol, 'string', ['length' => 44]);
$table->addColumn($this->expirationCol, 'integer', ['unsigned' => true]);
$table->setPrimaryKey([$this->idCol]);
$this->addTableToSchema($schema);

foreach ($schema->toSql($conn->getDatabasePlatform()) as $sql) {
$conn->exec($sql);
Expand Down Expand Up @@ -285,6 +281,22 @@ public function createTable(): void
$conn->exec($sql);
}

/**
* Adds the Table to the Schema if it doesn't exist.
*/
public function configureSchema(Schema $schema): void
{
if (!$this->getConnection() instanceof Connection) {
throw new \BadMethodCallException(sprintf('"%s::%s()" is only supported when using a doctrine/dbal Connection.', __CLASS__, __METHOD__));
}

if ($schema->hasTable($this->table)) {
return;
}

$this->addTableToSchema($schema);
}

/**
* Cleans up the table by removing all expired locks.
*/
Expand Down Expand Up @@ -351,4 +363,13 @@ private function getCurrentTimestampStatement(): string
return time();
}
}

private function addTableToSchema(Schema $schema): void
{
$table = $schema->createTable($this->table);
$table->addColumn($this->idCol, 'string', ['length' => 64]);
$table->addColumn($this->tokenCol, 'string', ['length' => 44]);
$table->addColumn($this->expirationCol, 'integer', ['unsigned' => true]);
$table->setPrimaryKey([$this->idCol]);
}
}
10 changes: 10 additions & 0 deletions Tests/Store/PdoDbalStoreTest.php
Expand Up @@ -11,7 +11,9 @@

namespace Symfony\Component\Lock\Tests\Store;

use Doctrine\DBAL\Connection;
use Doctrine\DBAL\DriverManager;
use Doctrine\DBAL\Schema\Schema;
use Symfony\Component\Lock\PersistingStoreInterface;
use Symfony\Component\Lock\Store\PdoStore;

Expand Down Expand Up @@ -59,4 +61,12 @@ public function testAbortAfterExpiration()
{
$this->markTestSkipped('Pdo expects a TTL greater than 1 sec. Simulating a slow network is too hard');
}

public function testConfigureSchema()
{
$store = new PdoStore($this->createMock(Connection::class), ['db_table' => 'lock_table']);
$schema = new Schema();
$store->configureSchema($schema);
$this->assertTrue($schema->hasTable('lock_table'));
}
}

0 comments on commit 1b73c7b

Please sign in to comment.