Skip to content
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.

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

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

Open
ADmad opened this issue Oct 10, 2017 · 11 comments
Open

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

ADmad opened this issue Oct 10, 2017 · 11 comments

Comments

@ADmad
Copy link
Member

ADmad commented Oct 10, 2017

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.

@dereuromark
Copy link
Member

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..

@ADmad
Copy link
Member Author

ADmad commented Oct 10, 2017

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

@dereuromark
Copy link
Member

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.

@HavokInspiration
Copy link
Member

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.

@ADmad
Copy link
Member Author

ADmad commented Oct 11, 2017

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.

@HavokInspiration
Copy link
Member

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 ?).

@groovenectar
Copy link

groovenectar commented Jun 28, 2019

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:

public function templateData()

    /**
     * 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

@dereuromark
Copy link
Member

PR welcome then.

@dereuromark
Copy link
Member

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

@ADmad
Copy link
Member Author

ADmad commented Feb 22, 2024

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

@dereuromark
Copy link
Member

So whats the current consensus here, what can we do to address this?

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

No branches or pull requests

4 participants