Make table options configurable instead of forcing a default

[m-vo]

Haste sets the collation to MyISAM with character set UTF-8, which currently throws an exception on database update in my setup ('utf8mb4_unicode_ci' is not valid for CHARACTER SET 'utf8'). Imo the collation should not be forced to MyISAM in the first place and this line could be dropped. :-)

[qzminski]

@aschempp @Toflar what do you think?

[Toflar]

I agree with Moritz 😄 Contao should have a default and we should take that. If it does not add it we should check for the default values and add them ourselves. Also, maybe it would make sense to be able to explicitly define it via $arrRelation, wdyt @m-vo?

[m-vo]

Maybe that's a use case. I could update the PR accordingly. Would we want to specify the charset/collation or the SQL options in general? I don't think the engine is of any use, is it?

[Toflar]

Just thinking about Contao 4.5 where we switched to innodb so :)

[m-vo]

I'm not sure why anyone would still want to have MyISAM tables besides the case InnoDB isn't fully supported on the server (in which the defaults could be overwritten)? :-) It could of course be the other way round ifr someone needs transactional integrity for instance and can't accept MyISAM...

Btw.: The bug that this PR tries to circumvent originates from the fact that haste sets a charset without a collation which currently doesn't work the right way in Contao (see https://github.com/contao/core-bundle/pull/1289).

We could allow something like:

$GLOBALS['TL_DCA']['tl_table_one']['fields']['my_field']['relation'] = array
(
   // [...] 
   'tableSqlOptions' => "ENGINE=InnoDB DEFAULT CHARSET utf8 COLLATE utf8_general_ci",
);

Or semantically (probably preferrable):

$GLOBALS['TL_DCA']['tl_table_one']['fields']['my_field']['relation'] = array
(
   // [...] 
   'tableOptions' => [...] // Doctrine table options array
);

[fritzmg]

I'm not sure why anyone would still want to have MyISAM tables besides the case InnoDB isn't fully supported on the server (in which the defaults could be overwritten)? :-)

One reason is the limited key length in InnoDB, when

innodb_large_prefix = 1
innodb_file_format = Barracuda
innodb_file_per_table = 1

is not active. That's only a problem with long table fields or indices though and "only" with MySQL versions lower than 5.7.7 or MariaDB versions lower than 10.2.

I think you should definitely use the defaults from the used Contao version. So something like

$arrDefinitions[$arrRelation['table']]['TABLE_OPTIONS'] = $arrRelation['tableOptions'] ?: self::getDefaultTableOptions()

See also https://github.com/contao/managed-edition#innodb-large-prefix

[m-vo]

Using InnoDB without the options you mentioned breaks Contao anyway, doens't it? So a user hit by this limitation would set the defaults to MyISAM.

If we want to use the default we wouldn't need to specify the default table option?

[fritzmg]

Using InnoDB without the options you mentioned breaks Contao anyway, doens't it? So a user hit by this limitation would set the defaults to MyISAM.

Exactly.

If we want to use the default we wouldn't need to specify the default table option?

I don't know, may be it makes sense for the user to still be able to define custom table options. Not sure what the use case would be.

[m-vo]

I updated the PR accordingly.

Now it's possible to define table options like so:

    'relation' => [
        'type'     => 'haste-ManyToMany',
        'table'    => 'tl_table_two',
        'tableSql' => 'DEFAULT CHARSET=binary COLLATE binary ENGINE=MyISAM',
        // [...]

[Toflar]

So is Contao providing defaults now? Or will it fail in e.g. Contao 3.5?

[m-vo]

Hmm. Creating a table without options uses the MySQL database/server defaults. Tables will get converted though which could be a problem if someone has relied on utf8/MyISAM.

Do you know why has this been set or why it maybe was necessary at some time?

[Toflar]

No idea 😄