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

doctrine:schema:validate returns DROP TABLE doctrine_migration_versions; #1406

Open
ValentinDk opened this issue Feb 22, 2024 · 63 comments · May be fixed by #1418 or doctrine/DoctrineMigrationsBundle#529

Comments

@ValentinDk
Copy link

ValentinDk commented Feb 22, 2024

BC Break Report

Q A
BC Break yes
Version 3.0.0

Summary

When I updated doctrine/orm to version 3.0.0, my CI started to fail due to the doctrine:schema check:validate, which returned DROP TABLE doctrine_migration_versions;

I found that adding schema_filter can fix this problem: "^(?!doctrine_migration_versions$)" in the dbal configuration, indeed, then the error no longer appears, but after that my migrations stopped tracking the current state and think that at the moment no migration has been performed.

In version 3.0, the saveMode argument was removed in \Doctrine\ORM\Tools\SchemaTool::getUpdateSchemaSql and because of this, doctrine:schema:validate tracks deleted tables and returns sql for this action.
telegram-cloud-photo-size-2-5384566650814190356-y

Previous behavior

image

Current behavior

image

How to reproduce

  1. composer require doctrine/doctrine-migrations-bundle
  2. composer require doctrine/orm-pack
  3. bin/console doctrine:migration:diff
  4. bin/console doctrine:migration:migrate
  5. bin/console doctrine:schema:validate -v
@dmamchyts
Copy link

+1:

 // 1 schema diff(s) detected:                                                                                          

     DROP TABLE doctrine_migration_versions;

@derrabus derrabus transferred this issue from doctrine/orm Mar 4, 2024
@derrabus derrabus changed the title [3.0.0] doctrine:schema:validate returns DROP TABLE doctrine_migration_versions; doctrine:schema:validate returns DROP TABLE doctrine_migration_versions; Mar 4, 2024
@greg0ire greg0ire pinned this issue Mar 11, 2024
@greg0ire
Copy link
Member

Somebody posted a workaround for Symfony applications relying on the Doctrine bundle:

doctrine:
    dbal:
        schema_filter: ~^(?!doctrine_)~

Can people here please test?

@dmamchyts
Copy link

@greg0ire

[OK] The database schema is in sync with the mapping files.                    

doctrine:migrations:diff also work fine.

image

@greg0ire
Copy link
Member

Possible next steps:

  • determine how this works with DBAL 3
  • determine why it no longer works with DBAL 4

@wmouwen
Copy link

wmouwen commented Mar 12, 2024

The bug exists on ORM 3.1 with DBAL 3.8 as well, it is not per se a DBAL 4.0 issue.

@greg0ire
Copy link
Member

greg0ire commented Mar 12, 2024

OK so possible next steps:

  • determine how this works with ORM 2
  • determine why it no longer works with ORM 3

If somebody could dump their DBAL schema filter at runtime using the DBAL configuration object on ORM 2 then on ORM 3, it might help.

@greg0ire
Copy link
Member

greg0ire commented Mar 13, 2024

Can somebody here please confirm that they get the issue with ORM 2 when using the --complete flag? I just remembered that this is likely the cause for this bug.

@dmamchyts
Copy link

/var/www/html $ composer show | grep doctrine
doctrine/cache                        2.2.0
doctrine/collections                  2.2.1
doctrine/common                       3.4.3
doctrine/data-fixtures                1.7.0
doctrine/dbal                         3.8.3
doctrine/deprecations                 1.1.3
doctrine/doctrine-bundle              2.11.3
doctrine/doctrine-fixtures-bundle     3.5.1
doctrine/doctrine-migrations-bundle   3.3.0
doctrine/event-manager                2.0.0
doctrine/inflector                    2.0.10
doctrine/instantiator                 2.0.0
doctrine/lexer                        3.0.1
doctrine/migrations                   3.7.4
doctrine/orm                          2.19.0
doctrine/persistence                  3.3.2
doctrine/sql-formatter                1.2.0
phpstan/phpstan-doctrine              1.3.62
symfony/doctrine-bridge               v7.0.5



