Support alternative migration syntax

schmunk42 commented 7 years ago

What steps will reproduce the problem?

Use migrations with namespaces, like described in https://github.com/dmstr/yii2-migrate-command/issues/19

What is the expected result?

It's logical that the migration can not be found, but it would be nice if there'd be a way, i.e. via alias or application events, to support the syntax via $app->params, which was laid out originally in https://github.com/yiisoft/yii2/issues/384#issuecomment-41576518

Any ideas how to do this?

Additional info

Q	A
Yii version	2.0.10
PHP version	7.x
Operating system	Alpine (Docker)

klimov-paul commented 7 years ago

Migration namespaces support has been added by #12511. No further improvement is planned.

schmunk42 commented 7 years ago

@klimov-paul Yes, I've seen that.

I asked you once about the support of the initially proposed syntax, but there was no answer.

I am fine with the implementation using namespaces, I am just asking for some help or ideas here.

Our package has constantly more than >50 installs/day (~30000 in total) and over 10 dependents, since Yii supports another syntax than many people were using for a while now, I'd really like to provide a solution here.

klimov-paul commented 7 years ago

Sorry, but I prefer not to spend time on compatibility with third party extension. If you wish to get some advice or start discussion conversation - use forum for that.

schmunk42 commented 7 years ago

To be friendly speaking the response is not what I've expected in this case from the Yii side.

As a résumé:

I raised the feature request over 3,5 years ago
there was a discussion about that, Qiang proposed a syntax
I've created a PR, which did not get merged, because of release pressure in 2.0
then there were so many changes, that the PR could not longer be merged, without even reviewing it beforehand
so I've created an extension for it
after that we had to wait over 2 years to get the feature implemented

I was asking here, because supporting an alternative syntax might require a tiny tweak at the core - or not, I don't know it right from the top of my head. But I'll be able to figure out another solution if there'll be absolutely even no further discussion.

CC: @cebe @samdark @SilverFire Any additional thoughts about this?

samdark commented 7 years ago

Aliases and namespaces are not the same but similar. Did extension break because of latest changes in Yii?

schmunk42 commented 7 years ago

@samdark Yes, kind of. Since you're allowed to use namespaces in migrations and (only) if you do so.

Even though some newer features of the migration controller are not supported, it just works.

I understand that the issue is, that our extensions simply overrides a lot of stuff from the underlying class. Then there was the refactoring ... I also received a PR once, which was trying to keep up with the changes made, but it did not pass all the tests, it's not that trivial.

I might implement the old syntax on top of the new version, if there's no other way. I don't request anyone of you to do my work, just to have a closer look at it than 10 seconds.

samdark commented 7 years ago

So, am I correct that you propose having aliases instead of namespaces in the core?

schmunk42 commented 7 years ago

Not instead, either as an additional or alternative syntax (a param to enable this would be totally fine). Or some solution to register additional autoloading paths or aliases in Yii.

Just thinking loud: aliasMigrationNamespacesFromYiiApplicationParams = true. A bit long :) but you get the idea, this way our extension could be completely abandoned while keeping the current config.

I need to double check, if we could easily move over to namespaced syntax, i.e. if it would require refactoring all migration classes, since this would not be feasible for 3rd party extensions.

Or an application event which converts aliases to namespaces, something like that is what I am trying to figure out.

cebe commented 7 years ago

@klimov-paul I think this deserves some discussion, so I'm going to reopen the issue. Also the fact that we broke the extension looks like we broke compatibility here, that should be checked.

klimov-paul commented 7 years ago

To be friendly speaking the response is not what I've expected in this case from the Yii side.

We're quite busy with 574 opened issues so I see no reason why fixing 3rd party extension compatibility is significant enough.

I think this deserves some discussion, so I'm going to reopen the issue.

The problem with extension mentioned appears in case BaseMigrateController::$migrationNamespaces is enabled. If author is unable to make his code compatible with this option - he should either place a restriction on the yiisoft/yii2 package version inside the composer.json to exclude new core version or throw an exception inside his code in case this option is set.

