View Issue Details

IDProjectCategoryView StatusLast Update
17492Development Otherpublic2021-11-26 11:31
Reporterollehar Assigned Toollehar  
PrioritynoneSeverityminor 
Status testingResolutionopen 
Product Version5.x.0-dev 
Summary17492: Refactor updatedb helper into multiple command classes
Descriptionhttps://refactoring.guru/design-patterns/command/php/example

https://www.yiiframework.com/doc/guide/1.1/en/database.migration

https://www.yiiframework.com/doc/guide/2.0/en/db-migrations

Use-case:
* Avoid typos, forgotten transactions, forgotten dbvalue set
* Avoid merge conflicts when multiple updates are added in different branches
* Keep it framework agnostic to avoid Yii 1 --> 3 problems (but inspired by the Yii code)

NB: Using timestamp for dbversion instead of a small manual number is the same as manual number but with larger space between the number.
Ex: 450, 451 --> 450, 460 --> 2021450, 2021460 --> 20210101_180203, 20210201_152349
It has the same possible problem with ordering, but less chance of conflict.
Won't work with dev + master branch. Prefix timestamp with semver number? Ex: 5.1.0-20210101_180203 or 5_1_0_20210101_180203 (semver cannot be compared as a string, just use version_compare())
But needs to be int? So 05010020210101180223. Too long.
Internal hosting tools need to be adjusted if it's changed from int to string.

File: updatedb_helper.php

Function db_upgrade_all() signature stays exactly the same but is added to a new service class. The function body is refactored.

Algorithm:

1. Fetch all files in application/helpers/update/updates
2. Filter all updates that are lower than current dbversion in global settings
3. Sort in chronological order based on file and class name
4. Execute each command class inside a database transaction

class DatabaseUpdateHelper
{
  public function upgradeAll($oldDbVersion, $silent = false)
  {
    $updates = $this->getAllUpdates();
    $updates = $this->filterAlreadyUpdatedUpdates($updates);
    foreach ($updates as $update) {
      $transaction = $db->beginTransaction();
      try {
        $update->safeUp();
      } catch (Exception $ex) {
        $transaction->rollback();
        // Abort update and show error message
      }
      $transaction->commit();
      $this->setDbversion($update->getVersion());
    }
  }
}

Interface:

interface DatabaseUpdateInterface
{
  public function __construct(CDbConnection $db);
  public function safeUp(): void;
  public function isCritical(): bool; // TODO: Maybe replace with base class with default false?
}

Alternative base class:

abstract class DatabaseUpdateBase
{
  public $isCritical = false;
  public $db;
  public function __construct(CDbConnection $db)
  {
    $this->db = $db;
  }
  public function safeUp();
}

Example class:

Class name is DatabaseUpdate_ + timestamp when update was created. This timestamp replaces dbversion number to avoid merge conflicts when multiple updates are added.

// TODO: Inherit from CDbMigration? It has all helper methods like dropIndex(), addPrimaryKey(), etc
// TODO: Extend CDbMigration to YiiDbMigration, adding criticalUpdate : bool? Maybe also remove or block unused functions (always use safeUp, etc)
// TODO: Depending on Yii class might make it harder to move to Yii 2 or 3
// NB: Yii 1 code to generate migration class name: $name='m'.gmdate('ymd_His').'_'.$name;
// (framework/cli/commands/MigrateCommand.php)
final class DatabaseUpdate_202101011000 implements DatabaseUpdateInterface
{
  public function __construct(CDbConnection $db)
  {
    $this->db = $db;
  }

  public function safeUp()
  {
        addColumn('{{surveys}}', 'usetokens', "string(1) NOT NULL default 'N'");
        addColumn('{{surveys}}', 'attributedescriptions', 'text');
        dropColumn('{{surveys}}', 'attribute1');
        dropColumn('{{surveys}}', 'attribute2');
        $this->upgradeTokenTables134();
  }

  public function isCritical()
  {
    return false;
  }

  private function upgradeTokenTables134()
  {
    $atables = dbgettableslike("tokens%");
    foreach ($atables as $stable) {
        addcolumn($stable, 'validfrom', "datetime");
        addcolumn($stable, 'validuntil', "datetime");
    }
  }
}

A Yii command is added to generate the scaffold class for a new database update, including the timestamp class name.

$ php yii command/create-new-update
Generating class DatabaseUpdate_202107171800...
TagsNo tags attached.

Users monitoring this issue

User List There are no users monitoring this issue.

Activities

gabrieljenik

gabrieljenik

2021-08-04 14:33

manager   ~65808

About the refactor, make sense to include each migration in its own file. I like the general approach.

> Filter all updates that are lower than current dbversion in global settings
How this should be accomplished? What's your view?