/var/www/html $ bin/console doctrine:migration:diff --help

Description:
  Generate a migration by comparing your current database to your mapping information.

Usage:
  doctrine:migrations:diff [options]

Options:
      --configuration=CONFIGURATION                        The path to a migrations configuration file. [default: any of migrations.{php,xml,json,yml,yaml}]
      --em=EM                                              The name of the entity manager to use.
      --conn=CONN                                          The name of the connection to use.
      --namespace=NAMESPACE                                The namespace to use for the migration (must be in the list of configured namespaces)
      --filter-expression=FILTER-EXPRESSION                Tables which are filtered by Regular Expression.
      --formatted                                          Format the generated SQL.
      --line-length=LINE-LENGTH                            Max line length of unformatted lines. [default: "120"]
      --check-database-platform[=CHECK-DATABASE-PLATFORM]  Check Database Platform to the generated code. [default: false]
      --allow-empty-diff                                   Do not throw an exception when no changes are detected.
      --from-empty-schema                                  Generate a full migration as if the current database was empty.
  -h, --help                                               Display help for the given command. When no command is given display help for the list command
  -q, --quiet                                              Do not output any message
  -V, --version                                            Display this application version
      --ansi|--no-ansi                                     Force (or disable --no-ansi) ANSI output
  -n, --no-interaction                                     Do not ask any interactive question
  -e, --env=ENV                                            The Environment name. [default: "dev"]
      --no-debug                                           Switch off debug mode.
      --profile                                            Enables profiling (requires debug).
  -v|vv|vvv, --verbose                                     Increase the verbosity of messages: 1 for normal output, 2 for more verbose output and 3 for debug






/var/www/html $ bin/console doctrine:schema:validate --help

Description:
  Validate the mapping files

Usage:
  doctrine:schema:validate [options]

Options:
      --em=EM                Name of the entity manager to operate on
      --skip-mapping         Skip the mapping validation check
      --skip-sync            Skip checking if the mapping is in sync with the database
      --skip-property-types  Skip checking if property types match the Doctrine types
  -h, --help                 Display help for the given command. When no command is given display help for the list command
  -q, --quiet                Do not output any message
  -V, --version              Display this application version
      --ansi|--no-ansi       Force (or disable --no-ansi) ANSI output
  -n, --no-interaction       Do not ask any interactive question
  -e, --env=ENV              The Environment name. [default: "dev"]
      --no-debug             Switch off debug mode.
      --profile              Enables profiling (requires debug).
  -v|vv|vvv, --verbose       Increase the verbosity of messages: 1 for normal output, 2 for more verbose output and 3 for debug

No --complete flags

@greg0ire
Copy link
Member

--complete is a flag on commands of the doctrine namespace, which are the commands this issue is about, right?

@yapro
Copy link

yapro commented Mar 13, 2024

This is a real problem for our project, especially for production.
And this trick doesn't work:

schema_filter: ~^(?!doctrine_)~

because it causes an even bigger problem:

bin/console doctrine:migrations:migrate --no-interaction

In ExceptionConverter.php line 45:
                                                                                                                                                              
  An exception occurred while executing a query: SQLSTATE[42S01]: Base table or view already exists: 1050 Table 'doctrine_migration_versions' already exists  
                                                                                                                                                              

In Exception.php line 28:
                                                                                                               
  SQLSTATE[42S01]: Base table or view already exists: 1050 Table 'doctrine_migration_versions' already exists  
                                                                                                               

In Connection.php line 33:
                                                                                                               
  SQLSTATE[42S01]: Base table or view already exists: 1050 Table 'doctrine_migration_versions' already exists  
                                                                                                               

doctrine:migrations:migrate [--write-sql [WRITE-SQL]] [--dry-run] [--query-time] [--allow-no-migration] [--all-or-nothing [ALL-OR-NOTHING]] [--configuration CONFIGURATION] [--em EM] [--conn CONN] [--] [<version>]

@wmouwen
Copy link

