Implement database migrations

[k-pd]

• On homepage check for latest DB version
• Add migrations config setting
• Add read-only config vars to admin settings page
• Add initial DB migration to add DB version to admin settings
• Add "class" to action log type definitions
• Add "superadmin_deploy_failed" to action log types
• Enhance action log list output for "class"
• Log DB update errors in action log

[k-pd]

I chose opening the Homepage to trigger DB update checking. No matter who loads this page will trigger the checking. The DB migration files are collected in Config/sql/migrations/, starting with 0.sql to add the DB version to the configuration table. The version is a simple number, any file named higher than that number will be imported. There can be many files, they are imported in their numerical sequence. There may be gaps in the numbering, but the gaps cannot be filled later. So better avoid gaps.

Using a number indepent of the release version allows development of multiple features / issues and each having an individual migration file in it. There must be some instance to control the file numbering, so later releases do not contain lower numbers.

Very much care was taken for execution supervision - but solving problems cannot be automated. That means, if there is any problem preventing the autoupdate, someone must inspect the server / DB for why it doesn't work and solve the problem before re-starting the autoupdate.

[k-pd]

Oh, yes: I accidentally enabled "remove trailing whitespace" in my editor. So there is a lot of changes doing whitespace removal only. Sorry!

[mrothauer]

looks great! my first feedback:

1. please consider the cache clearing if debug mode == 0 /tmp/cache/models and/or /tmp/cache/persistent need to be cleared if db structure has changed - maybe Cache::clear() is enough.

2. if you place doDbMigrations() in FrontendController::beforeRender, it would be triggered on any frontend page call (not only the home) => IMHO this would also be fine

more feedback soon!

[k-pd]

FrontendController is for all Frontend pages? Is redirecting to '/' OK then? Didn't find my function being triggered on other pages than the Homepage...going to clear cache, too.

[mrothauer]

1. i reflected... i would trigger the migrations on any page call (also admin pages) beforeRender in AppController can be used for that. if the first call after an update is an admin call, there would be an error because the db schema does not fit to code. the redirect to home is not necessary i'd say.
   UPDATE: and maybe also trigger the migrations in AppShell.php - just in case a cronjob is the first action called after a version update. once the logic is in a component (see 3) it should be possible.

2. on my local branch i added a 1.sql with an error in it. i got the correct error message (DB update error), but when i then corrected the sql the error stayed and the migration was not executed. that's what i would expect.

3. the 4 new methods code in the frontendcontroller (which should - as mentioned above) be moved in appcontroller should be moved into a component to keep appcontroller clean.

4. a success message would also be nice "Die Datenbank wurde soeben auf die neueste Version aktualisiert." or something like that

5. whitespace... what's better: remove all trailing whitespace of whole project in one commit - or disable the feature in editors?

6. the whole feature is really really good! thanks a lot!

[k-pd]

To 1. The redirect is necessary, as cake loads the table information from DB on _construct() of Model Classes. This is why You don't have to define that on Model Implementation. So after possibly changing the whole structure of everything a reload must be done. By the way, it is not safely possible to change configuration table or action log table, as these are used during migration. Will move it to AppController.

To 2. The design is exactly like that. See this comment above. It is not designed to recover from errors. As a developer doing DB structure changes You are able to look into the DB and SQL to find an error. Then it's easy for You to reset the DB version and the last tried update version to before Your wrong version and thus re-enable auto-migration. Any user cannot do such a thing. If You release a working SQL for auto-migration, and on one installation there is a problem, You will not be able find the cause without digging into that particular DB. And when doing so, You will be able to also reset the DB version and get it back to auto-migrate.

To 3. I'll try that. But I think the whole auto-migration thing will move outside of cake soon. There are too many problems to change the running system from inside. So when doing something like Maintainance Mode, Installer or the like, this auto-migration should be taken into that maintainance system. Remember: it's a quick solution, not a perfect one.

To 4. I'll do that.

To 5. I didn't expect trailing whitespace at all :) Remove them from all files would be my choice, they are to be avoided anyways. Maybe there can be a filter hook on GitHub to do that automagically?

[k-pd]

OT: Please avoid adding additional wishes as Updates, there is no eMail triggered by editing a comment. I may miss it.

About adding to AppShell: How is re-running the Shell handled? As stated above re-starting is necessary to avoid Model - DB structure mismatching. And how about user informing about change success (errors will be seen on the frontend later, no problem here)?

[k-pd]

I cannot easily re-use the component, Shell uses Tasks that are "similar to components" but not the same. Requires some abstract class inbetween...

[mrothauer]

ad 1) ok
ad 2) ok - got it :-)
ad 3) cool that the code is not in app controller any more... maybe it's another issue to integrate migrations in a future auto-install and auto-update tool.
ad 4) great
ad 5) #38

integration in shells
maybe it's enough to just block the shells (and maybe inform the user about it), if the db migrations need to be executed. just to avoid clashes.

[mrothauer]

just added docblock comments, feel free to add you email address, you can also update the @author in the changed files if you want to

[mrothauer]

if it's easy to implement: change the migration file names from 0.sql, 1.sql and so on to date with time (20170315-230322) to avoid a confusing file list when opening the folder. because sorting will be alphabetically. or add two leading zeros, starting with 000.sql, then 001.sql if its easier.

0.sql
1.sql
11.sql
12.sql....19.sql
2.sql
21.sql

this could lead to not knowing how to name my new migration.

[k-pd]

That is a file browser thing, mine does "natural sorting"...which in turn has other issues. Right now many things highly depend on the version being an int. And having a non-gap int sequence also means, You cannot accidentally add a migration in the middle of the list which then will not be used. This turned me away from using date / time / anything else. I will pad the int with 0s though, but hopefully noone will mess it up with 00.sql and 0000.sql..

[k-pd]

Set @since to 1.3, because the files are not there when I install V1.2 from master...

[mrothauer]

all unit tests pass (had to fix one - was caused by another task - so update your branch if you want to run them). would a merge into develop be ok for you? or should i wait for something?