foundry icon indicating copy to clipboard operation
foundry copied to clipboard

minor: use `doctrine:schema:update` in `ResetDatabase`

Open Qronicle opened this issue 2 years ago • 8 comments

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

Qronicle avatar Aug 28 '23 15:08 Qronicle

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_');
      });

nikophil avatar Aug 28 '23 16:08 nikophil

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

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

kbond avatar Aug 28 '23 16:08 kbond

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

nikophil avatar Aug 28 '23 17:08 nikophil

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

Qronicle avatar Aug 28 '23 17:08 Qronicle

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

nikophil avatar Aug 28 '23 17:08 nikophil

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?

kbond avatar Aug 28 '23 17:08 kbond

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.)

Qronicle avatar Aug 28 '23 17:08 Qronicle

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

kbond avatar Aug 28 '23 17:08 kbond