wmouwen commented Mar 13, 2024

Here is a shell script to trigger both a succeeding workflow with ORM 2.19 and a failing one with ORM 3.0.

Script
#!/usr/bin/env sh

# Set up new Symfony 7 project
composer create-project symfony/skeleton:"7.0.*" my_test_project
cd my_test_project

# Install Doctrine bundles
composer require --no-interaction "doctrine/doctrine-bundle:~2.11" "doctrine/doctrine-migrations-bundle:~3.3" "doctrine/orm:~2.19" "doctrine/dbal:~3.8"

# Use sqlite for local testing
echo "DATABASE_URL=\"sqlite:///%kernel.project_dir%/var/data.db\"" > .env.local

# Install empty migration. This creates the sqlite database with the doctrine_migration_versions table
bin/console doctrine:migrations:generate
bin/console doctrine:migrations:migrate

# Validate schema: succeeds
bin/console doctrine:schema:validate -v

# Upgrade to ORM 3.*
composer require "doctrine/orm:~3.0"

# Validate schema: fails
bin/console doctrine:schema:validate -v

The difference causing the change is that within the callstack of the command, the variable $saveMode in \Doctrine\DBAL\Schema\SchemaDiff::_toSql (link) is true when using ORM 2.19 and false when using ORM 3.0, causing dropped tables to be included in the changeset.

Backtrace ORM 2.19
[generate trace] at vendor/doctrine/dbal/src/Schema/SchemaDiff.php:252
 Doctrine\DBAL\Schema\SchemaDiff->_toSql() at vendor/doctrine/dbal/src/Schema/SchemaDiff.php:225
 Doctrine\DBAL\Schema\SchemaDiff->toSaveSql() at vendor/doctrine/orm/src/Tools/SchemaTool.php:992
 Doctrine\ORM\Tools\SchemaTool->getUpdateSchemaSql() at vendor/doctrine/orm/src/Tools/SchemaValidator.php:358
 Doctrine\ORM\Tools\SchemaValidator->getUpdateSchemaList() at vendor/doctrine/orm/src/Tools/SchemaValidator.php:344
 Doctrine\ORM\Tools\SchemaValidator->schemaInSyncWithMetadata() at vendor/doctrine/orm/src/Tools/Console/Command/ValidateSchemaCommand.php:75
 Doctrine\ORM\Tools\Console\Command\ValidateSchemaCommand->doExecute() at vendor/doctrine/orm/src/Tools/Console/CommandCompatibility.php:18
 Doctrine\ORM\Tools\Console\Command\ValidateSchemaCommand->execute() at vendor/symfony/console/Command/Command.php:279
 Symfony\Component\Console\Command\Command->run() at vendor/symfony/console/Application.php:1049
 Symfony\Component\Console\Application->doRunCommand() at vendor/symfony/framework-bundle/Console/Application.php:125
 Symfony\Bundle\FrameworkBundle\Console\Application->doRunCommand() at vendor/symfony/console/Application.php:318
 Symfony\Component\Console\Application->doRun() at vendor/symfony/framework-bundle/Console/Application.php:79
 Symfony\Bundle\FrameworkBundle\Console\Application->doRun() at vendor/symfony/console/Application.php:169
 Symfony\Component\Console\Application->run() at vendor/symfony/runtime/Runner/Symfony/ConsoleApplicationRunner.php:49
 Symfony\Component\Runtime\Runner\Symfony\ConsoleApplicationRunner->run() at vendor/autoload_runtime.php:29
 require_once() at bin/console:11
