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

minor: use doctrine:schema:update in ResetDatabase #497

Open
wants to merge 3 commits into
base: 1.x
Choose a base branch
from

Conversation

Qronicle
Copy link

@Qronicle Qronicle commented Aug 28, 2023

Use schema:update in favour of schema:create when using the DatabaseResetter

Using schema:create always tries to create the non-default PostgreSQL schemas, which triggers an Exception when the schema already exists. (PostgreSQL schemas are not removed by schema:drop)

fixes #495

…esetter

Using schema:create always tries to create the non-default PostgreSQL schemas, which triggers an Exception when the schema already exists. (PostgreSQL schemas are not removed by schema:drop)
@Qronicle Qronicle changed the title Use schema:update in favour of schema:create when using the DatabaseR… Use schema:update in favour of schema:create Aug 28, 2023
@nikophil nikophil changed the title Use schema:update in favour of schema:create minor: use doctrine:schema:update in ResetDatabase Aug 28, 2023
@nikophil
Copy link
Member

nikophil commented Aug 28, 2023

Hi,
thanks for this, sorry for the late reply on your issue

I think you need to update method dropAndResetDatabase() as well.

I also think you need to pass --complete option, as it is deprecated to not pass it:
extract from bin/console d:s:u --help

  Finally, be aware that if the --complete option is passed, this
  task will drop all database assets (e.g. tables, etc) that are *not* described
  by the current metadata. In other words, without this option, this task leaves
  untouched any "extra" tables that exist in the database, but which aren't
  described by any metadata. Not passing that option is deprecated.
  
  Hint: If you have a database with tables that should not be managed
  by the ORM, you can use a DBAL functionality to filter the tables and sequences down
  on a global level:
  
      $config->setSchemaAssetsFilter(function (string|AbstractAsset $assetName): bool {
          if ($assetName instanceof AbstractAsset) {
              $assetName = $assetName->getName();
          }
  
          return !str_starts_with($assetName, 'audit_');
      });

@kbond
Copy link
Member

kbond commented Aug 28, 2023

I think you need to update method dropAndResetDatabase() as well.

Where in that method? It doesn't create the schema.

@nikophil
Copy link
Member

Where in that method? It doesn't create the schema.

yeah sorry, I misread, it is a doctrine:database:create, I'll correct my comment

@Qronicle
Copy link
Author

Added the --complete option. And I can confirm that dropAndResetDatabase has no relation to this indeed.

@nikophil
Copy link
Member

I'm just a bit worried of implications of this change: won't this be a big BC for people who have tables not managed by the ORM in the same db? because of this --complete option that we must add

@kbond
Copy link
Member

kbond commented Aug 28, 2023

because of this --complete option that we must add

Not passing is deprecated and will be required in dbal 4? Not sure how we can avoid? Only add if dbal 4 is detected?

@Qronicle
Copy link
Author

Yeah it's a bit weird that this is now mandatory (or at least will be).

I guess that how you handle this depends on what the goal of this package is then. I would think that resetting the database implies that it removes everything, especially as that is exactly what happens in the migrate mode. I'm also having troubles coming up with scenarios where testing would require keeping custom tables, but deleting orm managed ones.

Anyway, I understand that it might be too high of a risk. And since I discovered that migrate mode also works for us, this really isn't a blocker for me anymore anyway. (Although I can imagine migrations being slower over time.)

@kbond
Copy link
Member

kbond commented Aug 28, 2023

Maybe let's leave --complete off this PR for the time being to be discussed later.

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

Successfully merging this pull request may close these issues.

ResetDatabase does not work with multiple schemas
3 participants