We have made a decision at #384 in favor of namespaces. I can see no reason for it to be reconsidered. The main reason - is name collisions: 2 different paths may contains the migration classes with exact same name. Yes it is rare thing to happen - but it is still possible.

Also BaseMigrateController::$migrationPath is considered as deprecated and will be removed in future versions. Why we should extend its functionality?

Also the fact that we broke the extension looks like we broke compatibility here, that should be checked.

Unless the proper bug report, which proves BC break is provided I can not do anything about it.

cebe commented 7 years ago

@klimov-paul I understand that you consider the feature as complete as you have implemented it. I, as the original author of the Yii 1 extension that did the same thing as dmstr/yii2-migrate-command, would still want to take a look at the situation. So please keep it open, it's assigned to me anyway.

klimov-paul commented 7 years ago

As you wish.

Summarize: the issue https://github.com/dmstr/yii2-migrate-command/issues/19 happens not because there is any BC break at Yii code. It happens because the yii2-migrate-command extension is lack of support for BaseMigrateController::$migrationNamespaces, which should be explicitley added as it was done yiisoft/yii2-mongodb: https://github.com/yiisoft/yii2-mongodb/issues/152

In case BaseMigrateController::$migrationNamespaces is empty yii2-migrate-command works absolutely fine. I recommend you to continue this disscussion at https://github.com/dmstr/yii2-migrate-command/issues/19 if you concern about the matter.

schmunk42 commented 7 years ago

The problem is, if you have an new extension using the migration namespaces, like https://github.com/Nemmo/yii2-attachments you can no longer use the migrationPath syntax

yii migrate --migrationPath=@nemmo/attachments/migrations

Because it says, can not find the migration in the namespace.

If you use extensions with namespaces you also need to change the way you run migrations and that should be done in 2.1. Devs would need to make their extensions require Yii >= 2.0.10 or <2.0.10 depending on their migrations.

This is BC-breaking at least for a patch version.

Migrations are especially critical, since they should not be touched anymore, so having an extension which uses migrations is very hard to update.

cebe commented 7 years ago

we should document a way to deal with this situation if it is easily possible with current implementation or adjust implementation if it is not easily possible.

klimov-paul commented 7 years ago

The problem is, if you have an new extension using the migration namespaces, like https://github.com/Nemmo/yii2-attachments you can no longer use the migrationPath syntax

Native Yii migrate command never allowed namespaced migrations to be passed via migrationPath. The migrations declared at migrationPath should always be without namespace. I can see no BC break here. You have twisted original code to provide functionality, which contradicts the native implementation and now complain it is not working.

As I said you should update your code taking into account migrationNamespaces value instead of pointless arguing.

schmunk42 commented 7 years ago

The migrations declared at migrationPath should always be without namespace. I can see no BC break here.

There's nothing about that in the docs (yes, this is addressed above, now).

Every developer, who is using namespaced migrations MUST require Yii >= 2.0.10.

Please forgive me pointing out issues which might become a problem to the community, since the extension using the new feature isn't requiring the right version https://github.com/Nemmo/yii2-attachments/blob/1.0.0-beta.3/composer.json#L24 (I know that this should be fixed by the extension)

Native Yii migrate command never allowed namespaced migrations to be passed via migrationPath. You have twisted original code to provide functionality, which contradicts the native implementation and now complain it is not working. As I said you should update your code taking into account migrationNamespaces value instead of pointless arguing.

"As I said, you should take into account that namespaced migrations haven't been possible at all, so I could not twist anything - before you twisted the code - instead of pointless arguing." /satire

@klimov-paul I let it go at that, this is no productive discussion, I haven't criticized you or your work, but it looks to me like you think I would do that. I haven't filed a bug, only asked a question or for a possible feature request - that's all. You do not need to tell me, that I need to update my code, if you fail to see any issue with this. I got that.

klimov-paul commented 7 years ago

There's nothing about that in the docs

So what should I place in the docs exactly? The fact that original behavior of the migrations under migrationPath remains to be the same even after separated migrationNamespaces property has been introduced?

you should take into account that namespaced migrations haven't been possible at all

Then where is a BC break you declare to exist here?

schmunk42 commented 7 years ago

So what should I place in the docs exactly? The fact that original behavior of the migrations under migrationPath remains to be the same even after separated migrationNamespaces property has been introduced?

No, the fact that namespaced migrations require at least 2.0.10 and that you can not use them with migrationPath anymore, which has been the standard way up to 2.0.9 and was described several times in the meantime.

Then where is a BC break you declare to exist here?

It not obvious that you can no longer use commands on extensions, because they might or might have not switched to namespaced migrations, you just don't "see" that immediately.

All extensions switching from normal to namespaced migrations should therefore release a major version update, while requiring only patch level upgrade of Yii2.

Extensions like ours or some others need to require Yii < 2.0.10 - until properly updated.

I am not really saying that this is a BC-break, it's just because you asked for it ;) Nonetheless, it looked to me that it should be addressed.

There was already some confusion about this:

[edit]

Sidenote: There are (currently standard/non-namespaced) migrations in the core for session, log, cache and rbac. Just keep the namespace thing mind, if they get an update, they should be run from another location/namespace then. And they should stay (non-namespaced) in the current location. Or a change like this is postponed to 2.1.

klimov-paul commented 7 years ago

that namespaced migrations require at least 2.0.10

migrationNamespaces property has been introduced at 2.0.10, this is explicitly documented: https://github.com/yiisoft/yii2/blob/master/framework/console/controllers/BaseMigrateController.php#L58

All extensions switching from normal to namespaced migrations should therefore release a major version update, while requiring only patch level upgrade of Yii2.

Is not that obvious? If I change any class signature from non-namespaced to namespaced it always means a BC break. No matter if this is a migration class or any other. Any class renaming including namespace changing is a BC break change.

schmunk42 commented 7 years ago

If I change any class signature from non-namespaced to namespaced it always means a BC break.

My whole point is finding something (optional) so that there is no BC-break just because of that, so we can have a smoother transition and introduction of the new feature.

samdark commented 7 years ago

Aren't namespaced migrations optional?

schmunk42 commented 7 years ago

[edit] With the PR below you'd able to run ye-olde global namespaced migrations like so:

yii migrate/up \
    --migrationNamespaces=\
hrzg\\widget\\migrations,\
nemmo\\attachments\\migrations,\
yii/web/migrations,\
yii/rbac/migrations,\
vendor/pheme/yii2-settings/migrations,\
vendor/lajax/yii2-translate-manager/migrations \
    --migrationPath=@app

To be discussed, the current PR is not a real beauty.

klimov-paul commented 7 years ago

Misleading docs has been created by #13201

samdark commented 7 years ago

@klimov-paul could you help correcting these?

klimov-paul commented 7 years ago

I am working on it.

klimov-paul commented 7 years ago

25a7ed60e76ff27b201ba0a759e4552767e6e7f8

samdark commented 7 years ago

After discussion we're not going to support alternative syntax since there's an extension for that but issue voiced in a pull request about fallback when migrating to namespaced migrations from non-namespaced migrations in the middle of the project deserves more attention so PR remains open.

schmunk42 commented 7 years ago

After discussion we're not going to support alternative syntax since there's an extension for that

Which one?

samdark commented 7 years ago

Internal.

There is an existing way of solving the issue with multiple paths — namespaced migrations. The only issue which you correctly highlighted in the pull request is about migrating to namespaces in the middle of the project which is to be thought about and solved one way or another.

Introducing another way similar to namespaces into the core makes no sense from the framework perspective. If namespaces and non-namespaces issue will be solved with a fallback implementation, which is likely, extension providing aliases support would work fine in case of namespaces enabled as well.

yiisoft / yii2