Backtrace ORM 3.0
[generate trace] at vendor/doctrine/dbal/src/Schema/SchemaDiff.php:252
 Doctrine\DBAL\Schema\SchemaDiff->_toSql() at vendor/doctrine/dbal/src/Schema/SchemaDiff.php:242
 Doctrine\DBAL\Schema\SchemaDiff->toSql() at vendor/doctrine/dbal/src/Platforms/AbstractPlatform.php:2391
 Doctrine\DBAL\Platforms\AbstractPlatform->getAlterSchemaSQL() at vendor/doctrine/orm/src/Tools/SchemaTool.php:900
 Doctrine\ORM\Tools\SchemaTool->getUpdateSchemaSql() at vendor/doctrine/orm/src/Tools/SchemaValidator.php:322
 Doctrine\ORM\Tools\SchemaValidator->getUpdateSchemaList() at vendor/doctrine/orm/src/Tools/SchemaValidator.php:308
 Doctrine\ORM\Tools\SchemaValidator->schemaInSyncWithMetadata() at vendor/doctrine/orm/src/Tools/Console/Command/ValidateSchemaCommand.php:71
 Doctrine\ORM\Tools\Console\Command\ValidateSchemaCommand->execute() at vendor/symfony/console/Command/Command.php:279
 Symfony\Component\Console\Command\Command->run() at vendor/symfony/console/Application.php:1049
 Symfony\Component\Console\Application->doRunCommand() at vendor/symfony/framework-bundle/Console/Application.php:125
 Symfony\Bundle\FrameworkBundle\Console\Application->doRunCommand() at vendor/symfony/console/Application.php:318
 Symfony\Component\Console\Application->doRun() at vendor/symfony/framework-bundle/Console/Application.php:79
 Symfony\Bundle\FrameworkBundle\Console\Application->doRun() at vendor/symfony/console/Application.php:169
 Symfony\Component\Console\Application->run() at vendor/symfony/runtime/Runner/Symfony/ConsoleApplicationRunner.php:49
 Symfony\Component\Runtime\Runner\Symfony\ConsoleApplicationRunner->run() at vendor/autoload_runtime.php:29
 require_once() at bin/console:11

@greg0ire
Copy link
Member

Related PRs:

Instead of using SchemaDiff::toSaveSql() for discarding schema changes in 3rd-party database objects (if that's the case), it is recommended to use asset filtering.

I think we should leverage asset filtering to do this. If it causes issues with command from doctrine/migrations, then that needs to be investigated (maybe the filter needs to be dynamic).

@greg0ire
Copy link
Member

greg0ire commented Mar 13, 2024

Just remembered this solution that does exactly that (using a dynamic filter).

Something better that might benefit not just Symfony users might be to do it directly from code inside doctrine/migrations, at runtime.

@gjuric
Copy link

gjuric commented Mar 14, 2024

Proposed schema_filter solution does not work because it hides the doctrine_migrations_versions table from the rest of the code that needs to see it.

If you run for example doctrine:migrations:list you can see that Doctrine thinks that none of the migrations have been executed because it believes doctrine_migrations_versions table does not exist.

@greg0ire
Copy link
Member

@gjuric yes, @yapro already established that in #1406 (comment)

Can you try the solution in my last command? I'm currently working on integrated the solution from my previous comment in the migrations bundle.

@greg0ire
Copy link
Member

Here is my attempt: doctrine/DoctrineMigrationsBundle#526

@Brewal
Copy link

Brewal commented Mar 15, 2024

For now, I manually add the migration table to the schema with a doctrine event listener on postGenerateSchema event and don't use schema_filter.

Here's a gist if anyone interested : https://gist.github.com/Brewal/d0fe0792a69e7e5fdf3fb06898b20d35

@stof
Copy link
Member

stof commented Mar 15, 2024

Adding a listener for postGenerateSchema to register the migration table in the schema is an interesting approach. Maybe doctrine/migrations should ship with such a listener.

@greg0ire
Copy link
Member

If it relies solely on the ORM's event system and not on Symfony or the DoctrineBundle, I can see how it would be superior to the asset filtering solution, in that it could benefit to other frameworks such as Laminas or Laravel as well.

@Brewal @stof are you willing to open a pull request? Otherwise I might work on it soon.

If I understand properly, the ORM command works by diffing 2 schemas.
@FlyingDR's solution works by making sure the migrations table appears in neither schemas while @Brewal's solution works by making sure it appears in both schemas.

@FlyingDR's relies on asset filtering at the DBAL level, while Symfony's event system, which means it works only for Symfony (I think? Can orm:schema-tool:update be used in other frameworks, and if yes, are Symfony events also fired?). It means it also has to be disabled for migrations commands which need to know about the table.

@Brewal's solution relies on the ORM event system, which will kick in only for the ORM. I hope we are not forgetting about other cases relying on schema diffing and not relying on the ORM somehow.

Are there other pros or cons I'm forgetting ?

@wmouwen
Copy link

wmouwen commented Mar 15, 2024

by making sure the migrations table appears in neither schemas

Removing the table from the schemas won't detect changes to version_column_name or the other column-related settings. Having the table appear in both schemas does cover this.

@greg0ire
Copy link
Member

Gave it a try, and I'm starting to see some cons:

  1. Is it really orm:schema-tool:update's responsibility to create that migrations table? If you are using orm:schema-tool:update, then you are bypassing migrations, and do not need that table so… there is no point in detecting changes in it, is there? Likewise, the role of doctrine:schema:validate is, according to the error message in @ValentinDk 's screenshot above, to validate that the schema is in sync with the mapping files. The migrations table is not represented by mapping files, and is not part of the ORM schema.
  2. It leads to a duplication between the listener code and this method: https://github.com/doctrine/migrations/blob/3.7.x/lib/Doctrine/Migrations/Metadata/Storage/TableMetadataStorage.php#L231-L246

I think I'd rather we go with @FlyingDR's solution, where only doctrine/migrations knows about the metadata storage table.

@wmouwen
Copy link

wmouwen commented Mar 17, 2024

I gave it a try as well and ended up opening three PRs where each PR requires the previous one.

  1. Create postGenerateComparisonSchema event orm#11374
  2. Remove migration table from generated comparison schema #1418
  3. Register event listener to remove migration table from comparison schema DoctrineMigrationsBundle#529

Is it really orm:schema-tool:update's responsibility to create that migrations table? If you are using orm:schema-tool:update, then you are bypassing migrations, and do not need that table so… there is no point in detecting changes in it, is there?

I don't believe so as doctrine/migrations is using doctrine/orm, however doctrine/orm can open up its core through events to allow doctrine/migrations to alter its behavior. It has allowed it to do so with the postGenerateSchema event, so adding postGenerateComparisonSchema felt logical. The SchemaAssetFilter serves the same purpose but with a different, less open approach.

The migrations table is not represented by mapping files, and is not part of the ORM schema.

Hence the choice to remove the table from the comparison schema instead of adding it to the generated (metadata) schema. This doesn't catch changes to version_column_name as I mentioned in a previous comment, but there's no proper way to put that change in a migration anyway.

One thing that is missing is documentation on how to use #1418 without DoctrineMigrationsBundle. I feel like we can and should add that to the docs of this repository.

@cosminsandu
Copy link

Since the latest version of doctrine/orm is 3.8.0 I don't think this is an option ...

@fnowacki
Copy link

@cosminsandu so if it's not an option for you, please help and contribute to the PR with the fix 👍

@cosminsandu
Copy link

It works with the above workaround

@berkut1
Copy link

berkut1 commented Jul 23, 2024

@cosminsandu it doesn't. Re-run the diff command again :)

@cosminsandu
Copy link

cosminsandu commented Jul 23, 2024

Actually the following commands works OK on my side:

php bin/console doctrine:database:drop --force --if-exists
php bin/console doctrine:database:create --if-not-exists
php bin/console doctrine:migration:migrate
php bin/console doctrine:schema:validate -v

image

The php bin/console doctrine:migration:diff create a new empty migration with nothing in the up method.

I am using doctrine/orm 3.1.0 and doctrine/dbal 4.0.1

@Brewal
Copy link

Brewal commented Jul 23, 2024

@cosminsandu it doesn't. Re-run the diff command again :)

Did you gave this a try : #1406 (comment)

I fixed it for me with doctrine/orm 3.2.1 and dbal 4.0.4

@berkut1
Copy link

berkut1 commented Jul 23, 2024

@cosminsandu you will face with this issue soon #1406 (comment)

@Brewal Yeah, I saw this solution, but for now, the current error doesn't bother me much.

@cosminsandu
Copy link

@berkut1 indeed this problem appears #1406 (comment)

I will try this solution #1406 (comment)

@Brewal
Copy link

Brewal commented Jul 23, 2024

@berkut1 sorry, I replied to the wrong person 😄

I was trying to prevent "any updates ?" comments notifications that might bother people.

For anyone else : as of now there are workarounds and also PRs opened to fix this issue. Understand that my solution #1406 (comment) will probably not be implemented (see this comment : #1406 (comment) and the following ones). That's why I keep notifications on this issue.

@esraanfq
Copy link

esraanfq commented Aug 7, 2024

when will the solution be merged?

@mvhirsch
Copy link

I can confirm this issue applies on my project, too. Using workaround in #1406 (comment) prevents me from re-running doctrine:migrations:migrate -n again.

symfony composer info | grep doctrine
dama/doctrine-test-bundle           v8.2.0  Symfony bundle to isolate doctrine database tests and improve test performance
doctrine/cache                      2.2.0   PHP Doctrine Cache library is a popular cache implementation that supports many different drivers such as redis, memcache, apc, mongodb and others.
doctrine/collections                2.2.2   PHP Doctrine Collections library that adds additional functionality on top of PHP arrays.
doctrine/data-fixtures              1.7.0   Data Fixtures for all Doctrine Object Managers
doctrine/dbal                       3.9.1   Powerful PHP database abstraction layer (DBAL) with many features for database schema introspection and management.
doctrine/deprecations               1.1.3   A small layer on top of trigger_error(E_USER_DEPRECATED) or PSR-3 logging with options to disable all deprecations or selectively for packages.
doctrine/doctrine-bundle            2.13.0  Symfony DoctrineBundle
doctrine/doctrine-fixtures-bundle   3.6.1   Symfony DoctrineFixturesBundle
doctrine/doctrine-migrations-bundle 3.3.1   Symfony DoctrineMigrationsBundle
doctrine/event-manager              2.0.1   The Doctrine Event Manager is a simple PHP event system that was built to be used with the various Doctrine projects.
doctrine/inflector                  2.0.10  PHP Doctrine Inflector is a small library that can perform string manipulations with regard to upper/lowercase and singular/plural forms of words.
doctrine/instantiator               2.0.0   A small, lightweight utility to instantiate objects in PHP without invoking their constructors
doctrine/lexer                      3.0.1   PHP Doctrine Lexer parser library that can be used in Top-Down, Recursive Descent Parsers.
doctrine/migrations                 3.8.1   PHP Doctrine Migrations project offer additional functionality on top of the database abstraction layer (DBAL) for versioning your database schema and easily deploying changes to it. It is ...
doctrine/orm                        3.2.2   Object-Relational-Mapper for PHP
doctrine/persistence                3.3.3   The Doctrine Persistence project is a set of shared interfaces and functionality that the different Doctrine object mappers share.
doctrine/sql-formatter              1.4.1   a PHP SQL highlighting library
phpstan/phpstan-doctrine            1.5.3   Doctrine extensions for PHPStan
symfony/doctrine-bridge             v6.4.12 Provides integration for Doctrine with various Symfony components

Issue persists after upgrading doctrine/dbal to v4.4.1.

@greg0ire
Copy link
Member

greg0ire commented Oct 9, 2024

Hello from the Doctrine team meetup.

Workaround: as soon as you use migrations, run bin/console doctrine:schema:validate --skip-sync && bin/console doctrine:migrations:up-to-date instead of bin/console doctrine:schema:validate.
Possible solution to be implemented: detect doctrine:migrations:up-to-date is available and automatically call it when calling doctrine:schema:validate.

@stof
Copy link
Member

stof commented Oct 9, 2024

@greg0ire bin/console doctrine:migrations:up-to-date will not catch a mistake where you changed the mapping but forgot to generate a corresponding DB migration.

My CI setup involves executing migrations starting from an empty database (so bin/console doctrine:migrations:up-to-date will then always succeed after that) and running bin/console doctrine:schema:validate to validate that the migrated schema is in sync with the mapping.

@greg0ire
Copy link
Member

greg0ire commented Oct 9, 2024

@stof oh right! To elaborate on my previous answer, we would like to be able to remove the schema tool update feature and rely solely on migrations for this. If I understand your answer well, the missing piece would be a flag or a separate command in the migrations namespace to check that the diff is empty, is that it? In the end, the UX would be slightly better, because it could tell you whether you forgot to execute a migration or to generate it. We plan to introduce a new dummy metadata storage that will allow to use migrations without a database table, and remove the duplicate code.

So the right current workaround would be bin/console doctrine:schema:validate --skip-sync && bin/console doctrine:migrations:up-to-date && ! bin/console doctrine:migrations:diff

That's assuming the only reason doctrine:migrations:diff exits with a non zero status code is because there is no diff, and that's a big assumption, so the first thing to do would be to introduce a new command or a new flag just to check this.

@stof
Copy link
Member

stof commented Oct 9, 2024

Given that all the infrastructure for comparing schemas is in DBAL anyway, why removing this from doctrine:schema:validate (even if you remove doctrine:schema:update) ?

@greg0ire
Copy link
Member

I suppose it not so much a removal as it would be a move. We'd be moving the sync test functionality to a place where it would belong more (the migrations package). Also, it seems that although there is a lot of code in common between this and doctrine:migrations:diff as you pointed out, doctrine:migrations:diff does not suggest to drop the migrations table. So there must be a crucial difference between how both places implement this feature, and why would we want to have to do extra work to have the same feature work in 2 places?

Anyway, I'd be grateful if somebody here could confirm that bin/console doctrine:schema:validate --skip-sync && bin/console doctrine:migrations:up-to-date && ! bin/console doctrine:migrations:diff is a satisfying workaround (I'm not claiming it's a solution and won't close this issue if you say yes).

