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

Configuration used by CLI commands is ignored #311

Closed
emodric opened this issue Apr 6, 2020 · 20 comments
Closed

Configuration used by CLI commands is ignored #311

emodric opened this issue Apr 6, 2020 · 20 comments
Milestone

Comments

@emodric
Copy link

emodric commented Apr 6, 2020

It seems that CLI commands ignore the --configuration parameter passed to them and instead use the globally configured migrations.

My config/packages/doctrine_migrations.yaml (src/Migrations is an empty folder):

doctrine_migrations:
    migrations_paths:
        'App\Migrations': 'src/Migrations'

My configuration used with CLI (notice how I added name as a test, which would make the config fail to parse), also tried adding the root config doctrine_migrations:

name: My Vendor Lib Migrations
migrations_paths:
    Vendor\Lib\Migrations\Doctrine: Doctrine
storage:
    table_storage:
        table_name: mylib_migration_versions

So when I run:

php bin/console doctrine:migrations:migrate --configuration=vendor/my/lib/migrations/doctrine.yaml

Instead of ignoring the globally configured migrations in favor of the config provided via CLI, I get the following:

In NoMigrationsToExecute.php line 14:

  Could not find any migrations to execute.


In NoMigrationsFoundWithCriteria.php line 14:

  Could not find any migrations matching your criteria (last).

This indicates that the global config is used (the one without migrations) instead of the CLI one.

@goetas
Copy link
Member

goetas commented Apr 6, 2020

doctrine:migrations:migrate should give you an error since the config parameter is not available at all in symfony.

Can you please post the whole command line rather than the single parts?

@emodric
Copy link
Author

emodric commented Apr 6, 2020

What do you mean?

The command in question is the one from the issue description:

php bin/console doctrine:migrations:migrate --configuration=vendor/my/lib/migrations/doctrine.yaml

With v2, this worked fine, even if a Symfony app on which I run this does not use or configure Migrations, and any configured migrations on app level are simply ignored.

On v3, Migrations seem to use the global app config no matter what.

@goetas
Copy link
Member

goetas commented Apr 6, 2020

Yes they are per application. In which situation do you have multiple migrations in a single application?

@emodric
Copy link
Author

emodric commented Apr 6, 2020

I will copy paste my answer from #310 and close that one, because that one is pretty much false issue, I was too quick to open it, before I ran into this one:

You have a Symfony app, fairly regular, doesn't need access to a database so no Doctrine is used at all.

If you wish to install our bundle, which uses Migrations to manage the DB tables, one of the steps is to run Migrations. To make the install process easier for the users, and to not require them to add Migrations config to their main config/packages, we have the following step:

php bin/console doctrine:migrations:migrate --configuration=vendor/netgen/layouts-core/migrations/doctrine.yaml

where the yaml file is:

name: Netgen Layouts Migrations
migrations_namespace: Netgen\Layouts\Migrations\Doctrine
table_name: nglayouts_migration_versions
migrations_directory: Doctrine

In v2, this works without issues, even if the Symfony app you're installing the bundle on, never even used Migrations and has no previous config. The command will just take the config from the provided file and run it.

This does not work on v3 any more, where our custom configuration specified by --configuration while using doctrine:migrations:migrate config is completely ignored in v3.

@emodric
Copy link
Author

emodric commented Apr 6, 2020

Basically, what we had before was the possibility to manage and run migrations specifically for our bundle, without ever having to think about if running the doctrine:migrations:migrate will potentially run some other migration from other part of the system it should not run. The passed configuration made sure that the configuration used comes from our bundle and everything else is ignored.

@goetas
Copy link
Member

goetas commented Apr 6, 2020

So from my understanding, what is happening in your case is that you were using the bundle for your core app while other parts of the system were using a side effect of the bundle that was allowing you to specify an alternative path for the configuration file while keeping the initial database connection defined globally. Am I right?

Personally I feel that it bends a bit what the bundle was supposed to do (I mean, taking some configs from additional files outside of symfony control).

@goetas
Copy link
Member

goetas commented Apr 6, 2020

Were this other parts of the system running migrations on the same database? If yes, Were there risks of collision with migration names? Or possible dependencies with your bundle?

@emodric
Copy link
Author

emodric commented Apr 6, 2020

Well, not just taking them from the additional files, but replacing the Symfony config entirely with the config specified in CLI, that is the whole point, and from my understanding, that was precisely the intention before.

Our config uses a custom migrations table name (nglayouts_migration_versions) specified in our configuration file (see above), so there is no conflicts possible, even if other parts of the system use their own migrations.

For example, our bundle works without issues with Sylius. Sylius install/upgrade process uses their own migrations configured in Symfony, using migration_versions table, while our bundle installed on top of Sylius, uses our own migrations tracked in already mentioned separate table, all in the same database. The tables of our bundle are completely separate from other tables. Sylius uses their own DB tables, and has no knowledge of our tables, and vice versa.

@goetas
Copy link
Member

goetas commented Apr 6, 2020

Can't you in this case use the vendor/bin/doctrine-migrations commands? Since it seems that you were not relying on the bundle configurations...

@emodric
Copy link
Author

emodric commented Apr 7, 2020

