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

Added support for specifying aliases for migrationNamespaces #14241

Merged
merged 5 commits into from
Jun 5, 2017

Conversation

cebe
Copy link
Member

@cebe cebe commented Jun 1, 2017

This is used to specify pathes to migrations that do not have
namespaces.

While not directly supported by the migration command provided by the
framework, these migrations exist in a lot of extensions because custom implementations
of migration controllers out of the framework were using this approach
to load multiple migrations from multiple paths.

Even the framework itself currently ships non-namespaced migrations:

This change allows existing applications to adopt the new namespace-based approach
while keeping existing migrations. While it would be possible to add
namespaces to migrations in the application itself, it is not easily possible
to add namespaces to migrations that come from external sources like
extensions.

related discussion in #13359 and #13356.

this replaces #13359

  • I will add some tests to verify behavior later.

@cebe cebe added this to the 2.0.12 milestone Jun 1, 2017
@cebe cebe added the pr:request for unit tests Unit tests are needed. label Jun 1, 2017
@yii-bot
Copy link

yii-bot commented Jun 1, 2017

Thank you for putting effort in the improvement of the Yii framework.
We have reviewed your pull request.

In order for the framework and your solution to remain stable in the future, we have a unit test requirement in place. Therefore we can only accept your pull request if it is covered by unit tests.

Could you add these please?

Thanks!

P.S. If you have any questions about the creation of unit tests? Don't hesitate to ask for support. More information about unit tests

This is an automated comment, triggered by adding the label pr:request for unit tests.

@klimov-paul
Copy link
Member

I am against such change as it mixing up different terms 'namespace' and 'path alias'.

@klimov-paul klimov-paul removed their request for review June 1, 2017 17:27
@cebe
Copy link
Member Author

cebe commented Jun 1, 2017

@klimov-paul I partly agree with that. We could alternatively add another property or make migrationPath accept an array.

Copy link
Member

@dynasource dynasource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree on having this in a separate property called $migrationPaths

cebe and others added 4 commits June 2, 2017 13:34
This is used to specify pathes to migrations that do not have
namespaces.

While not directly supported by the migration command provideded by the
framework, these migrations exist in a lot of extensions because custom implementations
of migration controllers out of the framework were using this approach
to load multiple migrations from multiple paths.

Even the framework itself currently ships non-namespaced migrations:

- https://github.com/yiisoft/yii2/blob/17a1d91e4a517f4f15dce973bf3c50dd939dce63/framework/rbac/migrations/m140506_102106_rbac_init.php
- https://github.com/yiisoft/yii2/blob/17a1d91e4a517f4f15dce973bf3c50dd939dce63/framework/caching/migrations/m150909_153426_cache_init.php
- https://github.com/yiisoft/yii2/blob/17a1d91e4a517f4f15dce973bf3c50dd939dce63/framework/log/migrations/m141106_185632_log_init.php

This change allows existing applications to adopt the new namespace-based approach
while keeping existing migrations. While it would be possible to add
namespaces to migrations in the application itself, it is not easily possible
to add namespaces to migrations that come from external sources like
extensions.
cebe added a commit to yiisoft/yii2-mongodb that referenced this pull request Jun 2, 2017
@schmunk42
Copy link
Contributor

Works nicely with this rather complex setup:

    'controllerMap' => [
        // Yii 2.0 default migrate controller
        'dev:migrate' => [
            'class' => '\yii\console\controllers\MigrateController',
            'migrationPath' => [
                getenv('APP_MIGRATION_LOOKUP'),
                '@yii/rbac/migrations',
                '@yii/web/migrations',
                '@bedezign/yii2/audit/migrations',
                '@dektrium/user/migrations',
                '@hrzg/widget/migrations',
                '@dmstr/modules/contact/migrations',
                '@dmstr/modules/pages/migrations',
                '@dmstr/modules/redirect/migrations',
                '@vendor/lajax/yii2-translate-manager/migrations',
                '@vendor/pheme/yii2-settings/migrations',
                '@vendor/dmstr/yii2-prototype-module/src/migrations',
            ],
        ],

End of output:

*** applied m170320_133511_auth_items (time: 0.006s)

*** applying m170322_204909_update_timestamp_columns
    > alter column created_at in table dmstr_page to datetime ... done (time: 0.165s)
    > alter column updated_at in table dmstr_page to datetime ... done (time: 0.184s)
*** applied m170322_204909_update_timestamp_columns (time: 0.360s)

*** applying m170327_120427_update_icon_column
    > update dmstr_page ... done (time: 0.004s)
*** applied m170327_120427_update_icon_column (time: 0.010s)

*** applying m170328_111312_init
    > create table {{%dmstr_contact_log}} ... done (time: 0.053s)
*** applied m170328_111312_init (time: 0.057s)


60 migrations were applied.

Migrated up successfully.

Also work via the command line:

$ yii dev:migrate --migrationPath=@app/migrations/demo-data,@app/migrations
Yii Migration Tool (based on Yii v2.0.12-dev)

Total 1 new migration to be applied:
        m150917_193929_rbac

Apply the above migration? (yes|no) [no]:

If the default is wrapped in [] - will add a comment in the code.

* as the migration name contains the origin of the migration in the history, which is not the case when
* using multiple migration paths.
*
* @see $migrationNamespaces
*/
public $migrationPath = '@app/migrations';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to wrap this in brackets, so the CLI can recognize it as an array

public $migrationPath = ['@app/migrations'];

@schmunk42
Copy link
Contributor

At the moment there is migrationPath and migrationNamespaces, which is a bit confusing due to mixing singular and plural.

What's about having a singular and plural form for both - background is that you could use the singular form to inject just a single folder, while having the basic configuration still available in the plural form. So you do not have to override it with all values, plus the one you want to inject.

We use this workflow i.e. for demo-data or test-data.

@cebe
Copy link
Member Author

cebe commented Jun 2, 2017

The naming in plural and singluar has historical reason, in oder to keep BC we can not change it.
Adding more properties sounds complicated to me. I think it is fine as is.

@schmunk42
Copy link
Contributor

schmunk42 commented Jun 2, 2017

Basically I am fine with this, just a more general thing from my side to add: I'd find it helpful, if I could see in advance, if a property of method accepts an alias or requires a path.

So having migrationAliases along with migrationPath in 2.0 and deprecate the latter in 2.1.

[edit]

I know that migrationPath also accepts an alias, but AFAIR not all framework methods/props with ...path do.

@cebe
Copy link
Member Author

cebe commented Jun 5, 2017

I know that migrationPath also accepts an alias, but AFAIR not all framework methods/props with ...path do.

a general rule for this is that all parts that receive input via console or configuration accept an alias. Methods that are only called via code, e.g. FileHelper do not.

make sure console arguments are recognized as array.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants