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 8 commits
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
2 changes: 1 addition & 1 deletion administrator/components/com_menus/models/items.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ protected function getListQuery()
$app = JFactory::getApplication();

// Select all fields from the table.
$query->select($this->getState('list.select', 'a.id, a.menutype, a.title, a.alias, a.note, a.path, a.link, a.type, a.parent_id, a.level, a.published as apublished, a.component_id, a.ordering, a.checked_out, a.checked_out_time, a.browserNav, a.access, a.img, a.template_style_id, a.params, a.lft, a.rgt, a.home, a.language, a.client_id'));
$query->select($this->getState('list.select', 'a.id, a.menutype, a.title, a.alias, a.note, a.path, a.link, a.type, a.parent_id, a.level, a.published as apublished, a.component_id, a.ordering, a.checked_out, a.checked_out_time, a."browserNav", a.access, a.img, a.template_style_id, a.params, a.lft, a.rgt, a.home, a.language, a.client_id'));
Copy link
Owner

Choose a reason for hiding this comment

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

+1

$query->select('CASE a.type' .
' WHEN ' . $db->quote('component') . ' THEN a.published+2*(e.enabled-1) ' .
' WHEN ' . $db->quote('url') . ' THEN a.published+2 ' .
Expand Down
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;
$$;
$$;
14 changes: 9 additions & 5 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,8 @@ public function insertObject($table, &$object, $key = null)
$values = array();

// Create the base insert statement.
$statement = 'INSERT INTO ' . $this->quoteName($table) . ' (%s) VALUES (%s)';
$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 @@ -857,11 +858,14 @@ public function insertObject($table, &$object, $key = null)

// Prepare and sanitize the fields and values for the database query.
$fields[] = $this->quoteName($k);
$values[] = $this->quote($v);
$values[] = is_numeric($v) ? $v : $this->quote($v);
}

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

// Set the query and execute the insert.
$this->setQuery(sprintf($statement, implode(',', $fields), implode(',', $values)));
$this->setQuery($query);
if (!$this->query())
{
return false;
Expand Down Expand Up @@ -1618,7 +1622,7 @@ public function updateObject($table, &$object, $key, $nulls = false)
// Set the primary key to the WHERE clause instead of a field to update.
if ($k == $key)
{
$where = $this->quoteName($k) . '=' . $this->quote($v);
$where = $this->quoteName($k) . '=' . (is_numeric($v) ? $v : $this->quote($v));
continue;
}

Expand All @@ -1639,7 +1643,7 @@ public function updateObject($table, &$object, $key, $nulls = false)
// The field is not null so we prep it for update.
else
{
$val = $this->quote($v);
$val = (is_numeric($v) ? $v : $this->quote($v));
}

// Add the field to be updated.
Expand Down
51 changes: 29 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,7 @@ public function getCollation()
*/
public function getNumRows( $cur = null )
{
return pg_num_rows($cur ? $cur : $this->cursor);
return pg_num_rows((int)$cur ? $cur : $this->cursor);
Copy link
Owner

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?

Copy link
Author

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);

Copy link
Owner

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 .

Copy link
Author

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.

}

/**
Expand Down Expand Up @@ -450,10 +450,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 +470,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.

if ($key) {
$query->insert($this->quoteName($table))
->columns($fields)
->values(implode(',', $values))
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

->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
15 changes: 13 additions & 2 deletions libraries/joomla/database/table/user.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,12 @@ public function bind($array, $ignore = '')
*/
public function check()
{
// Set user id to null istead of 0, if needed
if ($this->id === 0)
{
$this->id = null;
}

// Validate user information
if (trim($this->name) == '')
{
Expand All @@ -207,11 +213,16 @@ public function check()
}

// Set the registration timestamp
if ($this->registerDate == null || $this->registerDate == $this->_db->getNullDate())
if (empty($this->registerDate) || $this->registerDate == $this->_db->getNullDate())
{
$this->registerDate = JFactory::getDate()->toSql();
}

// Set the lastvisitDate timestamp
if (empty($this->lastvisitDate))
{
$this->lastvisitDate = $this->_db->getNullDate();
}

// check for existing username
$query = $this->_db->getQuery(true);
$query->select($this->_db->quoteName('id'));
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
37 changes: 24 additions & 13 deletions libraries/joomla/installer/adapters/template.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,17 +284,24 @@ public function install()

if ($this->route == 'install')
{
//insert record in #__template_styles
$query = $db->getQuery(true);
$query->insert('#__template_styles');
$query->set('template=' . $db->Quote($row->element));
$query->set('client_id=' . $db->Quote($clientId));
$query->set('home=0');
$debug = $lang->setDebug(false);
$query->set('title=' . $db->Quote(JText::sprintf('JLIB_INSTALLER_DEFAULT_STYLE', JText::_($this->get('name')))));

$columns = array('template', 'client_id', 'home', 'title', 'params');
$values = array(
$db->Quote($row->element), $clientId, $db->Quote(0),
$db->Quote(JText::sprintf('JLIB_INSTALLER_DEFAULT_STYLE', JText::_($this->get('name')))),
$db->Quote($row->params) );

$lang->setDebug($debug);
$query->set('params=' . $db->Quote($row->params));

//insert record in #__template_styles
$query = $db->getQuery(true);
$query->insert($db->quoteName('#__template_styles'))
Copy link
Owner

Choose a reason for hiding this comment

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

Seems correct, but you've lost two "setDebug" .

Copy link
Author

Choose a reason for hiding this comment

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

Is those really necessary?

Copy link
Owner

Choose a reason for hiding this comment

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

I don't know, they were there so I've maintained them.

Copy link
Author

Choose a reason for hiding this comment

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

Ok. I'll back them.

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

$db->setQuery($query);

// There is a chance this could fail but we don't care...
$db->query();
}
Expand Down Expand Up @@ -359,7 +366,7 @@ public function uninstall($id)

// Deny remove default template
$db = $this->parent->getDbo();
$query = 'SELECT COUNT(*) FROM #__template_styles' . ' WHERE home = 1 AND template = ' . $db->Quote($name);
$query = "SELECT COUNT(*) FROM #__template_styles WHERE home = '1' AND template = " . $db->Quote($name);
Copy link
Owner

Choose a reason for hiding this comment

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

+1

$db->setQuery($query);

if ($db->loadResult() != 0)
Expand Down Expand Up @@ -412,13 +419,17 @@ public function uninstall($id)
}

// Set menu that assigned to the template back to default template
$query = 'UPDATE #__menu INNER JOIN #__template_styles' . ' ON #__template_styles.id = #__menu.template_style_id'
. ' SET #__menu.template_style_id = 0' . ' WHERE #__template_styles.template = ' . $db->Quote(strtolower($name))
. ' AND #__template_styles.client_id = ' . $db->Quote($clientId);

$query = 'UPDATE #__menu'
. ' SET template_style_id = 0'
. ' WHERE template_style_id in ('
. ' SELECT s.id FROM #__template_styles s'
. ' WHERE s.template = '. $db->Quote(strtolower($name)) .' AND s.client_id = '. $clientId .')';

Copy link
Owner

Choose a reason for hiding this comment

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

+1

$db->setQuery($query);
$db->Query();

$query = 'DELETE FROM #__template_styles' . ' WHERE template = ' . $db->Quote($name) . ' AND client_id = ' . $db->Quote($clientId);
$query = 'DELETE FROM #__template_styles WHERE template = ' . $db->Quote($name) . ' AND client_id = ' . $clientId;
Copy link
Owner

Choose a reason for hiding this comment

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

+1

$db->setQuery($query);
$db->Query();

Expand Down