> It has the same possible problem with ordering, but less chance of conflict.
Still, I would take the oportunity to review how to get rid of the incremental counters (timestamps as well), as to avoid merge bumps.
Maybe we can review some best practices available?


---


I know laravel works with migrations.
But that requires keeping track of all applied patches, which, I am not a big fan of.

I have some ideas about changing:
From: Apply this if dbversion is lower than XXX
To: Apply this if major dbversion is lower than XXX and (optionally) minor dbversion is lower than YYY. Only a few scripts would move the minor version.

That would avoid most of the collisions: collisions of independent migrations.
If we need to coordinate 2 migrations, we can afford the merge bumps maybe.

Just a thought. We can ellaborate more.

-----

Another idea:
- Place all migrations on
gabrieljenik

gabrieljenik

2021-08-04 14:38

manager   ~65810

> Filter all updates that are lower than current dbversion in global settings
> How this should be accomplished? What's your view?

Maybe we can name files with the base dbversion they are based on.
That should be quick. It is like the major and minor version I was suggesting, but taken to timestamps.
ollehar

ollehar

2021-08-04 14:51

administrator   ~65814

> I know laravel works with migrations. But that requires keeping track of all applied patches, which, I am not a big fan of.

Check the Yii links at the top.

I wouldn't mind logging all applied migrations to see if anything missed. Not a big effort.
gabrieljenik

gabrieljenik

2021-08-04 15:02

manager   ~65816

> I wouldn't mind logging all applied migrations to see if anything missed. Not a big effort.
I see this as error prone.
If something missed, maybe I would alert, not apply automatically.

> Check the Yii links at the top.
Read them. What should I focus on in those links? In refrence to what comment you are mentioning it?
ollehar

ollehar

2021-08-04 15:03

administrator   ~65818

> Read them. What should I focus on in those links? In refrence to what comment you are mentioning it?

Sorry, I meant in relation to Laravel migrations. :) Yii migrations does not take into account different branches, tho. Don't know if Laravel does. Can google it.
ollehar

ollehar

2021-08-06 11:44

administrator   ~65872

Other ideas:

* Make it possible to do *both* only dbversion number, OR dbversion number + timestamp (for smaller fixes).
* Put updates in different folders, one folder for patches, another for new features and structural changes to database (discussion here: https://forum.yiiframework.com/t/database-migrations-and-dev-master-branch/132658/5)
ollehar

ollehar

2021-08-13 15:13

administrator   ~65962

The command class could add an assert() function to check for post-conditions visible when debug = 2.
ollehar

ollehar

2021-08-13 15:18

administrator   ~65964

Another thing to consider is logging (via Yii) to make debugging easier.

Tear-down will not be supported.
ollehar

ollehar

2021-08-19 11:27

administrator   ~66066

NB: Split big updates into smaller parts to be able to trace problems?

NB: Use floats for dbversion to designate parts of an update?
gabrieljenik

gabrieljenik

2021-08-19 14:20

manager   ~66073

> NB: Split big updates into smaller parts to be able to trace problems?

An example?

> NB: Use floats for dbversion to designate parts of an update?

Could be a good idea for the update helper. We need to see how to include that requirement in the whole package
ollehar

ollehar

2021-08-19 14:21

administrator   ~66075

> An example?

Not sure, actually. The problem is with auto-commit, which happens when you do addColumn, dropColumn and other structural changes. If the db update then breaks in the middle, you're screwed and have to either restore the database or manually revert it.
gabrieljenik

gabrieljenik

2021-08-19 16:04

manager   ~66078

Ohh those updates you mean. Good. We can put them ina different DB version.
ollehar

ollehar

2021-11-15 17:24

administrator   ~67334

Started to work on this on branch dev/17492-refactor-updatedb
ollehar

ollehar

2021-11-16 14:34

administrator   ~67342

Last edited: 2021-11-16 15:40

View 2 revisions

Vim macro with writing to file:

:call writefile(getreg("", 1, 1), "Update_" . getreg("a") . ".php")

Delete first line (begin transaction) in

ls | xargs -i sed -i '1d' {}

Delete last two lines of all update files:

ls | xargs -i sed -i -e :a -e '$d;N;2,2ba' -e 'P;D' {}

Replace oDB with this->db in all update files:

ls | xargs -i sed -i 's/oDB/this->db/g' {}
ollehar

ollehar

2021-11-16 14:37

administrator   ~67343

codegen.php (646 bytes)   
<?php

// Small script to fix update files.

$file = $argv[1];
$lines = file($file);
$parts = explode('_', $file);
$dbVersion = (int) $parts[1];

$lines = array_merge(
    array_map(
        fn ($l) => $l . PHP_EOL,
        [
            '<?php',
            '',
            'namespace LimeSurvey\Helpers\Update;',
            '',
            sprintf('class Update_%d extends DatabaseUpdateBase', $dbVersion),
            '{',
            '    public function run()',
            '    {',
        ],
    ),
    $lines
);

$lines = array_merge(
    $lines,
    [
        '    }' . PHP_EOL,
        '}'
    ],
);

file_put_contents($file, $lines);
codegen.php (646 bytes)   
gabrieljenik

gabrieljenik

2021-11-16 15:14

manager   ~67345

Love this code generation scripts!
ollehar

ollehar

2021-11-16 15:34

administrator   ~67346

Last edited: 2021-11-16 15:39

View 2 revisions

^^
ollehar

ollehar

2021-11-16 18:28

administrator   ~67347

TODO: Logging

TODO: Make it possible to mock db connection
DenisChenu

DenisChenu

2021-11-26 11:12

developer   ~67584

codegen !!!
ollehar

ollehar

2021-11-26 11:31

administrator   ~67585

Hack the planet! :D

Issue History

Date Modified Username Field Change
2021-08-02 20:17 ollehar New Issue
2021-08-02 20:23 ollehar Description Updated View Revisions
2021-08-02 20:27 ollehar Description Updated View Revisions
2021-08-02 20:29 ollehar Description Updated View Revisions
2021-08-02 20:33 ollehar Description Updated View Revisions
2021-08-02 20:35 ollehar Description Updated View Revisions
2021-08-02 20:36 ollehar Description Updated View Revisions
2021-08-02 20:37 ollehar Description Updated View Revisions
2021-08-02 20:39 ollehar Description Updated View Revisions
2021-08-02 20:41 ollehar Description Updated View Revisions
2021-08-02 20:41 ollehar Product Version => 5.x.0-dev
2021-08-02 20:42 ollehar Description Updated View Revisions
2021-08-02 20:43 ollehar Description Updated View Revisions
2021-08-02 20:51 ollehar Description Updated View Revisions
2021-08-02 20:51 ollehar Description Updated View Revisions
2021-08-02 20:52 ollehar Description Updated View Revisions
2021-08-02 20:53 ollehar Sync to Zoho Project => |Yes|
2021-08-02 20:55 ollehar Description Updated View Revisions
2021-08-02 20:55 ollehar Sync to Zoho Project Yes => |Yes|
2021-08-02 20:58 ollehar Description Updated View Revisions
2021-08-02 20:58 ollehar Sync to Zoho Project Yes => |Yes|
2021-08-02 21:04 ollehar Description Updated View Revisions
2021-08-02 21:04 ollehar Sync to Zoho Project Yes => |Yes|
2021-08-03 15:04 galads Assigned To => ollehar
2021-08-03 15:04 galads Status new => assigned
2021-08-04 14:00 ollehar Description Updated View Revisions
2021-08-04 14:00 ollehar Sync to Zoho Project Yes => |Yes|
2021-08-04 14:01 ollehar Description Updated View Revisions
2021-08-04 14:01 ollehar Sync to Zoho Project Yes => |Yes|
2021-08-04 14:23 ollehar Description Updated View Revisions
2021-08-04 14:23 ollehar Sync to Zoho Project Yes => |Yes|
2021-08-04 14:26 ollehar Description Updated View Revisions
2021-08-04 14:26 ollehar Sync to Zoho Project Yes => |Yes|
2021-08-04 14:33 gabrieljenik Note Added: 65808
2021-08-04 14:38 gabrieljenik Note Added: 65810
2021-08-04 14:51 ollehar Note Added: 65814
2021-08-04 15:02 gabrieljenik Note Added: 65816
2021-08-04 15:03 ollehar Note Added: 65818
2021-08-06 11:44 ollehar Note Added: 65872
2021-08-13 15:13 ollehar Note Added: 65962
2021-08-13 15:18 ollehar Note Added: 65964
2021-08-19 11:27 ollehar Note Added: 66066
2021-08-19 14:20 gabrieljenik Note Added: 66073
2021-08-19 14:21 ollehar Note Added: 66075
2021-08-19 16:04 gabrieljenik Note Added: 66078
2021-11-15 17:24 ollehar Note Added: 67334
2021-11-16 14:34 ollehar Note Added: 67342
2021-11-16 14:37 ollehar Note Added: 67343
2021-11-16 14:37 ollehar File Added: codegen.php
2021-11-16 15:14 gabrieljenik Note Added: 67345
2021-11-16 15:34 ollehar Note Added: 67346
2021-11-16 15:39 ollehar Note Edited: 67346 View Revisions
2021-11-16 15:40 ollehar Note Edited: 67342 View Revisions
2021-11-16 18:28 ollehar Note Added: 67347
2021-11-17 11:46 ollehar Status assigned => testing
2021-11-26 11:12 DenisChenu Note Added: 67584
2021-11-26 11:31 ollehar Note Added: 67585