Interbase/Firebird database driver #1041

Closed
wants to merge 32 commits into
from

Conversation

Projects
None yet
4 participants
@timw4mail
Contributor

timw4mail commented Feb 14, 2012

A database driver for Firebird / Interbase databases - need some help testing, this should have all the basics covered, though.

@timw4mail

View changes

system/database/drivers/interbase/interbase_driver.php
+ */
+ public function _list_tables($prefix_limit = FALSE)
+ {
+ $sql = <<<SQL

This comment has been minimized.

Show comment Hide comment
@timw4mail

timw4mail Feb 14, 2012

Contributor

I don't think the styleguide mentions heredoc. Thoughts?

@timw4mail

timw4mail Feb 14, 2012

Contributor

I don't think the styleguide mentions heredoc. Thoughts?

@philsturgeon

This comment has been minimized.

Show comment Hide comment
@philsturgeon

philsturgeon Feb 14, 2012

Contributor

Should we care? This is in PDO right?

Contributor

philsturgeon commented Feb 14, 2012

Should we care? This is in PDO right?

@timw4mail

This comment has been minimized.

Show comment Hide comment
@timw4mail

timw4mail Feb 15, 2012

Contributor

No, this is for Interbase/Firebird. A new driver. Sorry for all the back and forth on the PDO driver, by the way ^^;

Contributor

timw4mail commented Feb 15, 2012

No, this is for Interbase/Firebird. A new driver. Sorry for all the back and forth on the PDO driver, by the way ^^;

@philsturgeon

This comment has been minimized.

Show comment Hide comment
@philsturgeon

philsturgeon Feb 15, 2012

Contributor

Right I was suggesting that pdo handles that so is there much point adding a new native driver for it?

Emailing from my iPhone 4S like a BOSS!

On 15 Feb 2012, at 00:15, Timothy Warrenreply@reply.github.com wrote:

No, this is for Interbase/Firebird. A new driver.


Reply to this email directly or view it on GitHub:
EllisLab#1041 (comment)

Contributor

philsturgeon commented Feb 15, 2012

Right I was suggesting that pdo handles that so is there much point adding a new native driver for it?

Emailing from my iPhone 4S like a BOSS!

On 15 Feb 2012, at 00:15, Timothy Warrenreply@reply.github.com wrote:

No, this is for Interbase/Firebird. A new driver.


Reply to this email directly or view it on GitHub:
EllisLab#1041 (comment)

@timw4mail

This comment has been minimized.

Show comment Hide comment
@timw4mail

timw4mail Feb 15, 2012

Contributor

Well, the driver for PDO is currently considered experimental, and this would be more generally compatible.

Contributor

timw4mail commented Feb 15, 2012

Well, the driver for PDO is currently considered experimental, and this would be more generally compatible.

@philsturgeon

This comment has been minimized.

Show comment Hide comment
@philsturgeon

philsturgeon Feb 15, 2012

Contributor

Their PDO is considered experimental or your PDO code is experimental?

Not judging, just trying to understand :)


Timothy Warren
mailto:reply@reply.github.com
15 February 2012 00:34

Well, the driver for PDO is currently considered experimental, and
this would be more generally compatible.


Reply to this email directly or view it on GitHub:
EllisLab#1041 (comment)

Contributor

philsturgeon commented Feb 15, 2012

Their PDO is considered experimental or your PDO code is experimental?

Not judging, just trying to understand :)


Timothy Warren
mailto:reply@reply.github.com
15 February 2012 00:34

Well, the driver for PDO is currently considered experimental, and
this would be more generally compatible.


Reply to this email directly or view it on GitHub:
EllisLab#1041 (comment)

@timw4mail

This comment has been minimized.

Show comment Hide comment
@timw4mail

timw4mail Feb 15, 2012

Contributor

PHP's PDO driver for Firebird is experimental. See: http://us2.php.net/manual/en/ref.pdo-firebird.php

Contributor

timw4mail commented Feb 15, 2012

PHP's PDO driver for Firebird is experimental. See: http://us2.php.net/manual/en/ref.pdo-firebird.php

@timw4mail

This comment has been minimized.

Show comment Hide comment
@timw4mail

timw4mail Feb 15, 2012

Contributor

For reference here's a guide to the syntax common to Interbase and Firebird : http://fbclient.googlecode.com/files/LangRef.pdf

Contributor

timw4mail commented Feb 15, 2012

For reference here's a guide to the syntax common to Interbase and Firebird : http://fbclient.googlecode.com/files/LangRef.pdf

@timw4mail

This comment has been minimized.

Show comment Hide comment
@timw4mail

timw4mail Feb 15, 2012

Contributor

Okay, some testing, and I think this should be good to go.

Contributor

timw4mail commented Feb 15, 2012

Okay, some testing, and I think this should be good to go.

@timw4mail

This comment has been minimized.

Show comment Hide comment
@timw4mail

timw4mail Feb 15, 2012

Contributor

@narfbg Any suggestions?

Contributor

timw4mail commented Feb 15, 2012

@narfbg Any suggestions?

@narfbg

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Feb 16, 2012

Contributor

@timw4mail Yeah ... I have some:

  • Remove 1 of the spaces after <?php and those at lines 8 & 10 ... I've been removing them from every file and currently half of them don't have them and the other half have pending pull requests including that. - Remove error logging from db_connect() & db_pconnect() - CI_DB_driver::initialize() handles that.

  • First parameter for ibase_connect() and ibase_pconnect() should optionally include a hostname in order to support remote databases (see the manual).

  • Add 'interbase' to the $driver_version_exceptions in DB_driver.php

  • I don't see an ibase_gen_id() function in the PHP manual, and yet - it's used in insert_id(), which should return the last insert ID anyway, not generate a new one.

  • Remove @access lines from comments, if you have visibility declarations - they're useless.

  • I wouldn't use heredoc.

  • The coding guidelines say use single quotes (at least where it doesn't make it completely unreadable) ... e.g. _field_data() doesn't.

  • _drop_table(), _data_seek() and _list_databases() should return FALSE instead of an empty array.

  • You don't need to assign a string to $sql and return it on the very next line in _rename_table() (might also be the case in other methods as well ... just spotted it in there) ... just return the string. :)

  • Your current implementation of num_rows() has at least 2 issues, one of which could cause a bug in fetching the actual result sets. A better way to implement it is to override the default num_rows property (just define it as public $num_rows in the result class) and then do it something like this:

    public function num_rows()
    {
    if ($this->num_rows !== NULL)
    {
    return $this->num_rows;
    }

    return $this->num_rows = count($this->result_array());
    

    }

  • The for() loops in list_fields() and field_data() are effectively calling ibase_num_fields() on each iteration ... do it like this:

    for ($i = 0, $c = $this->num_fields(); $i < $c; $i++)

Contributor

narfbg commented Feb 16, 2012

@timw4mail Yeah ... I have some:

  • Remove 1 of the spaces after <?php and those at lines 8 & 10 ... I've been removing them from every file and currently half of them don't have them and the other half have pending pull requests including that. - Remove error logging from db_connect() & db_pconnect() - CI_DB_driver::initialize() handles that.

  • First parameter for ibase_connect() and ibase_pconnect() should optionally include a hostname in order to support remote databases (see the manual).

  • Add 'interbase' to the $driver_version_exceptions in DB_driver.php

  • I don't see an ibase_gen_id() function in the PHP manual, and yet - it's used in insert_id(), which should return the last insert ID anyway, not generate a new one.

  • Remove @access lines from comments, if you have visibility declarations - they're useless.

  • I wouldn't use heredoc.

  • The coding guidelines say use single quotes (at least where it doesn't make it completely unreadable) ... e.g. _field_data() doesn't.

  • _drop_table(), _data_seek() and _list_databases() should return FALSE instead of an empty array.

  • You don't need to assign a string to $sql and return it on the very next line in _rename_table() (might also be the case in other methods as well ... just spotted it in there) ... just return the string. :)

  • Your current implementation of num_rows() has at least 2 issues, one of which could cause a bug in fetching the actual result sets. A better way to implement it is to override the default num_rows property (just define it as public $num_rows in the result class) and then do it something like this:

    public function num_rows()
    {
    if ($this->num_rows !== NULL)
    {
    return $this->num_rows;
    }

    return $this->num_rows = count($this->result_array());
    

    }

  • The for() loops in list_fields() and field_data() are effectively calling ibase_num_fields() on each iteration ... do it like this:

    for ($i = 0, $c = $this->num_fields(); $i < $c; $i++)

@timw4mail

This comment has been minimized.

Show comment Hide comment
@timw4mail

timw4mail Feb 16, 2012

Contributor

Thanks.

ibase_gen_id - http://us2.php.net/manual/en/function.ibase-gen-id.php

I think heredoc is more readable with less escaping for those two long queries. I wouldn't use it everywhere, but I think it's better than having to escape all the single or double quotes.

I'll get around to the rest of those when I get the chance.

Contributor

timw4mail commented Feb 16, 2012

Thanks.

ibase_gen_id - http://us2.php.net/manual/en/function.ibase-gen-id.php

I think heredoc is more readable with less escaping for those two long queries. I wouldn't use it everywhere, but I think it's better than having to escape all the single or double quotes.

I'll get around to the rest of those when I get the chance.

@narfbg

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Feb 16, 2012

Contributor

I guess it's just my mirror doesn't have it then, sorry. :)

Contributor

narfbg commented Feb 16, 2012

I guess it's just my mirror doesn't have it then, sorry. :)

