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

RFC - extend from \yii\console\controllers\MigrateController #9

Closed
wants to merge 3 commits into from

Conversation

nineinchnick
Copy link

Extending from core MigrateController allows easier upgrading of this extension and avoids code duplication.

Because core controller assumes migration names are strings, I've encoded it into JSON to add the alias. This looks ugly when printing names, but works.

This PR can be closed, it's just a proof of concept.

…rateController and use json encoded array instead of migration name
@schmunk42
Copy link
Member

That sounds awesome.

The reason for the code duplication is here btw: yiisoft/yii2#3273 I made the PR, it was open for months and someone did a huge refactoring of the console controllers.

But your PR is not backward compatible, since the old schema is used, just with a different migration name, right ... Is this going to Yii 2 core?

@nineinchnick
Copy link
Author

I think the biggest problem here is when trying to use a different type for migration names than string. This forces to overide most of the methods and actions. That's why I just used JSON.

I did use the new schema with the alias column, but it's not required. And it could detect if the string is valid JSON or not and use the default migrationPath.

I'm not sure this fits into the core. If it would, I'd refactor the core controller to allow using a full object instead of this JSON string.

@schmunk42
Copy link
Member

After a while I gave this a try, but it did not recognize multiple paths specified in params yii.migrations.

There is still an open issue on yiisoft/yii2#384 about this feature.

@nineinchnick Any thoughts about how to continue?

@nineinchnick
Copy link
Author

What params are you referring to? I'm using my fork in production (https://github.com/nineinchnick/yii2-migrate-command). I just configure the migrationLookup property of the migrate command to include all my modules.

@schmunk42
Copy link
Member

See the README

@nineinchnick
Copy link
Author

It seems it's only mentioned in the readme and not used anywhere in the code.

@nineinchnick
Copy link
Author

I've updated the PR and added code handling that app param.

@schmunk42
Copy link
Member

Thanks for the update.

I merged it on develop, looks good to me on first sight.

There are no changed to schema and functionality, right?
But the codebase is much smaller.

CC: @Quexer69 @eluhr

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

Successfully merging this pull request may close these issues.

2 participants