Skip to content
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

Fixes and improvements for postgresql #3

Open
wants to merge 15 commits into
base: postgresql_v2
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -1831,7 +1831,7 @@ CREATE TABLE "#__modules" (
"id" serial NOT NULL,
"title" character varying(100) DEFAULT '' NOT NULL,
"note" character varying(255) DEFAULT '' NOT NULL,
"content" text NOT NULL,
"content" 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.

MySQL version hasn't default value, see here, why have you added this ?

Copy link
Author

Choose a reason for hiding this comment

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

I must verify what is the exactly error this generated. But as I can remember, PostgreSQL is not so fair like MySQL with bad SQL.

Copy link
Author

Choose a reason for hiding this comment

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

I tested this today and this is not necessary anymore. I think I changed this before last fix in postgresql driver. This is related to problems with null values. I will revert this.

Copy link
Owner

Choose a reason for hiding this comment

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

Ok, I'll wait your changes and then I'll merge.
Thanks!

"ordering" bigint DEFAULT 0 NOT NULL,
"position" character varying(50) DEFAULT '' NOT NULL,
"checked_out" integer DEFAULT 0 NOT NULL,
Expand All @@ -1842,7 +1842,7 @@ CREATE TABLE "#__modules" (
"module" character varying(50) DEFAULT NULL,
"access" bigint DEFAULT 0 NOT NULL,
"showtitle" smallint DEFAULT 1 NOT NULL,
"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.

same here as above.

Copy link
Author

Choose a reason for hiding this comment

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

As I mentioned above. I didn't test modules installation with MySQL version. But with PostgreSQL this is absolutely necessary for modules with no parameters in XML file. May be this is a missed bug in MySQL version.

Copy link
Owner

Choose a reason for hiding this comment

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

Are there cases with these two fields having null value ?
How can I replicate the error you're having for these two fields?

Thanks!

Copy link
Author

Choose a reason for hiding this comment

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

I tested this right now and defacto any module you try to install fail if params has not default to ''

Copy link
Owner

Choose a reason for hiding this comment

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

Ok, thanks!

"client_id" smallint DEFAULT 0 NOT NULL,
"language" character varying(7) NOT NULL,
PRIMARY KEY ("id")
Expand Down
53 changes: 49 additions & 4 deletions libraries/joomla/database/database/postgresql.php
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,8 @@ public function insertid()
public function insertObject($table, &$object, $key = null)
{
// Initialise variables.
$columns = $this->getTableColumns($table);

$fields = array();
$values = array();

Expand All @@ -467,7 +469,7 @@ public function insertObject($table, &$object, $key = null)

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

// Create the base insert statement.
Expand Down Expand Up @@ -1043,6 +1045,47 @@ public function replacePrefix($sql, $prefix = '#__')
return $replacedQuery;
}

/**
* This function return a field value as a prepared string to be used in a SQL statement.
*
* @param string $table_fields The table fields types returned by ::getTableColumns.
* @param string $value The php variable value.
*
Copy link
Owner

Choose a reason for hiding this comment

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

A comment for $columns is missing.

Maybe it's better to have this member private or protected.

Copy link
Author

Choose a reason for hiding this comment

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

I agree. I will fix this tonight.

Copy link
Author

Choose a reason for hiding this comment

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

This function enables contacts and all other CMS modules with no modification from master version (mysql). This is accomplished by fixing nulls of dates and numbers for SQL statements in database driver.

Copy link
Owner

Choose a reason for hiding this comment

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

Ok, after merging this change I'll add on platform code and I'll create tests for it.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Owner

Choose a reason for hiding this comment

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

Perfect.

Just a note: I've already added this method inside platform, as you can see I haven't used reference for $columns and I've to change it, I've handled boolean differently, I've added timestamp without time zone type and doing its test I've seen that numeric needs to be different, including a comma in its $field_name value, it's not handled otherwise.

So some suggestion to improve this method:

  1. make this method protected because no one externally have to call this.
  2. try to use my changes about types (numeric, boolean, timestamp without time zone)
  3. test if cms works with these changes, so I can merge this branch and change this method inside platform

Really thank you ;) !

* @return string The quoted string.
*
* @since 11.3
*/
public function sqlValue($columns, $field_name, $field_value)
{
switch ($columns[$field_name]) {
case 'boolean':
if ($field_value == 't')
{
$field_value = true;
}
$val = is_bool($field_value) ? ( $field_value ? 'TRUE' : 'FALSE' ) : 'NULL';
break;
case 'bigint':
case 'bigserial':
case 'integer':
case 'money':
case 'numeric':
case 'real':
case 'smallint':
case 'serial':
$val = strlen($field_value) == 0 ? 'NULL' : $field_value;
break;
case 'date':
if (empty($field_value))
{
$field_value = $this->getNullDate();
}
default:
$val = $this->quote($field_value);
}
return $val;
}

/**
* Method to commit a transaction.
*
Expand Down Expand Up @@ -1154,6 +1197,8 @@ public function unlockTables()
public function updateObject($table, &$object, $key, $nulls = false)
{
// Initialise variables.
$columns = $this->getTableColumns($table);

$fields = array();
$where = '';

Expand All @@ -1174,7 +1219,8 @@ 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) . '=' . (is_numeric($v) ? $v : $this->quote($v));
$key_val = $this->sqlValue($columns, $k, $v);
$where = $this->quoteName($k) . '=' . $key_val;
continue;
}

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

// Add the field to be updated.
Expand Down
2 changes: 1 addition & 1 deletion libraries/joomla/installer/adapters/component.php
Original file line number Diff line number Diff line change
Expand Up @@ -541,7 +541,7 @@ public function install()
return false;
}

$eid = $row->$key;
$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.

Are you retrieving errors about 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.

Yes. A "there is not $key variable defined" error. This happen because there is no "key" variable in function body. If you look at body's function there is some other lines using $row->extension_id just like I did. So, I decided to let the function like in master branch (using $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.

Perfect.


// Clobber any possible pending updates
$update = JTable::getInstance('update');
Expand Down