-
-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Fix MariaDB reserved keywords issue #1106
Conversation
LGTM! we need to check/fix the install tool / Database Updater too though. |
*/ | ||
public static function escapeColumnName($name) | ||
{ | ||
if (in_array($name, ['rows'], true)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we use the doctrine platform method here to quote everything we need (plus force rows
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't in_array($name, ['rows'], true)
the same as 'rows' === $name
? Or did you just want to avoid the Yoda condition. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
He just prepared for more in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ausi, what about this? (should work in 3.5 too):
// I'd name it "quote", not "escape"...
public static function quoteColumnName($name)
{
if (method_exists('Contao\System', 'getContainer')) {
$connection = Contao\System::getContainer()->get('connection');
if ($connection instanceof Doctrine\DBAL\Connection) {
$name = $connection->getDatabasePlatform()->quoteSingleIdentifier($name);
}
}
// Force quoting "rows" for older Contao versions
// (which will be ignored if already done by the platform in newer Contao versions)
if (in_array($name, ['rows'], true)) {
$name = '`'.$name.'`';
}
return $name;
}
We already discussed parts of this issue in contao/installation-bundle#70 and I think there are open questions we have to agree on first:
I would say YES to 1. and NO to 2. |
To me, renaming the column name is not an option. And then, there are 2 problems. One is the database schema and the other is querying the database. We need to fix querying anyway so this PR here is needed but imho can be improved as shown in my comment. |
Yes, if we want to support keywords as column names. But then we have to fix it everywhere in Contao (also in DC_Table for example). Do you think we should support keywords as table names too? Or only for columns and indexes? |
I do think reserved keywords have to be properly escaped. To me it's best practice to do so in all the places you are not in control of the name of the column (= where you don't write the query yourself). And it applies to all three of them, tables, columns and indexes although I tend to kind of ignore the tables. |
OK, then I will update this PR, so that we quote/escape all columns and indexes via doctrine. |
I agree, although it seems you have agreed on something else meanwhile. If so, never mind. 😄 |
I'm honestly not sure. Fixing this one issue is more important to me than adding a new feature to escape all fields in the deprecated database class... I would not invest too much time into this. |
Closed in favor of #1262 |
Description ----------- This is the port of contao/core#8813. Also see #1106 and contao/installation-bundle#70 Commits ------- 84c4746 Quote reserved words in database queries 7bf64b1 Use static instead of \Database as self-reference.
Description ----------- As discussed in contao/core-bundle#1106 and the Contao Mumble call, we want to escape all column names that are reserved keywords. In Contao 4 we will use `\System::getContainer()->get('connection')->getDatabasePlatform()->quoteSingleIdentifier($name);`. In Contao 3.5 (this pull request) we only quote `rows`. There is no need to fix the install tool in 3.5, as it uses quoted column names already. @leofeyer Do you have an automated workflow to port this pull request to Contao 4.4, or should I do it by hand? Commits ------- e33738a Quote reserved words in database queries 5d5c2af Rename quoteColumnName() to quoteIdentifier(). 2eefd3f Only quote identifiers in the find_in_set() methods. 57803a5 Make the column check case-insensitive again. 3b692c4 Remove the redundant regex check.
See #918