@timw4mail

This comment has been minimized.

Show comment Hide comment
@timw4mail

timw4mail Feb 16, 2012

Contributor

I get the same error with PHP 5.3.10. Thoughts? @narfbg

Contributor

timw4mail commented Feb 16, 2012

I get the same error with PHP 5.3.10. Thoughts? @narfbg

@narfbg

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Feb 16, 2012

Contributor

@timw4mail It's because of this line in num_rows():

return $this->num_rows = (isset($this->result_array()) ? count($this->result_array()) : 0;

... you can't use isset() on a method and it's pointless anyway. You've probably meant to do that with the result_array property, but it will always return TRUE for it.

Contributor

narfbg commented Feb 16, 2012

@timw4mail It's because of this line in num_rows():

return $this->num_rows = (isset($this->result_array()) ? count($this->result_array()) : 0;

... you can't use isset() on a method and it's pointless anyway. You've probably meant to do that with the result_array property, but it will always return TRUE for it.

@timw4mail

This comment has been minimized.

Show comment Hide comment
@timw4mail

timw4mail Feb 17, 2012

Contributor

Hmm...this num_rows function is driving me nuts. When I do it the way @narfbg suggests, I run into excessive memory usage issues. When I do it the current way, the number of items is only found when geting all of the items from the database. Which means that the list_tables function doesn't work, because it checks for the number of rows before retrieving the rows.

Any suggestions @philsturgeon @narfbg @toopay?

Contributor

timw4mail commented Feb 17, 2012

Hmm...this num_rows function is driving me nuts. When I do it the way @narfbg suggests, I run into excessive memory usage issues. When I do it the current way, the number of items is only found when geting all of the items from the database. Which means that the list_tables function doesn't work, because it checks for the number of rows before retrieving the rows.

Any suggestions @philsturgeon @narfbg @toopay?

@narfbg

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Feb 17, 2012

Contributor

@timw4mail

Sorry, but I'm not really an interbase/firebird user. What I can do is make suggestions from my personal PHP knowledge and by looking at the ibase_*() functions' manuals. With that in mind ...

On the hostname - it's optional, so I don't know if always prepending it is OK. If it works for you - I'll have to assume it's correct. :)

As for num_rows() ... If you have memory usage issues with my method - well, you'll always have them. The issue is not in num_rows() - it just does a simple count().
It's in result_array() - you need to override it, because it doesn't work well with database APIs that don't have a native num_rows() function/method. See my implementations for it in the Oracle/oci8, PDO and SQLite3 drivers (pending).

Contributor

narfbg commented Feb 17, 2012

@timw4mail

Sorry, but I'm not really an interbase/firebird user. What I can do is make suggestions from my personal PHP knowledge and by looking at the ibase_*() functions' manuals. With that in mind ...

On the hostname - it's optional, so I don't know if always prepending it is OK. If it works for you - I'll have to assume it's correct. :)

