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

DB select() escaping -> error on spaces #1469

Closed
timo1 opened this issue Jun 13, 2012 · 10 comments
Closed

DB select() escaping -> error on spaces #1469

timo1 opened this issue Jun 13, 2012 · 10 comments
Labels
Milestone

Comments

@timo1
Copy link

timo1 commented Jun 13, 2012

Version: 2.1.1

The sql statement get destroyed when spaces in it.

Example Table:

CREATE TABLE IF NOT EXISTS `test` (
  `ts` datetime NOT NULL,
  `ts2` datetime NOT NULL
) ENGINE=MyISAM DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci;

INSERT INTO `test` (`ts`, `ts2`) VALUES
('2012-06-12 21:01:05', '2012-06-11 21:01:05');

Code:

working correct:

$this->db->select('ts, SUM(CEIL((UNIX_TIMESTAMP(ts)-UNIX_TIMESTAMP(ts2))/60)) AS xy', FALSE);
$this->db->from('test');

$q = $this->db->get();
print_r($q->row_array());

error occurs (spaces arround "CEIL())":

$this->db->select('ts, SUM( CEIL((UNIX_TIMESTAMP(ts)-UNIX_TIMESTAMP(ts2))/60) ) AS xy', FALSE);
...

error message:

You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'FROM (test)' at line 2

SELECT ts, SUM( CEIL((UNIX_TIMESTAMP(ts)-UNIX_TIMESTAMP(ts2))/60) ) AS xy FROM (test)

@lerio
Copy link

lerio commented Jun 13, 2012

Same here. Datamapper 1.8.2 Library is broken too.

Here's the diff between DB_driver.php 2.1.0 and 2.1.1, and that's where the issue probably lies:

        // Convert tabs or multiple spaces into single spaces
-       $item = preg_replace('/[\t ]+/', ' ', $item);
+       $item = preg_replace('/\s+/', ' ', $item);
 
        // If the item has an alias declaration we remove it and set it aside.
        // Basically we remove everything to the right of the first space
-       $alias = '';
-       if (strpos($item, ' ') !== FALSE)
+       if (preg_match('/^([^\s]+) (AS )*(.+)$/i', $item, $matches))
        {
-           $alias = strstr($item, " ");
-           $item = substr($item, 0, - strlen($alias));
+           $item = $matches[1];
+
+           // Escape the alias
+           $alias = ' '.$matches[2].$this->_escape_identifiers($matches[3]);
        }
+       else
+       {
+           $alias = '';
+       }

@Nom4d3
Copy link

Nom4d3 commented Jun 13, 2012

It seems this bug is related to this post:

http://codeigniter.com/forums/viewthread/219130/

@WanWizard
Copy link
Contributor

Looking at the code, it seems that

if (preg_match('/^([^\s]+) (AS )*(.+)$/i', $item, $matches))

also matches when there is no "AS " in the string, due to the * following it (meaning zero or more matches). So it escapes stuff it shouldn't.

@WanWizard
Copy link
Contributor

I get Datamapper back operational by changing that regex to

if (preg_match('/^([^\s]+) (AS )*([^\s]+)$/i', $item, $matches))

So basically only deal with the formats "string string" and "string AS string".

@narfbg
Copy link
Contributor

narfbg commented Jun 13, 2012

There's more to it than that. That regular expression is intended to escape aliases as well as field names, but as it turned out - it's breaking more stuff than what it has fixed. For example ORDER BY fields are passed to that method along with ASC/DESC, WHERE fields along with operators after them, etc., etc.

The quick fix is to revert to the previous version - expect that in the new release shortly (as it is already in the 2.1-stable branch).

@WanWizard
Copy link
Contributor

Any fix is good. ;)

@narfbg
Copy link
Contributor

narfbg commented Jun 14, 2012

Version 2.1.1 has been re-released (under the same version number), with a fix for this bug included. Go download it again at http://codeigniter.com/downloads/ :)

@narfbg narfbg closed this as completed Jun 14, 2012
@Nom4d3
Copy link

Nom4d3 commented Jun 14, 2012

Great! I did some quick tests here and it seems that everything is working nice now!

Thanks!

@mcryan
Copy link

mcryan commented Jun 14, 2012

Excellent, thanks. Will download and test!

@lerio
Copy link

lerio commented Jun 14, 2012

Working fine now. Thank you very much

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants