Reset the KEY_BLOCK_SIZE when migrating the MySQL engine and row format

[aschempp]

I have a Contao 4.6 setup on a server that did not support InnoDB. Therefore, I configured the DBAL like so which worked for about 2 years now.

doctrine:
    dbal:
        connections:
            default:
                charset: utf8
                default_table_options:
                    charset: utf8
                    collate: utf8_unicode_ci
                    engine: MyISAM

I could finally convince the client to move to a good hoster and also do an update to Contao 4.9. First step was to move the existing installation to the new hoster, on Contao 4.6 and with this DBAL configuration. Then I updated to Contao 4.9 and did run contao:migrate, which fails at migrating the tl_files table:

ALTER TABLE tl_files ENGINE = InnoDB ROW_FORMAT = DYNAMIC......FAILED
[ERROR] An exception occurred while executing 'ALTER TABLE tl_files ENGINE = InnoDB ROW_FORMAT = DYNAMIC':
SQLSTATE[HY000]: General error: 1031 Table storage engine for '#sql-10b9_2e20d0a' doesn't have this option

After a suggestion from @ausi I tried to set ROW_FORMAT=DEFAULT before changing the engine. Changing the row format worked, but I got a new error after that (this time in german due to phpMyAdmin).

error 1478 - Speicher-Engine 'InnoDB' der Tabelle unterstützt die Option 'KEY_BLOCK_SIZE' nicht

I did some digging on the internet and found the following to MySQL issues:

• https://bugs.mysql.com/bug.php?id=88843
• https://bugs.mysql.com/bug.php?id=56628

Apparently, my database table has a KEY_BLOCK_SIZE defined, but that's not supported by an DYNAMIC (uncompressed) row format. According to the second bug report, setting KEY_BLOCK_SIZE=0 is the correct way to remove that setting since MySQL 5.5.9

[ausi]

Is setting KEY_BLOCK_SIZE supported in MySQL 5.1 too, or would this result in an error?

[aschempp]

According to https://bugs.mysql.com/bug.php?id=56628 it has been fixed 5.1.55 and 5.5.9. Before that fix, it did trigger a warning for invalid value, but the keyword seems valid.

[m-vo]

Before that fix, it did trigger a warning for invalid value, but the keyword seems valid

Is this also true when running in strict mode? And with MariaDB? 🙈

[aschempp]

🤷 we'll never know…

[ausi]

I think we should check for false !== stripos($tableOptions->Create_options, 'key_block_size=') and only then add KEY_BLOCK_SIZE=0 to the query. This should still fix the issue but not create any warnings or errors.

[leofeyer]

Is this also true when running in strict mode? And with MariaDB? 🙈

🤷 we'll never know…

Then we cannot merge this PR, can we? We would risk breaking installations.

[ausi]

Then we cannot merge this PR, can we? We would risk breaking installations.

I think we won’t break anything if we check the create options of the table as I suggested in #2363 (comment)

If the KEY_BLOCK_SIZE is already present in the create options, it should be save to unset it (by setting it to 0).

[leofeyer]

I think we should check for false !== stripos($tableOptions->Create_options, 'key_block_size=') and only then add KEY_BLOCK_SIZE=0 to the query. This should still fix the issue but not create any warnings or errors.

See #2363 (comment)

[aschempp]

PR updated accordingly. I have tested this on my customer's database, which worked correctly. To adjust the unit tests, I had to rewrite the getInstaller to use the $fromSchema as source of truth instead of varying the configuration based on table name. Also noticed that the installation-bundle was missing in the phpunit.xml.dist, therefore coverage was not generated for it.

[leofeyer]

Also noticed that the installation-bundle was missing in the phpunit.xml.dist, therefore coverage was not generated for it.

That is intended.

[aschempp]

Can you explain why that would be intended? Except for we're too lazy to write tests 😝

[leofeyer]

The bundle is deprecated and will hopefully be replaced by the Contao Manager soon, therefore we had decided not to write tests and not to track coverage.

[aschempp]

that sounds fair. But doesn't that mean the installer should be moved to the core-bundle then?

[leofeyer]

Probably yes. I think there is a ticket for that already.