As for num_rows() ... If you have memory usage issues with my method - well, you'll always have them. The issue is not in num_rows() - it just does a simple count().
It's in result_array() - you need to override it, because it doesn't work well with database APIs that don't have a native num_rows() function/method. See my implementations for it in the Oracle/oci8, PDO and SQLite3 drivers (pending).

@toopay

This comment has been minimized.

Show comment Hide comment
@toopay

toopay Feb 17, 2012

Contributor

If you ask me, it probably would start a debate :) In my understanding, if some database interface did not support a pre-fetched num_rows because it simply cannot locate the cursor before it start fetched the result handler, than i will simply return it with -1.

But, as we already discussed this, several time in PDO or SQLite, then the solution indeed to do the revert process, to get the num_rows works out of the box. In most of situation, any performance glitch, causing by this process, is still acceptable.

Contributor

toopay commented Feb 17, 2012

If you ask me, it probably would start a debate :) In my understanding, if some database interface did not support a pre-fetched num_rows because it simply cannot locate the cursor before it start fetched the result handler, than i will simply return it with -1.

But, as we already discussed this, several time in PDO or SQLite, then the solution indeed to do the revert process, to get the num_rows works out of the box. In most of situation, any performance glitch, causing by this process, is still acceptable.

@timw4mail

This comment has been minimized.

Show comment Hide comment
@timw4mail

timw4mail Feb 17, 2012

Contributor

Either I'm crazy, or something about using in_array with the results returned from the list_tables function just isn't correct.

if you use the table_exists method, it always returns false. You look at the print_r output of the array from the list_tables function, and it's just as you expect it would be. But using in_array with that just doesn't seem to work.

This is the case for me in version 5.3.10, and 5.4RC7.

Other than that crazy issue, I think I'm satisfied with this now.

Contributor

timw4mail commented Feb 17, 2012

Either I'm crazy, or something about using in_array with the results returned from the list_tables function just isn't correct.

if you use the table_exists method, it always returns false. You look at the print_r output of the array from the list_tables function, and it's just as you expect it would be. But using in_array with that just doesn't seem to work.

This is the case for me in version 5.3.10, and 5.4RC7.

Other than that crazy issue, I think I'm satisfied with this now.

@timw4mail

This comment has been minimized.

Show comment Hide comment
@timw4mail

timw4mail Feb 20, 2012

Contributor

Here's the db class I've been using for testing: https://gist.github.com/1871431

Everything works with the exception of the check_tables() method, which has the in_array problems previously mentioned.

Any other comments, @toopay @narfbg @philsturgeon?

Contributor

timw4mail commented Feb 20, 2012

Here's the db class I've been using for testing: https://gist.github.com/1871431

Everything works with the exception of the check_tables() method, which has the in_array problems previously mentioned.

Any other comments, @toopay @narfbg @philsturgeon?

@narfbg

View changes

system/database/drivers/interbase/interbase_driver.php
+ public $dbdriver = 'interbase';
+
+ // The character used to escape with
+ public $_escape_char = '"';

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Feb 20, 2012

Contributor

This, as well as $_like_escape_str, $_like_escape_char, $_count_string and $_random_keyword should be protected.

@narfbg

narfbg Feb 20, 2012

