[Loggable] Create migration script for DBAL 4 (array to json)

[stephanvierkant]

Fixed #2883

[Copilot]

Pull request overview

This PR updates the Loggable extension to be compatible with Doctrine DBAL 4 by switching the log entry data storage format from PHP-serialized arrays to JSON, and adds a console command + documentation to help migrate existing databases.

Changes:

• Change LogEntry mapping for data from DBAL array to json / Types::JSON.
• Add a Symfony console command to migrate existing serialized data values into a new JSON-compatible column.
• Update Loggable documentation and changelog to describe the new JSON storage and migration path.

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/Gedmo/Loggable/LoggableEntityTest.php	Removes the DBAL 4 incompatibility skip guard from Loggable tests.
src/Loggable/Entity/MappedSuperclass/AbstractLogEntry.php	Switches the data column mapping from array to JSON.
src/Loggable/Command/MigrateDataToJsonCommand.php	Adds a migration command to convert legacy serialized values into JSON.
doc/loggable.md	Documents DBAL 4 compatibility and provides migration instructions and command usage.
CHANGELOG.md	Notes the addition of the migration script for DBAL 4.
.gitignore	Stops ignoring a top-level bin directory.

Comments suppressed due to low confidence (4)

src/Loggable/Command/MigrateDataToJsonCommand.php:281