We can, but it makes it automatically more complicated:

  • Composer bin-dir can be changed in root composer.json, automatically making our install instructions invalid. We had those issues before and I'd like to avoid them.
  • vendor/bin/doctrine-migrations requires manually specifying database connection details in migrations-db.php file or via command line, which makes it impossible to run migrations with zero config. Our clients would need to copy and paste their DB config from Symfony to a config file for migrations, which I'd like to avoid. In v2, they would just run the command, as the bundle would be automatically installed and activated via Flex and DB config would be picked up via Symfony commands.

I can't think of a reason why this feature stopped working in v3. The --configuration parameter is there in Symfony commands, just like it's there in vendor/bin/doctrine-migrations, but in case of the Symfony command it's not used.

@goetas
Copy link
Member

goetas commented Apr 7, 2020

I can't think of a reason why this feature stopped working in v3. The --configuration parameter is there in Symfony commands, just like it's there in vendor/bin/doctrine-migrations, but in case of the Symfony command it's not used.

The --configuration parameter should not be there because of this check. If is there, is some kind of bug of the alpha version.

The whole idea behind the multi-ns migrations was to have doctrine "owning" the process of migrations, but allowing bundles to provide migrations.

The workflow you are using seems working, but it seems to me that is relatively manual as each "module" had its own set of migrations, and running them was done issuing multiple doctrine commands. It seems to me that also managing dependencies between migrations was not possible or manually done.

In doctrine migrations, the whole set of dependencies and configurations is resolved at container-compile-time. Not sure how your workflow could be replicated with v3.
Probably this thread is a good place to brainstorm about it.

@goetas
Copy link
Member

goetas commented Apr 7, 2020

A possibile solution would be to replace only a subset of the configs from the dependency factory if the configuration parameter is provided...

@emodric
Copy link
Author

emodric commented Apr 7, 2020

In doctrine migrations, the whole set of dependencies and configurations is resolved at container-compile-time. Not sure how your workflow could be replicated with v3.
Probably this thread is a good place to brainstorm about it.

Maybe introducing some kind of "named" configuration sets would be the way to go. Similar how Doctrine DBAL has a main DB connection, and other, named DB connections, and each connection has it's set of parameters.

The workflow you are using seems working, but it seems to me that is relatively manual as each "module" had its own set of migrations, and running them was done issuing multiple doctrine commands.

That's true, but our product is usually upgraded independent from the rest of the system and has its own clear instructions which detail the exact command that needs to be run, and so far our users did not care that it is somehow different from main migrations, if they even exist. The command worked, and it upgraded our DB tables and that's what matters to our users. What matters more is the fact that upgrading our product is a self-contained process and has no possible way to break other parts of their system by, let's say, running some other migrations by mistake.

It seems to me that also managing dependencies between migrations was not possible or manually done.

Maybe, but in our case, our migrations are completely separate from global app migrations, have their own table and absolutely no dependencies on global app migrations, so no worries there.

A possibile solution would be to replace only a subset of the configs from the dependency factory if the configuration parameter is provided...

I think that only replacing subset might be confusing. What would be the issue with replacing everything?

I definitely need the possibility to specify my own migration paths and a custom migration versions table (nglayouts_migration_versions). Other config options are not relevant to me now, but that can always change.

@goetas
Copy link
Member

goetas commented Apr 8, 2020

Can you please test doctrine/migrations#959 and let me know if it solves the issue for you?

@emodric
Copy link
Author

emodric commented Apr 8, 2020

Hi @goetas !

There's definitelly progress!

First, Migrations complained again that no migrations are configured, which means you can't activate the bundle without having some configuration, which is our case. Maybe #310 shouldn't have been closed? I solved this by adding some dummy config in config/packages.

Next, I tried introducting errors in our config, and Migrations, as expected complained about them. This is good.

However, once the config file was correct, I got the error message that I need to run sync-metadata-storage command and I got the following:

An exception occurred while executing 'ALTER TABLE nglayouts_migration_versions
ADD execution_time INT DEFAULT NULL,
CHANGE version version VARCHAR(1024) NOT NULL,
CHANGE executed_at executed_at DATE TIME DEFAULT NULL':                                                                                                                                                                                      

SQLSTATE[42000]: Syntax error or access violation: 1071 Specified key was too long; max key length is 3072 bytes

My DB charset is utf8mb4 and collation is utf8mb4_unicode_520_ci.

I set the version column length to 255 and then sync command executed OK.

After that, I could run my migrations without issues.

@goetas
Copy link
Member

goetas commented Apr 9, 2020

The issue relative to the sql error is tracked in doctrine/migrations#960

@goetas
Copy link
Member

goetas commented Apr 22, 2020

@emodric I have updated doctrine/migrations#959 with what should be the final version. Could you please test it again and let me know if it works for you?

@emodric
Copy link
Author

emodric commented Apr 22, 2020

@goetas Everything works fine, thanks!

@goetas
Copy link
Member

goetas commented Apr 23, 2020

Solved in doctrine/migrations#959

@goetas goetas closed this as completed Apr 23, 2020
@emodric
Copy link
Author

emodric commented Apr 23, 2020

Thank you @goetas !

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

No branches or pull requests

2 participants