Contributor

This, as well as $_like_escape_str, $_like_escape_char, $_count_string and $_random_keyword should be protected.

@narfbg

View changes

system/database/drivers/interbase/interbase_driver.php
+
+ $query = $this->query($this->_count_string . $this->_protect_identifiers('numrows') . ' FROM ' . $this->_protect_identifiers($table, TRUE, NULL, FALSE));
+
+ $query->result_array();

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Feb 20, 2012

Contributor

Why this?

@narfbg

narfbg Feb 20, 2012

Contributor

Why this?

This comment has been minimized.

Show comment Hide comment
@timw4mail

timw4mail Feb 20, 2012

Contributor

Hmm...good question. I think it just slipped through on accident.

@timw4mail

timw4mail Feb 20, 2012

Contributor

Hmm...good question. I think it just slipped through on accident.

@narfbg

View changes

system/database/drivers/interbase/interbase_driver.php
+ * @param string
+ * @return string
+ */
+ public function _escape_identifiers($item)

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Feb 20, 2012

Contributor

All of the methods prefixed with an underscore should be protected, unless you know that they'd be directly called by DB forge and/or DB utility as with the current implementation they won't be accessible from there.

@narfbg

narfbg Feb 20, 2012

Contributor

All of the methods prefixed with an underscore should be protected, unless you know that they'd be directly called by DB forge and/or DB utility as with the current implementation they won't be accessible from there.

@narfbg

View changes

system/database/drivers/interbase/interbase_forge.php
+ {
+ $sql = 'CREATE TABLE ';
+
+ $sql .= $this->db->_escape_identifiers($table)."(";

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Feb 20, 2012

Contributor

Can you use protect_identifiers() in here? Also - it would be better to replace _protect_identifiers() with it, so we can make the latter protected at some point.

@narfbg

narfbg Feb 20, 2012

Contributor

Can you use protect_identifiers() in here? Also - it would be better to replace _protect_identifiers() with it, so we can make the latter protected at some point.

@narfbg

View changes

system/database/drivers/interbase/interbase_result.php
+ public function list_fields()
+ {
+ $field_names = array();
+ for ($i = 0, $num_fields=$this->num_fields(); $i < $num_fields; $i++)

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Feb 20, 2012

Contributor

Having spaces around the equals sign would make this a bit more readable. :)

@narfbg

narfbg Feb 20, 2012

Contributor

Having spaces around the equals sign would make this a bit more readable. :)

@narfbg

View changes