• In convertRows(), pagination uses LIMIT ... OFFSET ... while each iteration updates rows so they no longer match WHERE data_serialized IS NOT NULL AND data IS NULL. Because the result set shrinks as you update, incrementing the offset can skip rows and leave some records unmigrated. Prefer keyset pagination (e.g., ORDER BY id and keep the last processed id) or repeatedly selecting the next LIMIT batch without an offset until no rows remain.

        $converted = 0;
        $offset = 0;

        $qData = $platform->quoteSingleIdentifier('data');
        $qDataSerialized = $platform->quoteSingleIdentifier('data_serialized');
        $qId = $platform->quoteSingleIdentifier('id');

        while (true) {
            $rows = $connection->fetchAllAssociative(
                sprintf(
                    'SELECT %s, %s FROM %s WHERE %s IS NOT NULL AND %s IS NULL LIMIT %d OFFSET %d',
                    $qId,
                    $qDataSerialized,
                    $quotedTable,
                    $qDataSerialized,
                    $qData,
                    $batchSize,
                    $offset
                )
            );

src/Loggable/Command/MigrateDataToJsonCommand.php:268

• The migration hard-codes the identifier column name as id for both selection and updates. Custom LogEntry implementations (or legacy schemas) may use a different identifier column, and LogEntryInterface does not guarantee an id field, so the command may fail or update zero rows. Consider resolving the identifier column name(s) from ORM metadata or from the table schema (primary key) and using that in queries.

        $qData = $platform->quoteSingleIdentifier('data');
        $qDataSerialized = $platform->quoteSingleIdentifier('data_serialized');
        $qId = $platform->quoteSingleIdentifier('id');

src/Loggable/Command/MigrateDataToJsonCommand.php:363

• The SQLite path recreates the table using only the CREATE TABLE SQL from sqlite_master and then drops the original table. This does not recreate indexes, triggers, or other schema objects (e.g., the ext_log_entries indexes defined in the ORM mapping), so running this migration on SQLite can silently remove important indexes/constraints. Prefer using SQLite's ALTER TABLE ... RENAME COLUMN when available, or explicitly capture and recreate indexes/triggers from sqlite_master as part of the rebuild.

        $createSql = $connection->fetchOne(
            "SELECT sql FROM sqlite_master WHERE type='table' AND name=?",
            [$table]
        );

        if (false === $createSql || null === $createSql) {
            throw new \RuntimeException(sprintf("Could not read schema for table '%s'.", $table));
        }

        $tmpCreate = preg_replace(
            self::SQLITE_CREATE_TABLE_HEADER_PATTERN,
            '$1'.$quotedTmp.'$3',
            $createSql,
            1
        );

        $tmpCreate = preg_replace(
            self::SQLITE_DATA_COLUMN_PATTERN,
            ' "data_serialized"',
            $tmpCreate,
            1
        );

        $connection->executeStatement($tmpCreate);

        $cols = $connection->fetchAllAssociative(sprintf('PRAGMA table_info(%s)', $table));
        $colList = implode(', ', array_map(
            static fn (array $c): string => $platform->quoteSingleIdentifier($c['name']),
            $cols
        ));
        $connection->executeStatement(sprintf('INSERT INTO %s SELECT %s FROM %s', $quotedTmp, $colList, $quotedTable));

        $connection->executeStatement(sprintf('DROP TABLE %s', $quotedTable));
        $connection->executeStatement(sprintf('ALTER TABLE %s RENAME TO %s', $quotedTmp, $quotedTable));

src/Loggable/Command/MigrateDataToJsonCommand.php:416

• finalize() always uses ALTER TABLE ... DROP COLUMN when --drop-legacy is set. On SQLite this may fail depending on the SQLite version (and you already special-case SQLite elsewhere), which would make the command report success for conversion but fail at cleanup. Consider handling SQLite drops via the same safe table-rebuild approach (including index recreation) or documenting/enforcing a minimum SQLite version for --drop-legacy.

        if ($dropLegacy) {
            $output->writeln("Dropping legacy column 'data_serialized'...");
            $connection->executeStatement(sprintf(
                'ALTER TABLE %s DROP COLUMN %s',
                $quotedTable,
                $platform->quoteSingleIdentifier('data_serialized')
            ));
            $output->writeln('  Done.');

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[mbabker]

Similar here, and Copilot has already pointed out why this is a bit of a pain to deal with at this level.

(All of this might be fine for a one-off tool in an application, but if the package is going to ship a migrator tool, it's a bit more complex to deal with)

[mbabker]

The ORM provides a Doctrine\ORM\Tools\Console\Command\AbstractEntityManagerCommand which can resolve a named entity manager from the command options. It'd be better to use that versus tying this to a single DBAL connection and the manager registry (so instead of having to change the service definition each time you need it, you can use the --em=foo option to get to the right service).

[mbabker]

I'm not sure I'm a fan of this script managing the table schema migration, I'd almost rather tell folks to generate their own temporary migration. This one block is having to replicate a bunch of logic the DBAL and Migrations libraries already cover and only works well for MySQL, PostgreSQL, and SQLite (SQL Server doesn't use this same ALTER TABLE statement for renames).

[stephanvierkant]

I'm not a fan of this script either ;). I know this doesn't work for all users. I thought a very basic migration script is better than no migration script at all.

[mbabker]

Similar here, and Copilot has already pointed out why this is a bit of a pain to deal with at this level.

(All of this might be fine for a one-off tool in an application, but if the package is going to ship a migrator tool, it's a bit more complex to deal with)

[mbabker]

This probably needs to allow some core classes to be deserialized out-of-the-box, or be configurable for end users. Otherwise, this migration script is at best a template to copy and could just go in the documentation (in my own application, I had DateTimeInterface and libphonenumber\PhoneNumber objects in the data array).

[mbabker]

This would need a [B/C Break] marker; the PR isn't just adding a migration script, it's changing the column definition and requires a data migration.

(This is also part of the reason I ended up at building a new extension over trying to migrate in place)

[stephanvierkant]

That's correct. Is bumping to a major version and requiring dbal 4 the best way to deal with it? Not upgrading to dbal 4 is not an option, because you can't upgrade to Symfony 8 with dbal 3.

[mbabker]

Since the library still supports PHP 7.4, the command name and description should probably also be set with the deprecated $defaultName and $defaultDescription properties. That said, I don't know if there's actually a usable combination of symfony/console, doctrine/dbal, and doctrine/orm versions where you'd still be on PHP 7.4 and running this migration, but as is, the command registration would be incomplete for that environment.

[stephanvierkant]

You're right. You'll need Symfony 7.3 as well, but it currently requires ^5.4 || ^6.4 || ^7.3 || ^8.0.

[mbabker]

This would need to be a full service definition since Symfony's DI won't autowire vendor classes either. Factoring in the comment about the abstract command class elsewhere (and using the existing docs conventions), you end up with something like this:

gedmo.loggable.migrate_data_command:
  class: Gedmo\Loggable\Command\MigrateDataToJsonCommand
  arguments: ['@doctrine.orm.command.entity_manager_provider']
  tags:
    - { name: console.command, command: 'gedmo:loggable:migrate-data-to-json' }