Skip to content

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Apr 7, 2012
Merged

Fixes for insertid issues with postgresql driver #2

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fbe7606
Fixes for insertid issues with postgresql driver
carlopires Apr 3, 2012
31f0952
Fix template adapters to work with postgresql
carlopires Apr 3, 2012
2a94a01
Fixed JDatabase::insertObject and JDatabase::updateObject to use JDat…
carlopires Apr 3, 2012
9881248
Fixed joomla users creation
carlopires Apr 3, 2012
bd747b5
Finished users management fixes with PostgreSQL
carlopires Apr 4, 2012
a97e2ff
Removed insertid calls from installer adapters to avoid pitfalls rela…
carlopires Apr 4, 2012
559c69e
Fixed menu edition with PostgreSQL
carlopires Apr 4, 2012
8d6e9e1
Better fix to JDatabasePostgreSQL::getNumRows
carlopires Apr 4, 2012
a4aa9f7
Joomla coding style applied to JDatabasePostgreSQL::insertObject
carlopires Apr 4, 2012
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions installation/sql/postgresql/joomla.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1703,7 +1703,7 @@ CREATE TABLE "#__menu" (
"img" character varying(255) DEFAULT '' NOT NULL,
"template_style_id" integer DEFAULT 0 NOT NULL,
-- JSON encoded data for the menu item.
"params" text NOT NULL,
"params" text DEFAULT '' NOT NULL,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

-- Nested set lft.
"lft" bigint DEFAULT 0 NOT NULL,
-- Nested set rgt.
Expand Down Expand Up @@ -2385,4 +2385,4 @@ BEGIN

RETURN soundex;
END;
$$;
$$;
4 changes: 2 additions & 2 deletions libraries/joomla/database/database.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -838,7 +838,7 @@ public function insertObject($table, &$object, $key = null)
$values = array();

// Create the base insert statement.
$statement = 'INSERT INTO ' . $this->quoteName($table) . ' (%s) VALUES (%s)';
$statement = 'INSERT INTO ' . $this->quoteName($table) . ' (%s) VALUES (%s) RETURNING id';
Copy link
Owner

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.

Copy link
Author

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)

Copy link
Owner

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.


// Iterate over the object variables to build the query fields and values.
foreach (get_object_vars($object) as $k => $v)
Expand Down Expand Up @@ -868,7 +868,7 @@ public function insertObject($table, &$object, $key = null)
}

// Update the primary key if it exists.
$id = $this->insertid();
$id = $this->loadResult();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refer to last comment.

if ($key && $id)
{
$object->$key = $id;
Expand Down
55 changes: 33 additions & 22 deletions libraries/joomla/database/database/postgresql.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,11 @@ public function getCollation()
*/
public function getNumRows( $cur = null )
{
return pg_num_rows($cur ? $cur : $this->cursor);
if($cur)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have you add this block ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To stop php warning on non integer value

$cur = intval($cur);
else
$cur = $this->cursor;
return pg_num_rows($cur);
}

/**
Expand Down Expand Up @@ -450,10 +454,6 @@ public function insertObject($table, &$object, $key = null)
$fields = array();
$values = array();

// Create the base insert statement.
$query = $this->getQuery(true);
$query->insert($this->quoteName($table));

// Iterate over the object variables to build the query fields and values.
foreach (get_object_vars($object) as $k => $v)
{
Expand All @@ -474,24 +474,35 @@ public function insertObject($table, &$object, $key = null)
$values[] = is_numeric($v) ? $v : $this->quote($v);
}

$query->columns($fields);
$query->values(implode(',', $values));

// Set the query and execute the insert.
$this->setQuery($query);
if (!$this->query())
{
return false;
}

// Update the primary key if it exists.
$id = $this->insertid();
if ($key && $id)
{
$object->$key = $id;
// Create the base insert statement.
$query = $this->getQuery(true);
Copy link
Owner

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

Copy link
Owner

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.

Copy link
Owner

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

if ($key) {
$query->insert($this->quoteName($table))
->columns($fields)
->values(implode(',', $values))
->returning($key);

// Set the query and execute the insert.
$this->setQuery($query);

$id = $this->loadResult();
if ($id) {
$object->$key = $id;
return true;
} else
return false;
} else {
$query->insert($this->quoteName($table))
->columns($fields)
->values(implode(',', $values));

// Set the query and execute the insert.
$this->setQuery($query);
if (!$this->query())
return false;
else
return true;
}

return true;
}

/**
Expand Down
10 changes: 5 additions & 5 deletions libraries/joomla/installer/adapters/component.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -541,11 +541,11 @@ public function install()
return false;
}

$eid = $db->insertid();
$eid = $row->extension_id;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you do this change ?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you do this change ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid pitfalls related on calling insertid after JTable::store.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your intention is correct, but are they equal?
You're doing this because "extension_id" is incremented inside JTable::store, is it right?

Eng. Gabriele Pongelli

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extension_id is just the primary key of the installed component, which is the same value of $row->$key. I'm just callind $key by primary key name: extension_id.

The real value of extension_id is set inside JDatabase::insertObject, which is called by JTable::store, which call insertid in right context (just after setQuery). Calling insertid here again is redundant and may lead to unexpected results.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perfect!
I'm merging this pull but here I'll change to $row->$key to be independent of future primary key's change.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't do that because there is more uses of $row->extension_id in this function. So, I decided to keep the code following the original guidelines.


// Clobber any possible pending updates
$update = JTable::getInstance('update');
$uid = $update->find(array('element' => $this->get('element'), 'type' => 'component', 'client_id' => '', 'folder' => ''));
$uid = $update->find(array('element' => $this->get('element'), 'type' => 'component', 'client_id' => 1, 'folder' => ''));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for this and next three.


if ($uid)
{
Expand Down Expand Up @@ -1021,7 +1021,7 @@ public function update()

// Clobber any possible pending updates
$update = JTable::getInstance('update');
$uid = $update->find(array('element' => $this->get('element'), 'type' => 'component', 'client_id' => '', 'folder' => ''));
$uid = $update->find(array('element' => $this->get('element'), 'type' => 'component', 'client_id' => 1, 'folder' => ''));

if ($uid)
{
Expand Down Expand Up @@ -1323,7 +1323,7 @@ public function uninstall($id)

// Clobber any possible pending updates
$update = JTable::getInstance('update');
$uid = $update->find(array('element' => $row->element, 'type' => 'component', 'client_id' => '', 'folder' => ''));
$uid = $update->find(array('element' => $row->element, 'type' => 'component', 'client_id' => 1, 'folder' => ''));

if ($uid)
{
Expand Down Expand Up @@ -1950,7 +1950,7 @@ public function discover_install()

// Clobber any possible pending updates
$update = JTable::getInstance('update');
$uid = $update->find(array('element' => $this->get('element'), 'type' => 'component', 'client_id' => '', 'folder' => ''));
$uid = $update->find(array('element' => $this->get('element'), 'type' => 'component', 'client_id' => 1, 'folder' => ''));

if ($uid)
{
Expand Down
3 changes: 0 additions & 3 deletions libraries/joomla/installer/adapters/file.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,9 +254,6 @@ public function install()
return false;
}

// Set the insert id
$row->set('extension_id', $db->insertid());

// Since we have created a module item, we add it to the installation step stack
// so that if we have to rollback the changes we can undo it.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you removed this row (and similarly in module.php) because "extension_id" is a serial row?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. The real problem is that JDatabasePostgreSQL::insertid is not working for me. I had to manage to remove all calls to insertid when using postgresql.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not only your problem, and it's larger than expected, let me explain.

If you read JDatabasePostgresql::insertid dockblock you'll find some useful information on how to use "insertid" call: because I need to extract information from last "INSERT INTO" JDatabasePostgresqlQuery object I MUST run "insertid" after INSERT INTO query execution.

In this case, inside file.php and others under this folder, as you can see there's no JDatabaseQuery to insert row, but JTable object.
To go more deeper, inside JTable::store you can find an insert into (or an update query, depending on a value), and some other queries, so as I've just explained this procedure will not work for PostgreSQL because insertid doesn't follow INSERT INTO JDatabaseQuery.

I've opened some discussion under platform mailing list about JTable but till now nobody answer.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, changing JDatabase::insertObject with platform's code will fix these issues because JTable::store calls JDatabase::insertObject and JDatabase::updateObject. I'll try this.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but the big problem is logical: why do you call "insertid" if there was an update ? there is no sense on doing this.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look at JTable::store , there you can find an insertid after insertObject or updateObject , depending on which parameter has received JTable::store itself.

Eng. Gabriele Pongelli.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw your email on google groups and understood the problem.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember you're not using only PostgreSQL driver, so removing these "insertid" call in these files probably break compatibility with other database engine, even if they don't work with PostgreSQL too.

Eng. Gabriele Pongelli.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compatibility is maintained because JDatabase::insertObject sets the KEY using insertid. Therefore, those calls were redundant.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok perfect, .

$this->parent->pushStep(array('type' => 'extension', 'extension_id' => $row->extension_id));
Expand Down
3 changes: 0 additions & 3 deletions libraries/joomla/installer/adapters/module.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,9 +427,6 @@ public function install()
return false;
}

// Set the insert id
$row->extension_id = $db->insertid();

// Since we have created a module item, we add it to the installation step stack
// so that if we have to rollback the changes we can undo it.
$this->parent->pushStep(array('type' => 'extension', 'extension_id' => $row->extension_id));
Expand Down