system/database/drivers/interbase/interbase_result.php
+ public function _fetch_assoc()
+ {
+ //Increment row count
+ $this->num_rows++;

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Feb 20, 2012

Contributor

What if ibase_fetch_assoc() returns FALSE? We'll have a row count of 1, but no actual results ...
I still don't understand why this is done in here instead of num_rows() anyway.

@narfbg

narfbg Feb 20, 2012

Contributor

What if ibase_fetch_assoc() returns FALSE? We'll have a row count of 1, but no actual results ...
I still don't understand why this is done in here instead of num_rows() anyway.

This comment has been minimized.

Show comment Hide comment
@timw4mail

timw4mail Feb 20, 2012

Contributor

Good point, I'll fix that. The reason it is there is because it works :)

@timw4mail

timw4mail Feb 20, 2012

Contributor

Good point, I'll fix that. The reason it is there is because it works :)

@timw4mail

This comment has been minimized.

Show comment Hide comment
@timw4mail

timw4mail Feb 20, 2012

Contributor

@narfbg I believe that takes care of what you mentioned.

Contributor

timw4mail commented Feb 20, 2012

@narfbg I believe that takes care of what you mentioned.

@narfbg

View changes

system/database/drivers/interbase/interbase_result.php
+
+ // Since the object and array are really similar, just case
+ // the result object to an array if need be
+ if(count($this->result_object) > 0)

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Feb 20, 2012

Contributor

You should also have spaces between language constructs (if, foreach, for, etc.) and brackets that follow them.
I also spotted a few other uses of $var='value' and foreach ($array as $key=>$value) where the same applies.

@narfbg

narfbg Feb 20, 2012

Contributor

You should also have spaces between language constructs (if, foreach, for, etc.) and brackets that follow them.
I also spotted a few other uses of $var='value' and foreach ($array as $key=>$value) where the same applies.

@narfbg

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Feb 20, 2012

Contributor

@timw4mail _protect_identifiers() is still heavily used in DB forge ... switch it to protect_identifiers() please.

Contributor

narfbg commented Feb 20, 2012

@timw4mail _protect_identifiers() is still heavily used in DB forge ... switch it to protect_identifiers() please.

@timw4mail

This comment has been minimized.

Show comment Hide comment
@timw4mail

timw4mail Feb 21, 2012

Contributor

This look good now? @narfbg

Contributor

timw4mail commented Feb 21, 2012

This look good now? @narfbg

@narfbg

View changes

system/database/drivers/interbase/interbase_forge.php
+ $sql .= $this->db->protect_identifiers($table)."(";
+ $current_field_count = 0;
+
+ foreach ($fields as $field=>$attributes)

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Feb 21, 2012

Contributor

Spaces here.

@narfbg

narfbg Feb 21, 2012

Contributor

Spaces here.

@narfbg

View changes

system/database/drivers/interbase/interbase_result.php
+ {
+ foreach ($this->result_object as $obj)
+ {
+ $this->result_array[] = (array)$obj;

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Feb 21, 2012

Contributor

... and a space here, after (array).

@narfbg

narfbg Feb 21, 2012

Contributor

... and a space here, after (array).

@narfbg

View changes

system/database/drivers/interbase/interbase_driver.php
+ /**
+ * Insert ID
+ *
+ * @param string $generator_name

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Feb 21, 2012

Contributor

These 3 lines look a bit odd ... see if you use tabs in all of them and separate each pseudo-column with just one.

@narfbg

narfbg Feb 21, 2012

Contributor

These 3 lines look a bit odd ... see if you use tabs in all of them and separate each pseudo-column with just one.

@narfbg

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Feb 21, 2012

Contributor

Other than the spacing stuff I just commented - it looks OK, functionally at least.
Have you resolved that in_array() issue?

Contributor

narfbg commented Feb 21, 2012

Other than the spacing stuff I just commented - it looks OK, functionally at least.
Have you resolved that in_array() issue?

@timw4mail

This comment has been minimized.

Show comment Hide comment
@timw4mail

timw4mail Feb 21, 2012

Contributor

No, I can't figure out what the deal is with that in_array() issue. In a separate project that I'm incorporating Firebird into I have the same issue, and it's written completely differently. I'm stumped.

Contributor

timw4mail commented Feb 21, 2012

No, I can't figure out what the deal is with that in_array() issue. In a separate project that I'm incorporating Firebird into I have the same issue, and it's written completely differently. I'm stumped.

@narfbg

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Feb 21, 2012

Contributor

Can you give an example? I didn't quite get what exactly the issue is ...

Contributor

narfbg commented Feb 21, 2012

Can you give an example? I didn't quite get what exactly the issue is ...

@timw4mail

This comment has been minimized.

Show comment Hide comment
@timw4mail

timw4mail Feb 21, 2012

Contributor

When you use in_array with the output of the list_databases function, it always returns false. So, if you want to check that db foo exists, you get a false negative.

Contributor

timw4mail commented Feb 21, 2012

When you use in_array with the output of the list_databases function, it always returns false. So, if you want to check that db foo exists, you get a false negative.

@timw4mail

This comment has been minimized.

Show comment Hide comment
@timw4mail

timw4mail Feb 21, 2012

Contributor

Moving to a new pull request for brevity

Contributor

timw4mail commented Feb 21, 2012

Moving to a new pull request for brevity

@timw4mail timw4mail closed this Feb 21, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment