migrations icon indicating copy to clipboard operation
migrations copied to clipboard

Incorrect behavior when using "migration_diff" with "--plugin" option.

Open ADmad opened this issue 8 years ago • 11 comments

When using migration_diff with --plugin option is creates migrations for all tables in the db instead of just the tables for which Table classes are present in the plugin. It would except it to only consider the tables of the plugin similar to how migration_snapshot does.

ADmad avatar Oct 10 '17 13:10 ADmad

The main problem is that without some table prefixing for plugins it will be rather difficult to detect "new tables" vs "existing but not plugin related". There will either be too many or too less shown then..

dereuromark avatar Oct 10 '17 13:10 dereuromark

Can't migration_diff simply ignore tables for which it doesn't find a class file inside the plugin?

ADmad avatar Oct 10 '17 14:10 ADmad

For normal purposes I agree. But what if you want to add this new SQL table into your plugin? Then you would want this one to be not excluded. I agree, however, that this case is more the edge case then yours.

dereuromark avatar Oct 10 '17 14:10 dereuromark

I remember doing that in the first version of the command : only including tables from the db which have a Table file in the plugin. But for some reason, they were comments that asked me to remove this. I don't remember why exactly though.

HavokInspiration avatar Oct 11 '17 15:10 HavokInspiration

Can't see why people wouldn't want that behavior since that's how snapshot works too. Anyway the behavior could have been made configurable.

ADmad avatar Oct 11 '17 16:10 ADmad

It still can.

One would need to modify the MigrationDiffTask::getCurrentSchema() method to only filter the tables with a Table class existing in the plugin directory, if there is a specific parameter (some other methods have a require-table bool flag that enforce the need for Table classes to exist), using the TableFinderTrait for instance (which might need a bit of refactor to avoid code duplication ?).

HavokInspiration avatar Oct 12 '17 07:10 HavokInspiration

Just to add some information, I tested including use TableFinderTrait; in MigrationDiffTask.php

If I add a few testing lines to the templateData() similar to what's in MigrationSnapshot.php:

https://github.com/cakephp/migrations/blob/e5a92d961acb3371f6b7649cf9017647d2cb6d9a/src/Shell/Task/MigrationDiffTask.php#L171

    /**
     * Process and prepare the data needed for the bake template to be generated.
     *
     * @return array
     */
    public function templateData()
    {
        $connection = ConnectionManager::get($this->connection);
        $options = [
            'require-table' => true,
            // Left out of this GitHub post for simplicity
            //'require-table' => $this->params['require-table'],
            'plugin' => $this->plugin
        ];
        $tables = $this->getTablesToBake($connection->getSchemaCollection(), $options);
        dd($tables);

        $this->dumpSchema = $this->getDumpSchema();
        $this->currentSchema = $this->getCurrentSchema();
        $this->commonTables = array_intersect_key($this->currentSchema, $this->dumpSchema);

        $this->calculateDiff();

        return [
            'data' => $this->templateData,
            'dumpSchema' => $this->dumpSchema,
            'currentSchema' => $this->currentSchema,
        ];
    }

Then run bin/cake bake migration_diff --plugin=PluginName TestAddColumn

It does indeed dump an array of just the table names contained within the plugin

groovenectar avatar Jun 28 '19 17:06 groovenectar

PR welcome then.

dereuromark avatar Apr 11 '20 13:04 dereuromark

@ADmad Are you still interested in following up on this?

dereuromark avatar Feb 21 '24 06:02 dereuromark

My only follow up for now is I would like to get it addressed/fixed :)

ADmad avatar Feb 22 '24 08:02 ADmad