SQL errors during article/blog/forum submit w/ PostgreSQL [#48591]

Blog entries are not inserted into the database. The log shows numerous errors in the pgsql driver.

Cause: conversion of node type's default options fails. If e.g. "promote" is not set, the value inserted into the db is not a zero (like it should be, IMHO) but an empty string (""), which maybe is converted by MySQL, but PostgreSQL (rightly) complains.

Steps to reproduce:

Components PHP 4.4.2, PostgreSQL 8.1.2, Drupal 4.6.5
System: Debian "Sid" / IA32 (updated 2006-02-07)
Steps: install Drupal, activate blogapi, de-select "promote" setting and try to post a blog entry using e.g. "Performancing" or similar.

Fix: is easy. 3 code lines need to be modified in blogapi.module; fix should not break anything, but that is not verified at this moment. Change the following lines in blogapi.module, starting with line 188:

  $edit['name'] = $user->name;
  $edit['promote'] = (int) in_array('promote', $node_type_default);
  $edit['comment'] = variable_get('comment_'. $edit['type'], 2);
  $edit['moderate'] = (int) in_array('moderate', $node_type_default);
  $edit['revision'] = (int) in_array('revision', $node_type_default);

The only change are the explicit type conversions of boolean to int.

Comment	File	Size	Author
#16	node_save-48591_3.diff	2.29 KB	Cvbge
#16
#13	node_save-48591_2.diff	2.66 KB	Cvbge
#13
#12	node_save-48591_1.diff	2.32 KB	Cvbge
#12
#4	node_save-48591_0.diff	3.57 KB	Cvbge
#4

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Kalessin CreditAttribution: Kalessin commented 9 February 2006 at 08:08

Title:	blogapi: SQL errors during article submit w/ PostgreSQL 8.1	» node.module has same problem (SQL errors during article submit w/ PostgreSQL 8.1)
Component:	blogapi.module	» node.module

The node module apparently has the same problem. One hotfix is to do an explicit type cast here as well (node.module, starting line 1248):

    // Force defaults in case people modify the form:
    $node_options = variable_get('node_options_'. $node->type, array('status', 'promote'));
    $node->status = (int) in_array('status', $node_options);
    $node->moderate = (int) in_array('moderate', $node_options);
    $node->promote = (int) in_array('promote', $node_options);
    $node->sticky = (int) in_array('sticky', $node_options);

Maybe one should adapt the PostgreSQL driver, if there are more of these lurking somewhere. The cause seems to be that booleans are

either not interpreted correctly by the pgsql php driver,
or (wrongly) stored as integers inside the db.

In the latter case, IMHO the db driver in Drupal would have to do the conversion, since this can be expected to lead to general problems.

Comment #2

Cvbge CreditAttribution: Cvbge commented 9 February 2006 at 08:47

Title:

node.module has same problem (SQL errors during article submit w/ PostgreSQL 8.1)

» SQL errors during article submit w/ PostgreSQL

Thanks for your report, I'll look into it.

Maybe one should adapt the PostgreSQL driver, if there are more of these lurking somewhere. The cause seems to be that booleans are

This is already done in cvs (future 4.7) Drupal. I really suggest to use that versions if possibile. 4.6 has some other problems with postgresql which are fixed in cvs.

Comment #3

Cvbge CreditAttribution: Cvbge commented 9 February 2006 at 08:48

Assigned:

Unassigned

Comment #4

Cvbge CreditAttribution: Cvbge commented 16 February 2006 at 20:52

Status:

Active

» Needs review

File	Size
node_save-48591_0.diff	3.57 KB

Thanks for your report, it helped me a lot :)

Attached patch which should fix the problems. I've also checked all other node_save() calls. The best would be to change the node_save(), but I'm reluctant to do so as it changes it'd change it behaviour a bit (would send '0' instead of '' for FALSE).

I didn't test it using blogapi, please test if it works for you.

It seems I don't need to cary about $node->revision (note lack of trailing s).

I've changed also the $publish which seems to be boolean and called from blog api; I don't know if it's 1/0 or TRUE/FALSE so I changed it just to be sure.

I've also removed additional arguments from node_save() calls, e.g. from node_save($node, array('nid', 'revisions'));. The function only uses 1 argument so I suppose the additional argument is some left-over?

Comment #5

ankon CreditAttribution: ankon commented 19 February 2006 at 11:21

The patch worked for me, thank you.

System details:
FreeBSD (-current, i386) with
postgresql-server-8.0.6
drupal-4.6.5
drivel-2.0.2

Comment #6

Cvbge CreditAttribution: Cvbge commented 22 February 2006 at 17:31

Priority:	Normal	» Critical
Status:	Needs review	» Reviewed & tested by the community

Since chx wants to release 4.6.6 tommorow...

Comment #7

Cvbge CreditAttribution: Cvbge commented 22 February 2006 at 17:32

Status:

Reviewed & tested by the community

» Needs review

OTOH I want someone with mysql to test it...

Comment #8

Cvbge CreditAttribution: Cvbge commented 22 February 2006 at 18:41

Status:

Needs review

» Reviewed & tested by the community

OTOH...

Comment #9

Dries CreditAttribution: Dries commented 22 February 2006 at 18:47

I think it's better to fix this in node_save(), much like we fixed it in user_save(). Like that, you rely on _db_query() doing the cast, making it work for all contributed modules as well.

Comment #10

Dries CreditAttribution: Dries commented 23 February 2006 at 10:30

Status:

Reviewed & tested by the community

» Needs work

Comment #11

Kalessin CreditAttribution: Kalessin commented 23 February 2006 at 13:54

