Diff Feature: Why is require-table enabled?

[robertscherer]

Hi,

first off thanks for the diff feature - this fits our workflow perfectly.

One thing I stumbled upon while playing with it was that the schema-dump-default.lock contains just tables with have an ORM Table object in the App\Model\Table namespace.

This happens because in Migrations\Command\Dump::execute() the require-table param is hardcoded to true.

Is there a reason for this?

Thanks

[lorenzo]

I agree, it should probably default to the same as the rest of the commands. I was surprised by this feature yesterday when doing the workshops

[HavokInspiration]

The main reason it is forced is because of tables that could be created from plugins.
Using the Table classes is the only way to ensure we won't add tables from plugins to diff.
I have yet to find a better way to deal with this.

It can become an option, that would be fine with me. It will be inline with baking a snapshot.
The logic for baking a snapshot is the following : if you bake for the app, it will take all tables unless you set the require-table to true. If you bake for a plugin however, it will always force the require-table to true, to make sure it only bakes for plugin.

I can add the option and remove the hardcoded value if you would rather have the same behavior 😄
I agree it would be better since it would bring consistency.

[HavokInspiration]

Giving this more thought, it should be known that when migrating or rolling back, when the dump is created it will add all tables found in the dump file.
That would mean that we might need to add a require-table kind of option to migrate and rollback as well ?
Adding it to the dump command and the baking diff class should be ok, but to migrate and rollback, I'm not so sure...

Unless I'm over thinking the problem with plugins tables... But that is something that could happen.

[robertscherer]

These are good points, I didn't think of the rollback scenario. Still, usually if we talk about a dump we mean the whole database.

I don't think you're overthinking this, but also I'm not sure if there's an easy solution here.

[lorenzo]

I understand that sometimes tables are created from plugins... but I still think it is a better default to follow the rest of the commands and not require the table classes to be present.

[HavokInspiration]

I can add it in. I have a feeling there will be some case where I'll need your opinion for this @lorenzo.
I might ping you on slack.

I'll finish the work for #212 and take care of this.

[ypnos-web]

I tried this feature today to find that many tables in my database where part of the diff. This does not make any sense as my initial migration file already contains all these tables (created via migration_snapshot).

Consider doing this:

bin/cake bake migration_snapshot Initial
bin/cake bake migration_diff NothingReallyChanged

The current situation is that NothingReallyChanged will re-create all non-class tables.
I understand that you are already working on resolving this issue, just wanted to suggest you ensure that the default behavior of these two commands will be consistent.

[HavokInspiration]

@ypnos-web It actually should not be.
If you have a snapshot, the tables it contains should be in the dump (generated after baking the snapshot) and excluded from the diff (unless there were changes made to it).
Unless you baked before the feature was implemented in which case you need to generate a dump (see http://book.cakephp.org/3.0/en/migrations.html#generating-a-diff-between-two-database-states and http://book.cakephp.org/3.0/en/migrations.html#dump-generating-a-dump-file-for-the-diff-baking-feature)

However, I'm curious to see the files in question to try to shed some light on this.

[ypnos-web]

I had run a migrate command before which created the dump file. But I also tried this again today:

johannes@boron /s/h/L/app> bin/cake migrations dump

Welcome to CakePHP v3.2.10 Console
---------------------------------------------------------------
App : src
Path: /srv/http/LIMS/app/src/
PHP : 7.0.7
---------------------------------------------------------------
using migration path /srv/http/LIMS/app/config/Migrations
using seed path /srv/http/LIMS/app/config/Seeds
Writing dump file `/srv/http/LIMS/app/config/Migrations/schema-dump-default.lock`...
Dump file `/srv/http/LIMS/app/config/Migrations/schema-dump-default.lock` was successfully written
johannes@boron /s/h/L/app> bin/cake bake migration_diff Tester

Welcome to CakePHP v3.2.10 Console
---------------------------------------------------------------
App : src
Path: /srv/http/LIMS/app/src/
PHP : 7.0.7
---------------------------------------------------------------

Creating file /srv/http/LIMS/app/config/Migrations/20160608131539_Tester.php
Wrote `/srv/http/LIMS/app/config/Migrations/20160608131539_Tester.php`
Marking the migration 20160608131539_Tester as migrated...
using migration path /srv/http/LIMS/app/config/Migrations
using seed path /srv/http/LIMS/app/config/Seeds
Migration `20160608131539` successfully marked migrated !
Creating a dump of the new database state...
using migration path /srv/http/LIMS/app/config/Migrations
using seed path /srv/http/LIMS/app/config/Seeds
Writing dump file `/srv/http/LIMS/app/config/Migrations/schema-dump-default.lock`...
Dump file `/srv/http/LIMS/app/config/Migrations/schema-dump-default.lock` was successfully written

The result is the same: the Tester migration creates all tables without classes in up() and drops all of them in down().

Which files in particular are you interested in? Here are the dump file and the Tester migration.

schema-dump-default.txt
20160608131539_Tester.txt

[HavokInspiration]

@ypnos-web I need to understand your setup a bit more.

I need :

• a list of all your tables. Mark the one from a plugin (with the name of the plugin) and from your app.
• list of tables that have a model Table class

And what did you do ? Bake a snapshot ? And after that, you just baked a diff ?

Basically, I need to know your setup and each step you did so I can reproduce the issue and know if that comes from the require-table option.

[ypnos-web]

Here is a full table list:

* comments
  departments
* files
  i18n
* material_datas
* materials
  materials_sources
  materials_tasks
  materialtype_fields
* materialtypes
  mime_types
x phinxlog
  sop_attachment_types
  sop_attachments
  sop_materialtypes
  sop_materialtypes_resulting_materialtypes
  sop_resulting_materialtypes
  sops
  steps
  steps_tasks
  systemgroups
  task_files
  tasks
  unit_groups
* units
* users
* visits

All tables marked with * also have a table class. And I already checked for you that these are exactly the tables not present in the Tester migration. The phinxlog table also is not in the Tester migration.

What I did was, bake an initial migration snapshot, bake and apply a few migrations on top of it (affecting Visits and Files tables), then running migrate once after updating to the new migrations plugin version which obtained me a dumpfile, then baking the diff. Afterwards, what I had written in my last comment.

(In case you are wondering, most of these tables are legacy from a previous Cake2 app and the parts of the app corresponding to them had not been ported yet. Only a few tables without classes are actually used by the app)

[HavokInspiration]

@ypnos-web Are you using Windows by any chance ?

Strike that one, I was able to reproduce the behavior you are experiencing on my Linux system.

[HavokInspiration]

I opened PR #248.
It should fix the issue you have @ypnos-web.

This PR sets the require-table option to false and should solve the questions raised in this ticket.