Identifier quoting in Postgres

[arcijsg]

Hi!

I was quite impressed by compact and clean code of idiorm - so, I'm giving it a try in project where Postgresql database is be used. Problem I had was with ORM::_quote_identifier_part() - which quotes table/column names with "`", but a SQL statement in postgres commits seppuku when treated that way :) Could quoting character be made as a configuration option?

.. code-block:: php

protected function _quote_identifier_part($part) {
   $quote = self::$_config['identifier_quote_ch'];
   return $quote . $part . $quote;
}

Or - probably, identifier quoting could be called as a callback, leaving ORM::_quote_identifier_part just as a default method for that and be overridable in configuration?

How do You feel about such changes? What the correct approach would be?

[j4mie]

Hi, thanks for trying out Idiorm.

I'm aware that this is an issue and it's definitely something I'd like to address.

I had a couple of ideas here. I like your first suggestion - just having a config setting for the quoting character. This would be a really simple solution. However, it would be nice if Idiorm could do this more automatically, without having to set an obscure config setting to get it to work on Postgres (or SQL server etc).

Unfortunately, PDO doesn't provide a portable way to quote identifiers (if you do a bit of Googling, you'll find a few feature requests to add this functionality to PDO). So, my proposal would be to use $_db->getAttribute(PDO::ATTR_DRIVER_NAME) to get the name of the driver, and set the quote character based on whether the returned value is sqlite or mysql or.. whatever the name of the postgres driver is!

You would still be able to override this with a config setting, so if your DB driver isn't listed or if you want to use a non-standard quote character you still can.

Does this sound like a good solution? Can you see any problems? If not, I'll add this next time I get a bit of free time (hopefully tonight or tomorrow).

[j4mie]

Hi again,

I just pushed commit 168c48103e964553156b02aac5db00e396a0aad7 which addresses this.

Can you give it a go and let me know if it works?

I'll need to add details of the other driver names (SQL Server, Oracle, etc) to the switch statement in the _detect_identifier_quote_character method, but I can't find a list of these anywhere. Any ideas?

I also have no idea which database backends are actually supported. As far as I know, Idiorm generates pretty standard SQL, but I don't really have any way of testing it on different databases. I know the limit/offset stuff won't work on MS SQL for example.

Maybe I'll just leave it and wait for people using particular drivers to report issues..

[j4mie]

Added a few more in commit 62776548f513014f131ec962838d4c024e57ee46

Let me know if this works for you, I'll close it if so.

[arcijsg]

Thanks for a quick reply and support!

Seems like a good solution to me - I will try it out in a few moments..

As _quote_identifier_part() is a "leaf" method - called quite often - could't we benefit from moving auto-detecting code to _setup_db()? Detecting by selected DB type either way cannot be done before first call to that mmethod..

Or - even better:

• make ORM::_setup_db() to call ORM::set_db($pdo_object) internally instead of directly setting:

  self::$_db = new PDO($connection_string, $username, $password, $driver_options);

and then, place quote-char autodetecting in ORM::set_db(). Besides we would have then single point where DB connection could be changed.

[arcijsg]

Apart from where autodetecting happens - quoting for Postgres now works for me - thanks!

[j4mie]

Great. I like your idea of moving the auto-detection stuff to set_db and changing _setup_db to use this. I'll take a look at this soon and close this issue when I'm done.

Please let me know if you have any more problems or suggestions.

[j4mie]

Identified a problem with this. _setup_db is only called when the first database query is run, but _quote_identifier_part needs to be called before this. I've pushed a quick fix, but I need to do some rethinking here.

[j4mie]

OK - I think I've got this all fixed up in 86019de1137e5c8ef03b3b6c106c41327654f9a8

Thanks for the help and suggestions. Please let me know if you have any further problems.

[jakob]

I just stumbled accross this discussion when googling something else, and the implementation of _quote_identifier_part jumped into my eye, because it might enable an SQL injection attack! It doesn't properly quote identifiers!

This is what it should look like:

protected function _quote_identifier_part($part) {
   $quote = self::$_config['identifier_quote_ch'];
   return $quote . str_replace($quote, "$quote$quote", $part) . $quote;
}

Note that str_replace is missing in the original code!

[treffynnon]

This a very old ticket. Please see the actual functionality in master: https://github.com/j4mie/idiorm/blob/master/idiorm.php#L1417

[jakob]

Oh, I see it is fixed now. I did check if the problem is still there in the newest version, but I must have been looking at a wrong branch. I'm sorry.

On 01.07.2013, at 14:07, Simon Holywell notifications@github.com wrote:

This a very old ticket. Please see the actual functionality in master: https://github.com/j4mie/idiorm/blob/master/idiorm.php#L1417

— Reply to this email directly or view it on GitHub.