I have switched my setup to MySQL, choosing the path of least resistance... also it is now in production mode, so I will not play around with it too much. But I have a sandbox Apache here, anyway, so I can set up a clean Drupal site using PostgreSQL again, and check if the patches work in both configurations. But IMHO this only makes sense if the patch is considered to be a candidate for a definite solution, quality-wise.

Comment #12

Cvbge CreditAttribution: Cvbge commented 23 February 2006 at 15:26

Status:

Needs work

» Needs review

File	Size
node_save-48591_1.diff	2.32 KB

I had considered fixing it in node_save(), but decided that'd be "bigger" change and it's simpler to fix the calls.

This patch fixes the node_save() as you suggested. I've tested it with postgres - created/edited a couple of pages/books/blogs and everything seems to work.

Comment #13

Cvbge CreditAttribution: Cvbge commented 23 February 2006 at 15:34

File	Size
node_save-48591_2.diff	2.66 KB

We don't need to call 'fields' nodeapi.

Comment #14

chx CreditAttribution: chx commented 23 February 2006 at 16:32

Well, the problem is that we have a 'fields' nodeapi op. Rare but it's used. If someone modifies node table for speed, then he'll be in trouble here. You can modify the op to return 'field' => %d or %s but if you do so, then it's an API change in a minor version? Dries, what's your take on that? It's a VERY obscure place of API and the modification is minor.

Comment #15

Cvbge CreditAttribution: Cvbge commented 23 February 2006 at 19:04

Hmm I see.

I could change the patch so it takes the 'fields' into consideration, and treats unknown fields as strings. That would work for core, and if someone writes it's own module - he should make sure the value is apropriate.

Better solutions would be to return %d/%s as you say. grep -rE '_nodeapi.*fields' showed only 1 module which uses it (import_export.module). My copy of contrib modules might be a bit old though.

Comment #16

Cvbge CreditAttribution: Cvbge commented 23 February 2006 at 19:23

File	Size
node_save-48591_3.diff	2.29 KB

Here.

I didn't use db_escape_string(), because it's false security. It only escapes characters e.g. on http://pl2.php.net/manual/en/function.mysql-real-escape-string.php. E.g. ";" is not escaped at all. If we want to do it right, we should write some db_check_correct_field_name() [which would in fact accept only [a-ZA-Z0-9_] and maybe some others].

Comment #17

Cvbge CreditAttribution: Cvbge commented 8 March 2006 at 14:03

Title:

SQL errors during article submit w/ PostgreSQL

» SQL errors during article/blog/forum submit w/ PostgreSQL

Marked http://drupal.org/node/28996 as duplicate of this issue.

Comment #18

Dries CreditAttribution: Dries commented 9 March 2006 at 14:57

If only one contrib module uses it, I'm ok with dropping that fields-functionality in favor of this patch (i.e. it makes many modules work with PostgreSQL).

Is this patch only required for 4.6?

Comment #19

rkerr CreditAttribution: rkerr commented 24 March 2006 at 00:44

Does this problem still exist in HEAD (soon to be 4.7)?

db_escape_string should still be called for the values put into the string fields of the queries. Escaping ';' is sort of irrelevant because php will only ever execute one sql command per function call. The db_escape_string will be needed to avoid even simple parse errors with single-quotes and whatnot, won't it?

Also, I'd rather see one "case x:" per line, but that could just be my personal preference and not anything to do with Drupal coding standards.

Comment #20

Zen CreditAttribution: Zen commented 24 March 2006 at 06:22

rkerr: http://drupal.org/node/48591#comment-72205

-K

Comment #21

Cvbge CreditAttribution: Cvbge commented 24 March 2006 at 20:59

Version:

4.6.5

» 4.6.6

I was a bit wrong. I've checked again 4.6 modules and:

comment module implements 'fields' and returns correct data ('comment' column)
i18n module implements 'fields' and returns correct data
paging module implements 'fields' nodeapi (but does not return fields array but modifies $node. A hack probably)

So the 'fields' nodeapi call is needed after all. Also, I'd prefer not to remove this functionality from stable branch.

This leaves us with http://drupal.org/node/48591#comment-75905 patch which takes 'fields' nodeapi into consideration (it might not work for some contrib modules, but nothing can be done with this without changing the 'fields' semantic)

Escaping ';' is sort of irrelevant because php will only ever execute one sql command per function call.

I think you're wrong. IIRC I tried it once and it worked. http://pl.php.net/pg_query also says this about query:

The SQL statement or statements to be executed. When multiple statements are passed to the function, they are automatically executed as one transaction

which seems to suggest that you can have more than one statement in the query.

One of comments for mysql_query also suggests you can send many statements in one query.

Comment #22

rkerr CreditAttribution: rkerr commented 25 March 2006 at 02:28

Ah, sorry. Perhpas I was remembering really old information, or just bad information :)
The string values should probably still be escaped though, or is that already handled by db_query itself?

Comment #23

Cvbge CreditAttribution: Cvbge commented 25 March 2006 at 08:54

The string values should probably still be escaped though, or is that already handled by db_query itself?

If you're talking about values that are substituted for %foo escapes, then yes, db_query calls db_escape_string() on the values.

Comment #24

beginner CreditAttribution: beginner commented 14 August 2006 at 18:56

Priority:

Critical

» Normal

Cvbge in #2 said:

This is already done in cvs (future 4.7) Drupal. I really suggest to use that versions if possibile. 4.6 has some other problems with postgresql which are fixed in cvs.

This issue can't be that critical, as
1) it affects only people using PostgreSQL who are a very small minority.
2) 4.6 was buggy for PostgreSQL users, so that most or all of those probably upgraded to 4.7 a long time ago.
3) nobody has complained about this bug in a long time, anyway.

=> downgrading.
Set as fixed, wontfix?