Database\Update::convertSingleField() overwrite changed values

[tristanlins]

The situation for extensions is a little bit different then for contao. Contao formally will check if the database upgrad to 3.2 is required and give this option only one time!

Extensions use the runonce.php which runs every time the extension is updated. According to #6459 (comment) such a runonce.php may look like:

\Database\Updater::convertSingleField('tl_theme_plus_javascript', 'file');
\Database\Updater::convertSingleField('tl_theme_plus_stylesheet', 'file');
\Database\Updater::convertSingleField('tl_theme_plus_variable', 'file');

Precondition:
for my test, I have added a stylesheet file to tl_theme_plus_stylesheet,
here is the row (I have stripped unnecessary fields):

+----+...+------+...
| id |...| file |...
+----+...+------+...
|  1 |...| 21   |...
+----+...+------+...

Now what will happen after the upgrade to 3.2 (the runonce.php is not executed)?

• Nothing changed, because the contao update skip extension fields.

What happen after the runonce.php run the first time?

• A field file_backup is created, storing the original value and file will be filled with the file's uuid.

+----+...+-------+...+-------------+
| id |...| file  |...| file_backup |
+----+...+-------+...+-------------+
|  1 |...| 12345 |...| 21          |
+----+...+-------+...+-------------+

PS: I replace the uuid with a human readable value just for this example.

Until now, everything is fine.
Now we work on the system, change the file selection and after some time, the row looks like this:

+----+...+-------+...+-------------+
| id |...| file  |...| file_backup |
+----+...+-------+...+-------------+
|  1 |...| 67890 |...| 21          |
+----+...+-------+...+-------------+

But now, we upgrade the extension and the runonce.php is executed a second time. What happen?

Situation 1

Precondition: We change the selection on the field file.
Precondition: We do not remove the file_backup field in the database backup.
Let's have a look

public static function convertSingleField($table, $field)
{
    $backup = $field . '_backup';
    $objDatabase = \Database::getInstance();

    // Backup the original column and then change the column type
    if (!$objDatabase->fieldExists($backup, $table, true))
    {
        // NOTE: The field **will not** be recreated, because it still exists.
        $objDatabase->query("ALTER TABLE `$table` ADD `$backup` varchar(255) NOT NULL default ''");
        $objDatabase->query("UPDATE `$table` SET `$backup`=`$field`");
        $objDatabase->query("ALTER TABLE `$table` CHANGE `$field` `$field` binary(16) NULL");
        $objDatabase->query("UPDATE `$table` SET `$field`=NULL");
    }

    // NOTE: search for all non-empty backup_field-rows, which will still found ALL rows
    $objRow = $objDatabase->query("SELECT id, $backup FROM $table WHERE $backup!=''");

    while ($objRow->next())
    {
        // Numeric ID to UUID
        if (is_numeric($objRow->$backup))
        {
            // NOTE: find the file by the backup ID (in my case 21), which will return the row with the uuid 12345
            $objFile = \FilesModel::findByPk($objRow->$backup);

            // NOTE: this will overwrite the updated value $field=67890 with the value from the update 12345
            $objDatabase->prepare("UPDATE $table SET $field=? WHERE id=?")
                        ->execute($objFile->uuid, $objRow->id);
        }

        // NOTE: not tested, but should still the same as if $backup is numeric
        // Path to UUID
        else
        {
            $objFile = \FilesModel::findByPath($objRow->$backup);

            $objDatabase->prepare("UPDATE $table SET $field=? WHERE id=?")
                        ->execute($objFile->uuid, $objRow->id);
        }
    }
}

After all, the row is:

+----+...+-------+...+-------------+
| id |...| file  |...| file_backup |
+----+...+-------+...+-------------+
|  1 |...| 12345 |...| 21          |
+----+...+-------+...+-------------+

As you can see, we lost the new selected value file=67890.

Situation 2

Precondition: We change the selection on the field file.
Precondition: We remove the file with the ID 21 from file manager.

Nearly the same as Situation 1 expect that the search for the previous value will not find the file record with ID 21. So the file field will be cleared:

+----+...+-------+...+-------------+
| id |...| file  |...| file_backup |
+----+...+-------+...+-------------+
|  1 |...| NULL  |...| 21          |
+----+...+-------+...+-------------+

As you can see, we completely lost the file value.

Situation 3

Precondition: We remove the file_backup field in the database backup.

In short:

• convertSingleField recreate the field file_backup.
• convertSingleField write the UUID (especially only the varchar-valid chars) into file_backup.
• convertSingleField clear the file field

The row now looks like:

+----+...+-------+...+-------------+
| id |...| file  |...| file_backup |
+----+...+-------+...+-------------+
|  1 |...| NULL  |...| "ԥUV        |
+----+...+-------+...+-------------+

• convertSingleField try to find the file with path "ԥUV, found nothing and reset file with NULL.

Same as Situation 2, we completely lost the file value, also we lost the original file_backup value.

Conclusion

The Database\Update::convertSingleField() must check if the update is realy necessary. I don't see the point why every extension need to do the same check.

Hotfix for developers

Developers need to check if the update is necessary, like this:

Hint: please see #6481 (comment) for more details!

$desc = \Database::getInstance()->query('DESC tl_theme_plus_stylesheet file');
if ($desc->Type != 'binary(16)') {
    \Database\Updater::convertSingleField('tl_theme_plus_stylesheet', 'file');
}

[tristanlins]

After a short review, the Database\Update::convertMultiField() and Database\Update::convertOrderField() method have the same problem imo.

[tristanlins]

I need to improve my update check, I cannot go on the type only. Especially Theme+ 4.3 is dual compatible for 3.1 and 3.2 but use varbinary(16) even on 3.1 (simply because varbinary(16) can also hold an integer from 3.1).
In the update, I need to check if there are still records with numeric IDs exist.

$desc = \Database::getInstance()->query('DESC ' . $table . ' file');
$stillNumericRecordCount = \Database::getInstance()
    ->query('SELECT COUNT(id) AS count FROM tl_theme_plus_stylesheet WHERE file REGEXP \'^[0-9]+$\'')
    ->count;
if ($desc->Type != 'binary(16)' || $stillNumericRecordCount) {
    \Database\Updater::convertSingleField($table, 'file');
}

[leofeyer]

Ich antworte mal auf Deutsch, damit es nicht durch die Übersetzung noch zusätzlich Verwirrung gibt.

Ich bin auch der Meinung, dass die Lösung nicht optimal ist. Wir haben dazu schon in der AG Core diskutiert und auch auf dem Camp in großer und kleiner Runde. Leider haben wir keine bessere Lösung gefunden.

Das Problem ist, dass die "fileTree"-Felder identifiziert werden müssen. Diese Information befindet sich in den DCA-Dateien, als müssen diese per loadDataContainer() geladen werden. Das wiederum funktioniert nur für den Core, da die Extensions zum Zeitpunkt des Updates noch nicht aktuell sind.

Die einzige Alternative wäre, die DCA-Dateien als String einzulesen und mit einem PHP-Parser nach den "fileTree"-Feldern zu suchen. Da die DCA-Dateien aber sehr unterschiedlich ausfallen, ist das keine verlässliche Methode.

Ich würde den Parsing-Ansatz trotzdem nochmal testen, wenn wir überwiegend der Meinung sind, dass wir das wollen.

[aschempp]

Ich bin absolut gegen ein parsing. Eine unzuverlässigere Variante ist schlimmer als wenn der Entwickler selber Hand anlegen muss...

[leofeyer]

Vielleicht schaffen wir es ja gemeinsam, dass es eine zuverlässige Variante wird?

[tristanlins]

Also es darf auf keinen Fall zu Datenverlust kommen, wenn die Methode mehrfach aufgerufen wird :-\

Eine Teillösung fällt mir gerade zum Überschreiben neuerer Werte ein:

$objRow = $objDatabase->query(
  "SELECT id, $backup FROM $table WHERE $field IS NULL AND $backup!=''"
);

[psi-4ward]

multiSRC nicht vergessen!

[BugBuster1701]

Ich versteh nicht ganz die Aufregung, es war doch bisher immer die Aufgabe innerhalb der runonce selbst zu prüfen, ob ein Update/Migration nötig ist oder nicht. Mach ich jedenfalls seit Jahren so. Daher ist bei mir beispielsweise eine Repair Installation überhaupt kein Problem.

Zu der anderen Geschichte, ihr sucht jetzt doch wieder nach einer Möglichkeit die externen Erweiterungen mit zu migrieren? Das geht doch gar nicht. Die SQL Definitionen in dessen DCA bleibt doch die alte. Der Vergleich beim DB aktualisieren würde doch sofort wieder zuschlagen und die Feldänderungen rückgängig machen wollen.
Oder verstehe ich da was falsch?

[psi-4ward]

Und noch n Test: https://gist.github.com/psi-4ward/7677634
Kopf voll

@BugBuster1701 Das Problem ist das mehrfache ausführen der convertSingleField() Funktion(en). Machst du es öfters führt es zu Datenverlust. Die runonce wird ja bei jedem Update deiner Erweiterung wieder eingespielt und somit passiert genau das.

[tristanlins]

@BugBuster1701 Es kann wohl kaum im Sinne der Extension Entwickler sein, dass wir alle unsere Runonces mit Prüfungen überladen müssen, die in der convertSingleField() zentral für die allgemein gültigen Fälle gelöst werden müssen weil die Methode convertSingleField() bei mehrfachem Aufruf durch die runonce.php sonst zu Datenverlust führt! Das wird jeden Entwickler treffen, der nicht gerade wie ich sich den Code der convertSingleField() genauer angeschaut hat.

Im übrigen habe ich in Theme+ einen Spezialfall, das gebe ich ja zu. Mein Feld field muss ein blob sein, weil ich dort nicht nur UUIDs ablege, sondern dort auch alternativ Pfade (außerhalb des upload dirs) drin stehen können. Daher werde ich diese Methode wohl nicht verwenden können, ich habe meine beiden Theme+ Versionen mittlerweile zurück gezogen.

[BugBuster1701]

Wie gesagt, ich prüfe so etwas seit Jahren, ob das nun meine eigene Methode ist (die auch nie mehrmals ausgeführt werden darf) oder eine vom Core, da mache ich keinen Unterschied. Natürlich ist es von Vorteil, wenn die convert* Methoden das selber abfangen.

[leofeyer]

Behoben in e3900ea. Vielen Dank für den Input und eure Zeit und nochmal Entschuldigung, dass wir uns von ein paar Wenigen trotz vieler Warnungen und Vorbehalte haben überreden lassen, die UUIDs im Binärformat zu speichern.

[leofeyer]

Leider funktioniert die Lösung noch immer nicht: e3900ea#commitcomment-4727911

Hab es eben auch selbst getestet:

mysql -e "SELECT CAST(UNHEX('3981ad92585111e3aedaf5cf2c70ab0a') AS SIGNED)"

+-----------------------------------------------------------+
| CAST(UNHEX('3981ad92585111e3aedaf5cf2c70ab0a') AS SIGNED) |
+-----------------------------------------------------------+
|                                                         9 |
+-----------------------------------------------------------+

[leofeyer]

Die Lösung ist glaube ich ganz einfach: Wir müssen zuerst rtrim($field, "\x00") machen und danach schauen, ob es ein Integer oder ein String ist.

// SELECT UNHEX('39000000000000000000000000000000') AS val 
rtrim($val, "\x00"); // string(1) "9"

// SELECT UNHEX('3981ad92585111e3aedaf5cf2c70ab0a') AS val 
rtrim($val, "\x00"); // string(16) "9�­’XQ�ã®ÚõÏ,p«
"

[leofeyer]

Sollte in 4728431 behoben sein. Test-Ergebnis:

stdClass Object
(
    [value] => 9
    [isUuid] => 
    [isNumeric] => 1
)
stdClass Object
(
    [value] => 9�­’XQ�ã®ÚõÏ,p«

    [isUuid] => 1
    [isNumeric] => 
)
stdClass Object
(
    [value] => files/m
    [isUuid] => 
    [isNumeric] => 
)