[WIP] Fix MariaDB reserved keywords issue

[ausi]

See contao/core-bundle#918

[ausi]

To be honest: I don’t really like this approach. Running replacements via regular expressions on database queries is generally not a good idea.

A better idea may be to replace the Doctrine\DBAL\Platforms\MySqlPlatform and overwriting the getReservedKeywordsClass() method. @contao/developers what do you think?

Anyway, I think we should apply this fix only to version 4.4, and remove it again in 4.5.

[aschempp]

what if someone already uses PHP 7.1 and therefore DBAL 2.6? Would'nt the name already be escaped?

[ausi]

Yes, it would. But the regular expression wouldn’t match then. I will add unit tests for these cases after we agree which way we want to go.

[aschempp]

A better idea may be to replace the Doctrine\DBAL\Platforms\MySqlPlatform and overwriting the getReservedKeywordsClass() method. @contao/developers what do you think?

Not really gonna work, the method is final public in AbstractPlatform 😢

[Toflar]

I'm still not sure we should fix this issue at all. This issue only happens in the latest MariaDB. So I don't understand why you would want to use the latest MariaDB with an old PHP version? Just update to PHP 7.1 and it will all just work fine?

[aschempp]

Just update to PHP 7.1 and it will all just work fine?

Yeah, apart from the fact that some users can't control that, and therefore will run into random errors...

[Toflar]

That's such an edge case, really. Why in the world would any provider update their DB stack but not their php stack. That makes no sense at all.

[ausi]

I think several people already had this problem as discussed in contao/core-bundle#918. So it’s probably not an edge case...

[stefansl]

Why in the world would any provider update their DB stack but not their php

They do update php as well. But they leave the option to choose an older php version. ;) Imagine, someone has chosen php 5.6 and the provider updates the system. The php version is still the old one.

[ausi]

A better idea may be to replace the Doctrine\DBAL\Platforms\MySqlPlatform and overwriting the getReservedKeywordsClass() method. @contao/developers what do you think?

Not really gonna work, the method is final public in AbstractPlatform 😢

It’s protected, not final: https://github.com/doctrine/dbal/blob/v2.5.13/lib/Doctrine/DBAL/Platforms/MySqlPlatform.php#L1034

But I’m not sure how easy it is to replace the platform service in the doctrine configuration and if it is a good idea to do so.

[Toflar]

I have a bad gut feeling about that regex escaping to be honest. Feels like this can cause more issues than it solves.

I know that this PR aims to fix the schema update but you can still have that problem if any extension queries like SELECT rows FROM table instead of SELECT `rows` FROM table. So there is no way we can ensure people will "not run into random errors". It's not an issue caused by Contao nor can we control it. It's an issue caused by updates to the database engine and that's why we should ignore that.

[fritzmg]

Just update to PHP 7.1 and it will all just work fine?

Some projects might have dependencies, that will prevent composer to update to doctrine/dbal 2.6.x. Or projects might have dependencies, that do not run on PHP 7.1 (yet).

Just saying, not that I care that much about it ;). In general I agree with @Toflar

[Toflar]

Again. This PR does not solve the issue at all. It tries to solve it for schema updates which would already be covered if you had php 7.1. Still there might be hundreds of extensions that query on a now reserved word without quoting it and we cannot solve the problem for them. Everyone has to solve it individually. It's not a Contao problem. It does not need to be "fixed" by Contao.

[aschempp]

It’s protected, not final: https://github.com/doctrine/dbal/blob/v2.5.13/lib/Doctrine/DBAL/Platforms/MySqlPlatform.php#L1034

You're right, I was looking at getReserverdKeywordsList: https://github.com/doctrine/dbal/blob/v2.5.13/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php#L3479

But I’m not sure how easy it is to replace the platform service in the doctrine configuration and if it is a good idea to do so.

Replacing the platform is easy and a supported use-case as far as I know, but it would obviously affect the whole Symfony setup and not just Contao. Not sure if we want to do that (same discussion as with all the other Symfony services).

Another option would be replace just the keywords class, but that would require reflections. Not that it would matter, as everything in Doctrine (ORM) uses reflections already 😂

I have a bad gut feeling about that regex escaping to be honest. Feels like this can cause more issues than it solves.

Be aware that the database installer already uses tons of regex to convert the Doctrine Schema to our readable instructions. No real issue for me honestly.

[ausi]

Still there might be hundreds of extensions that query on a now reserved word without quoting it and we cannot solve the problem for them. Everyone has to solve it individually. It's not a Contao problem. It does not need to be "fixed" by Contao.

Contao itself uses column names unquoted in many many places, so it has to be fixed by Contao IMO. Either by not using reserved words as column names, or by escaping column names everywhere.

[aschempp]

Either by not using reserved words as column names, or by escaping column names everywhere.

Correct. And the only update-safe implementation is escaping. Because either we might need to rename over and over again, or just add a new reserved keyword to an existing escaping method.

[ausi]

With contao/core-bundle#1262 we now escape the column names in Contao properly. As discussed, we don’t want to hack in support for using MariaDB 10.2.4 with PHP 5, so I’m closing this PR.

For the record, the following setups should work from now on:

• Contao 4.4 with PHP 7 and any MariaDB-Version
• Contao 4.4 with PHP 5 and MariaDB 10.2.3 and lower
• Contao 3.5 with any PHP- and any MariaDB-Version

The only setup that is not supported is:

• Contao 4.4 with PHP 5 and MariaDB 10.2.4 or higher