-
Notifications
You must be signed in to change notification settings - Fork 1
Fixes for insertid issues with postgresql driver #2
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
Conversation
Fixes to postgresql sql script Fixes to database adapters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is JDatabase::insertObject, base class for all drivers, so this is common for all driver while "RETURNING" clause works only on PostgreSQL and maybe on Oracle, but surely it doesn't work on MySQL .
This change, and the following on this file, can be useful for JDatabasePostgresql::insertObject, or change this to use implementation merged inside platform .
Anyway, thank you to submit this change I think I'll push inside platform's driver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah... I think this code must go to platform's driver. The current code did not work to me (postgresql 9.1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not work because there's no JDatabasePostgresqlQuery for that INSERT INTO statement, so "insertid" will crash.
If you change this code with that already present inside platform it will work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting change, could be better implemented without same code replication, maybe doing what you've proposed for database.php
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The real effort of this block is replacement of "insertid" with "returning" JDatabasePostgresqlQuery element.
I'm thinking a better solution than existing and than this change.
Ps
platform code has to pass some code style check, this change will not pass them.
…abaseQuery Revert installer adapters to use JDatabase::insertid and avoid break of compatibility with others drivers
|
Gabriele, I changed JDatabase::insertObject and JDatabase::updateObject like you suggested and could revert the changes about insertid on installer adapters. I was wondering if JDatabasePostgreSQL::inserid could just be just: As JDatabasePostgreSQL::insertObject are using RETURNING clause. The problem is that JDatabasePostgreSQL::insertObject can be called null key parameter and this could crash insertid in some point. I decided to keep the current code and now everything is working fine. The final diffs are now much less impactive. Thanks a lot for you help. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm reading pg_num_rows doc, it accepts only resources so casting null to integer doesn't seem to be a resource.
I think it's better something like ( !isnull($cur) ? $cur : $this->cursor) , don't you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$cur parameter comes with boolean value (false) and is causing php to issue warnings... What about this:
return pg_num_rows( is_numeric($cur) ? (int)$cur : $this->cursor);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the error is $cur value set to false, it has to be a resource link .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, but to fix this I will need to revisit all getNumRows calls and this is out of my possibilities, now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes are what I wanted to do after merging this pull, thank you.
+1
|
Really good job, thank you very much of this pull! Eng. Gabriele Pongelli. |
Fixes for insertid issues with postgresql driver
Also includes:
Gabriele,
First, thanks for your hard work on make joomla run with postgresql. I'm using it right now.
I had to make some changes to make my components install with postgresql_v2 branch. I'm pulling to you to see if these fixes are useful to you.