Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

ausi
Copy link
Member

@ausi ausi commented Sep 25, 2017

See #918

@aschempp
Copy link
Member

LGTM! we need to check/fix the install tool / Database Updater too though.

@ausi
Copy link
Member Author

ausi commented Sep 26, 2017

*/
public static function escapeColumnName($name)
{
if (in_array($name, ['rows'], true)) {
Copy link
Member

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)?

Copy link
Member

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. 😄

Copy link
Member

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.

Copy link
Member

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;
}

@ausi
Copy link
Member Author

ausi commented Oct 26, 2017

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:

  1. Do we want to fix this for PHP 5.6 / dbal 2.5? If so, we should replace the MySqlPlatform and add rows to the keyword list.
  2. Do we want to allow reserved keywords as column names in the future? If not we have to rename rows to something different in Contao 4.5

I would say YES to 1. and NO to 2.
@contao/developers ?

@Toflar
Copy link
Member

Toflar commented Oct 26, 2017

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.
Speaking about the schema, I would fix it in 3.5 hardcoded on rows and for Contao 4 (installation bundle) I wouldn't do anything and just tell people to update to PHP 7.1 so they get dbal 2.6. I think it's reasonable to say that if you use the latest and greatest MariaDB you can also use the latest and greatest PHP version.

@ausi
Copy link
Member Author

ausi commented Oct 26, 2017

We need to fix querying anyway so this PR here is needed

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?

@Toflar
Copy link
Member

Toflar commented Oct 26, 2017

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.

@ausi
Copy link
Member Author

ausi commented Oct 26, 2017

OK, then I will update this PR, so that we quote/escape all columns and indexes via doctrine.

@leofeyer
Copy link
Member

I would say YES to 1. and NO to 2.

I agree, although it seems you have agreed on something else meanwhile. If so, never mind. 😄

@aschempp
Copy link
Member

aschempp commented Nov 1, 2017

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.

@leofeyer leofeyer changed the base branch from master to 4.4 December 15, 2017 08:43
@leofeyer leofeyer added this to the 4.4.10 milestone Dec 15, 2017
@ausi
Copy link
Member Author

ausi commented Dec 20, 2017

Closed in favor of #1262

@ausi ausi closed this Dec 20, 2017
leofeyer pushed a commit that referenced this pull request Dec 27, 2017
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.
leofeyer pushed a commit to contao/core that referenced this pull request Jan 19, 2018
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.
@leofeyer leofeyer modified the milestones: 4.4.10, 4.4 May 14, 2019
leofeyer added a commit that referenced this pull request Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants