Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

quoteIdentifier() & quoteIdentifierChain() bug #2670

Closed
wants to merge 3 commits into from

3 participants

@JaredWilliams

Identifier escaping is done means of doubling the delimiter character, and not using a backslash.

JaredWilliams added some commits
@JaredWilliams JaredWilliams quoteIdentifier() & quoteIdentifierChain() bug
Identifier escaping is done means of doubling the delimiter character, and not using a backslash.
3ea563e
@JaredWilliams JaredWilliams quoteIdentifierInFragment() also has wrong escaping 2008dba
@ralphschindler
Collaborator

Thats a good catch, can you link the place in the mysql manual that talks about this? Also, any chance you can update the tests associated with qouteIdentifier() and quoteIdentifierChain ?

https://github.com/zendframework/zf2/blob/master/tests/ZendTest/Db/Adapter/Platform/MysqlTest.php

@JaredWilliams

http://dev.mysql.com/doc/refman/5.6/en/identifiers.html

Section that starts with "Identifier quote characters can be included within an identifier if you quote the identifier."

Also it's not clear what quoteIdentifierInFragment() should be doing. The preg_split() doesn't take into account ` marks in identifiers. IMHO it's a horrid function and should be deprecated/removed.

@JaredWilliams

Also want to look at the SQL Server identifier quoting as that is wrong.

Should be doubling any closing bracket.

http://msdn.microsoft.com/en-us/library/bb399786.aspx

@ralphschindler
Collaborator

Also it's not clear what quoteIdentifierInFragment() should be doing. The preg_split() doesn't take into account ` marks in identifiers. IMHO it's a horrid function and should be deprecated/removed.

The part of me that favors strictness over usability agrees with you. On the other hand, the not-so-strictly typed PHP developer in me understands that in some workflows, you want your components to help you get a project done over getting in your way while doing it.

The art in balancing strictness and usefulness. quoteIdentifierInFragment() is one of those places.

At times, a developer might just want to say "take this and quote it for me". The this I am referring to might be

foo AS bar

The place where this is used is primarily inside Zend\Db\Sql\Select when dealing with fragments of SQL, particularly when dealing with columns and when dealing with join experessions, and group/order expressions where you don't want to create a full expression object, you just want a SQL fragment, with a limited set of keywords, to be quoted as best as it can.

If you have a better idea, I am all ears. If you want to avoid this function, then use Expression objects everywhere. There you get full control over quoting.

You can see some of the various Select unit tests where this is used: (see the m = n) https://github.com/zendframework/zf2/blob/master/tests/ZendTest/Db/Sql/SelectTest.php#L586

I'll try to merge this in as soon as I can.

@akrabat akrabat closed this pull request from a commit
@akrabat akrabat Merge branch 'hotfix/2670'
Close #2670
2d1b40b
@akrabat akrabat closed this in 2d1b40b
@akrabat akrabat referenced this pull request from a commit
@akrabat akrabat Merge branch 'hotfix/2670' into develop
Forward port #2670
8943900
@ghost Unknown referenced this pull request from a commit
@akrabat akrabat Merge branch 'hotfix/2670'
Close #2670
31f89fa
@ghost Unknown referenced this pull request from a commit
@akrabat akrabat Merge branch 'hotfix/2670' into develop
Forward port #2670
3885950
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 4, 2012
  1. @JaredWilliams

    quoteIdentifier() & quoteIdentifierChain() bug

    JaredWilliams authored
    Identifier escaping is done means of doubling the delimiter character, and not using a backslash.
  2. @JaredWilliams
  3. @JaredWilliams
This page is out of date. Refresh to see the latest.
View
6 library/Zend/Db/Adapter/Platform/Mysql.php
@@ -46,7 +46,7 @@ public function getQuoteIdentifierSymbol()
*/
public function quoteIdentifier($identifier)
{
- return '`' . str_replace('`', '\\' . '`', $identifier) . '`';
+ return '`' . str_replace('`', '``', $identifier) . '`';
}
/**
@@ -57,7 +57,7 @@ public function quoteIdentifier($identifier)
*/
public function quoteIdentifierChain($identifierChain)
{
- $identifierChain = str_replace('`', '\\`', $identifierChain);
+ $identifierChain = str_replace('`', '``', $identifierChain);
if (is_array($identifierChain)) {
$identifierChain = implode('`.`', $identifierChain);
}
@@ -134,7 +134,7 @@ public function quoteIdentifierInFragment($identifier, array $safeWords = array(
case 'as':
break;
default:
- $parts[$i] = '`' . str_replace('`', '\\' . '`', $part) . '`';
+ $parts[$i] = '`' . str_replace('`', '``', $part) . '`';
}
}
return implode('', $parts);
View
7 tests/ZendTest/Db/Adapter/Platform/MysqlTest.php
@@ -50,8 +50,9 @@ public function testGetQuoteIdentifierSymbol()
public function testQuoteIdentifier()
{
$this->assertEquals('`identifier`', $this->platform->quoteIdentifier('identifier'));
+ $this->assertEquals('`ident``ifier`', $this->platform->quoteIdentifier('ident`ifier'));
}
-
+
/**
* @covers Zend\Db\Adapter\Platform\Mysql::quoteIdentifierChain
*/
@@ -60,6 +61,10 @@ public function testQuoteIdentifierChain()
$this->assertEquals('`identifier`', $this->platform->quoteIdentifierChain('identifier'));
$this->assertEquals('`identifier`', $this->platform->quoteIdentifierChain(array('identifier')));
$this->assertEquals('`schema`.`identifier`', $this->platform->quoteIdentifierChain(array('schema','identifier')));
+
+ $this->assertEquals('`ident``ifier`', $this->platform->quoteIdentifierChain('ident`ifier'));
+ $this->assertEquals('`ident``ifier`', $this->platform->quoteIdentifierChain(array('ident`ifier')));
+ $this->assertEquals('`schema`.`ident``ifier`', $this->platform->quoteIdentifierChain(array('schema','ident`ifier')));
}
/**
Something went wrong with that request. Please try again.