taxonomy.module broken with Postgres in CVS

OK, I found some similar code in the function taxonomy_save_vocabulary() in taxonomy.module where the 'tags' checkbox is handled:

  $edit['tags'] = ($edit['tags']) ? $edit['tags'] : 0;

So i removed my previous additions (which were in taxonomy_admin() by the way) and added these three lines to taxonomy_save_vocabulary() instead:

   $edit['multiple'] = ($edit['multiple']) ? $edit['multiple'] : 0;
   $edit['required'] = ($edit['required']) ? $edit['required'] : 0;
   $edit['relations'] = ($edit['relations']) ? $edit['relations'] : 0;              

Attached is a new patch.

And here's some info about my setup:

PostgreSQL 7.5.11
PHP 4.3.10
Apache 2.0.54
Drupal CVS, Oct 24 2005

Another case of the same problem turned up when I started adding terms to the vocabulary. It's the "weight" value that is not specified as '0'.

In the function taxonomy_save_term() I added:

  $edit['weight'] = ($edit['weight']) ? $edit['weight'] : 0 ;

This problem seems to be the same as the one described here: http://drupal.org/node/23690

The attached patch deals with both these issues. There may be more updates as I keep working with my taxonomies...

It would be nice if someone could confirm that I'm not just being an idiot and that this issue actually exists. :) Maybe we should even set the priority to critical?

Right, thanks for the pointer! :)
Here's another patch.

Any thoughts on the actual code?

That's nice.
I'm only wondering if this kind of stuff should be handled by the new Form API's validation mechanism. I'm not so familiar with it, so I don't know. Is taxonomy.module converted to it yet?

Anyway, the patch solves the problem and it doesn't add any new concepts to the code - it just repeats stuff that was already there. Might as well use it for now.

First, I'm new to the Drupal APIs and code structure so there's obviously a lot of things I don't know about it, but here are my thoughts:

I agree that it's not ideal to solve a general problem like this in countless places in module code. If there's a general solution to it, that would be better. But since modules construct the actual SQL queries that get sent to the database (don't they?), I guess they also have to make sure the queries are valid (in terms of what PostgreSQL and MySQL accepts).

The way it is now, taxonomy.module generates queries that try to set integer columns to ''. I don't see how Drupal's database layer can change that in any reasonable way.

Maybe the form layer is capable of checking that a certain value is always returned as an integer? The functions that define the forms are: taxonomy_form_vocabulary() and taxonomy_form_term()

If there is no such general solution, this has to be dealt with locally. So, here is the code that constructs the values in INSERT and UPDATE queries:

[...]
function _taxonomy_prepare_update($data) {
  foreach ($data as $key => $value) {
    $q[] = "$key = '". str_replace('%', '%%', db_escape_string($value)) ."'";
  }
[...]

function _taxonomy_prepare_insert($data, $stage) {
[...]
    foreach (array_values($data) as $value) {
      //Escape strings, but not integers
      if (is_int($value)) {
        $q[] = $value;
      }
      else {
        $q[] = "'". str_replace('%', '%%', db_escape_string($value)) ."'";
      }
    }
[...]

These two functions know nothing about the columns and their types, so the only way they can distinguish between different data types is by checking the actual PHP value. That means that the values (in $data) have to be the correct types before they get here. An empty string is not an integer, so is_int won't catch it.

(And BTW, why doesn't _taxonomy_prepare_update() check for ints when _taxonomy_prepare_insert() does? Should I add that to the patch?)

On the other hand, taxonomy_save_vocabulary() and taxonomy_save_term() deal with actual column names explicitly, so that's a better place to check for empty strings that correspond to integer columns. This already happens with the 'tags' column - all this patch does is adding similar checks for 'multiple', 'required', 'relations' and 'weight'.

So, should the patch be committed?

Cvbge, the issue you refer to is here: http://drupal.org/node/17656 (You pasted the wrong url.)

I read the discussion and I like the approach of building queries in one common place. This would replace _taxonomy_prepare_insert() and _taxonomy_prepare_update().

But, each module will still have to supply the values in their correct types. So the patch is still needed as far as I can see, with or without the new db_query().

(A nice feature for module developers would be to integrate form generation, validation and sql building so that each part is aware the each other. One would specify form fields, validation rules and database types once and the conversion would be handled automatically. But that's a kind of big project...)

OK, I've looked into this a bit more, and I can see that the proposed changes to db_query() described in http://drupal.org/node/17656 could be a solution to this problem (as all null values would be converted to 0). It would require that taxonomy.module was reworked to take advantage of the new functionality however. Until that patch gets committed, my patch works as a temporary solution.

I'm changing the status to critical as this bug definitely has to be fixed before 4.7 can be released.

We need this patch or some other solution. This module doesn't use the string formatting capabilities of db_query() – that would be another (preferred?) way to solve it, although it would require more changes to the code. I guess that's what Dries was talking about before, I just didn't get it. :)

Wouldn't that solution require that taxonomy passed the queries with %-modifiers to db_query()? The problem here I think is that taxonomy.module builds it's own queries by simply implode()ing arrays without first checking for empty values.

Yes, that's better. Nice filename standard for the patch too! :)

Alright, here's another patch based on Dries' comments. I've only tested it very briefly.

I have identified 4 calls to db_query() that don't use string formatting and thus are vulnerable to type errors. I changed those so that they specify each field explicitly, and use type modifiers, rather than simply implode()ing an array. Two functions became obsolete – _taxonomy_prepare_insert() and _taxonomy_prepare_update() – so I removed those.

(I can think of other ways to achieve the same thing, for example extending those two functions to handle type modifiers, but I doubt if it's worth it.)

The code is now somewhat less readable than before and maybe a little bit harder to maintain, but the compatibility issues should be gone and I think it conforms better to the way db_query() is meant to be used.

I made the calls to db_query() span over two lines to increase readability – maybe that's a deadly sin, I don't know. Just let me know and I'll change that.

Sorry, forgot to include Cvbge's PHP5 compatibility changes... We should do:

  $edit['nodes'] = empty($edit['nodes']) ? array() : $edit['nodes'];

...rather than...

  $edit['nodes'] = ($edit['nodes']) ? $edit['nodes'] : array();

What the... Empty file? Trying again.