@stof
Copy link
Member

stof commented Oct 10, 2024

As long as the issue of generating a DROP SCHEMA public in the down method is not solved, this is not even a workaround at all, as the diff will never be empty due to this statement.

Also, checking diff for failure is not reliable. If a PHP exception happens, you will also get a failure exit code, but this shouldn't indicate a success of the comparison. We really need a checking command.

So there must be a crucial difference between how both places implement this feature

The reason is that the DiffGenerator temporarily changes the configuration of DBAL to ignore that table from the introspection when generating the diff (so that it does not think it exists):

$this->dbalConfiguration->setSchemaAssetsFilter(
(to be exact, it permanently changes the config until the end of the PHP process, making this class unsuitable for usages outside the diff command that exits the process once generated)

@greg0ire
Copy link
Member

Oh right, this is not a workaround for Postgres users indeed. We'll have to give this more thought.

@jer-teamdotblue
Copy link

Also: running bin/console doctrine:migrations:diff in a pipeline might be problematic, since it tries to actually create a migration class / file. There doesn't appear to be a --dry-run flag on that command? A dry run might make it easier to replace bin/console doctrine:schema:validate -v.

@greg0ire
Copy link
Member

Well… the CREATE SCHEMA public in the down method issue just got fixed: #1463 🙂

might be problematic, since it tries to actually create a migration class / file

Do you mean because your CI filesystem is readonly or what? Please elaborate.

A dry-run flag might make sense indeed, it could be useful for other purposes than checking that the diff is empty, but I think a standalone command that would exit with 0 in case of an empty diff would be better, because you would not have to negate it like I did.

@cosminsandu
Copy link

cosminsandu commented Oct 11, 2024

Just Upgrading doctrine/migrations (3.8.1 => 3.8.2) https://github.com/doctrine/migrations/releases/tag/3.8.2
and the php bin/console doctrine:schema:validate -v returns

// 1 schema diff(s) detected:

     DROP TABLE doctrine_migration_versions;

But the php bin/console make:migration correctly returns [WARNING] No database changes were detected.

@jer-teamdotblue
Copy link

might be problematic, since it tries to actually create a migration class / file

Do you mean because your CI filesystem is readonly or what? Please elaborate.

A dry-run flag might make sense indeed, it could be useful for other purposes than checking that the diff is empty, but I think a standalone command that would exit with 0 in case of an empty diff would be better, because you would not have to negate it like I did.

I seem to have misunderstood this part:

We'd be moving the sync test functionality to a place where it would belong more (the migrations package)
The part that we need in the pipeline, is the mapping validation, which I suppose is not planned for any changes?

We have a couple of use cases for schema:validate - one is to check mappings in the pipeline step, but that one runs with --skip-sync, and check the exit status. We probably could live with a unnecessary Migration being created here, since the pipeline step would run in a container anyway, which at the moment is able to write to the folder.

The other use case could be compared to schema:update, where we use it when a developer have switched branches a couple of times, and gotten their database out of sync.
Since we have over 200 tables, some of them with gigabytes of data, it's usually not feasible to restore the database to an earlier point, when all we need to 'fix' is a couple of columns or indexes.

I usually use schema:validate -v to print out which columns are in error, and either use schema:update to fix it, or run the queries in a console.

If schema:update was replaced with migration:diff I would have to create a unwanted migration, and either run it, and wipe it from the migration metadata table, or copy the queries out of it, and run it manually.
That seems like an degradation in workflow experience, over having the schema:update command at hand, as it is now.

Also: a couple of projects are not using the migration system for security reasons. If the schema:validate sync part / schema:update part got moved to migrations, would there be no way to check if those are in sync with the development environment?

@greg0ire
Copy link
Member

The part that we need in the pipeline, is the mapping validation, which I suppose is not planned for any changes?

The mapping validation is not planned for any changes, no.

We probably could live with a unnecessary Migration being created here, since the pipeline step would run in a container anyway, which at the moment is able to write to the folder.

Yes… and more importantly, the exit code of the command being negated in my proposal would make your deployment fail right after the file is created.

If schema:update was replaced with migration:diff I would have to create a unwanted migration, and either run it, and wipe it from the migration metadata table, or copy the queries out of it, and run it manually.

Our plan would be to have a mode for the migrations package. When in changelog mode, everything would work as it works now. When in "no-changelog" mode, things would work "in memory": no migration file, and the metadata table would be ignored. So basically it would work like schema:update, and the end result would be that we would have this functionality reside in a single package instead of 2, making maintenance easier.

Also: a couple of projects are not using the migration system for security reasons. If the schema:validate sync part / schema:update part got moved to migrations, would there be no way to check if those are in sync with the development environment?

Can you elaborate on the "security reasons"?

@jer-teamdotblue
Copy link

Also: a couple of projects are not using the migration system for security reasons. If the schema:validate sync part / schema:update part got moved to migrations, would there be no way to check if those are in sync with the development environment?

Can you elaborate on the "security reasons"?

In some environments, database schema changes need to be controlled more tightly, using a two-man rule, and all schema changes are deployed by hand. The database user which Doctrine uses, have only a limited set of grants, and do not allow for schema changes. Those projects do not have the Doctrine Migrations Bundle installed at all.

@arderyp
Copy link

arderyp commented Oct 11, 2024

Also: a couple of projects are not using the migration system for security reasons. If the schema:validate sync part / schema:update part got moved to migrations, would there be no way to check if those are in sync with the development environment?

I don't fall into this camp, but this is a good question

@greg0ire
Copy link
Member

greg0ire commented Oct 11, 2024

I see. With the new design you would still have to install doctrine/migrations bundle on those projects. Not for migrating anything, but just to run that check command I guess, in non-changelog mode. A read-only user would be sufficient for that purpose. Feels a bit overkill, so I'll make sure to mention your point if we have internal discussions about this. Note that if we decide against moving the schema:validate sync part, you would still need the bundle in dev to do the schema:update part.

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