region is defined as NOT NULL with default value left within block.schema:

<?php
  $schema['blocks'] = array(
    'fields' => array(
      'bid'        => array('type' => 'serial', 'not null' => TRUE),
      'module'     => array('type' => 'varchar', 'length' => 64, 'not null' => TRUE, 'default' => ''),
      'delta'      => array('type' => 'varchar', 'length' => 32, 'not null' => TRUE, 'default' => '0'),
      'theme'      => array('type' => 'varchar', 'length' => 255, 'not null' => TRUE, 'default' => ''),
      'status'     => array('type' => 'int', 'not null' => TRUE, 'default' => 0, 'size' => 'tiny'),
      'weight'     => array('type' => 'int', 'not null' => TRUE, 'default' => 0, 'size' => 'tiny'),
      'region'     => array('type' => 'varchar', 'length' => 64, 'not null' => TRUE, 'default' => 'left'),
      'custom'     => array('type' => 'int', 'not null' => TRUE, 'default' => 0, 'size' => 'tiny'),
      'throttle'   => array('type' => 'int', 'not null' => TRUE, 'default' => 0, 'size' => 'tiny'),
      'visibility' => array('type' => 'int', 'not null' => TRUE, 'default' => 0, 'size' => 'tiny'),
      'pages'      => array('type' => 'text', 'not null' => TRUE),
      'title'      => array('type' => 'varchar', 'length' => 64, 'not null' => TRUE, 'default' => '')
    ),
    'primary key' => array('bid'),
  );
?>

this patch try to use 'left' (default value) rather than '', as it is not oracle friendly. it should not change overall result, but increase cross DB compatibility.

CommentFileSizeAuthor
drupal-6.x-dev-block_region_cant_be_null-1.0.diff1009 byteshswong3i

Comments

hswong3i’s picture

hswong3i commented
Title: block.module: region can't set to null » update block.module as SQL friendly
dries’s picture

dries commented

This could should specify the defaults:

        // Otherwise, use any set values, or else substitute defaults.
        else {
          $properties = array('status' => 0, 'weight' => 0, 'region' => 'left', 'pages' => '', 'custom' => 0, 'title' => '');
          foreach ($properties as $property => $default) {
            if (!isset($block[$property])) {
              $block[$property] = $default;
            } 
          }
        }

Might be a good idea to figure out why these aren't set at the time the block is inserted.

hswong3i’s picture

hswong3i commented

my partner (mike2854: http://drupal.org/user/31933) report that:

funny question ~~
as it is update instead of insert record,
the default value in table is useless,
so it try to insert null to oracle table and return error
in mysql,
seems the default value works in any change
including updating value ~~

oh my god that is mysql default handling... so, change can make case be oracle friendly ;-)

hswong3i’s picture

hswong3i commented

this patch just use the default value from schema back, if it is asking for default value. it seems to be harmless, and just as safety checking ;)

hswong3i’s picture

hswong3i commented
Status: Needs review » Reviewed & tested by the community

testbed with MySQL 5.0.38 and PHP 5.2.0-10. test passed: no error message during process admin page submit. is harmless to mysql.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Needs work

I think the proper solution is to allow '' to be inserted. My database has '' regions as you can see below:

mysql> select region, count(region) from blocks group by region;
+--------+---------------+
| region | count(region) |
+--------+---------------+
|        |             4 |
| left   |             2 |
+--------+---------------+

Forcing 'left' to be inserted changes this to:

mysql> select region, count(region) from blocks group by region;
+--------+---------------+
| region | count(region) |
+--------+---------------+
| left   |             6 |
+--------+---------------+

This looks OK as it doesn't change the behavior of the block administration page, however, it highlights a problem with the schema API. If defaults are ignored by Oracle, then it is probably a bad idea to define defaults to begin with?

See:

'region'     => array('type' => 'varchar', 'length' => 64, 'not null' => TRUE, 'default' => 'left'),

This patch seem to work around a more fundamental problem?

hswong3i’s picture

hswong3i commented

a good question with complicated solution... you are totally correct: if this is the only problem of oracle, then it should be handle within driver.

for current status, i think we may try to reload the default value from *.schema, or even from database directly, whenever we face a query with value not defined. this will greatly increase the workload of driver: as we may always need to do such mapping. i may ask mike to work it out, but it seems need a bit of time.

on the other hand, i think such mapping will not be the only exceptional case among oracle driver, if we are trying to force oracle working as similar as other DB as possible. e.g. the mapping of not null, mapping of long-to-short name, and even this.

i am now planning to add a "dynamic optimized query caching layer" within oracle driver: so whenever a new query come in, oracle driver will try to do whatever optimization as it required to, and then cache it. we will then simply reload such cached result in coming on query, to boost up the performance ;D

hswong3i’s picture

hswong3i commented
Status: Needs work » Reviewed & tested by the community

i try to discuss this problem with mike and we think this should be a bug fix. as stayed in #6, the record is not saved as expected within DB, even under mysql version: it should store 'left' or other stuff, but not ''(empty string) or NULL.

we guess that some checking is preformed after fetch result, catch the exceptional case (empty string), and apply 'left' as default value. this should be function under mysql, since mysql seems NULL and empty string as different, where oracle seems that as the same and face error.

according to programming logic, we should always ensure data correctness before storing it into DB, rather than catching the exceptional case afterward. according to this logic, this patch should be acceptable :D

hswong3i’s picture

hswong3i commented

this patch is now depended on oracle driver version 5.3 for drupal-6.x-dev (please refer to #113), and should be committed before that ;D

bjaspan’s picture

bjaspan commented

subscribe

bjaspan’s picture

bjaspan commented

I do not understand what this issue is about. If column region has default 'left', then when you INSERT into table blocks in a way that requests the default value for the region column, you get 'left'. (You request the default value by specifying a column list to insert which does not include region, or using the keyword DEFAULT in some databases, or maybe specifying NULL in NOT NULL column, but I'm not sure about that last one.)

However, if you explicity insert the value '' for the column region, you should get the value ''. You should NOT get 'left'. The default value does not mean "what to insert when I insert the empty string", it means "what to insert when I do not provide any value at all."

Finally, default values do not affect UPDATE statements at all. If you UPDATE block SET region='', you should get '', not 'left'; same reason as above. (UPDATE block SET region DEFAULT would give you 'left' on databases that support the DEFAULT keyword in that way.)

I feel like the issue here is that for some reason there are rows in the block table with the region column having value ''. That may or may not indicate a bug depending on whether it is intentional.

So, given all that, am I missing something? If so, please clarify it, as I am confused.

bjaspan’s picture

bjaspan commented

Continuing my previous comment, I do not see a problem with the schema definition or schema API here. The schema for the block table says the region column has a default of 'left', but block_admin_display_submit() uses UPDATE to set it to ''. The UPDATE works, and we get '' in the column. If someone thought setting region to '' should have set it to 'left', they were wrong.

At first glance this looks like a simple bug in block_admin_display_submit() which the patch on this issue fixes. I am not familiar enough with block.module to say for sure if it is right; I don't know if assigning the region to '' was intentional.

I do not see how this has anything to do with Oracle compatibility. I think hswong3i just stumbled on a bug in block.module while working on Oracle support.

hswong3i’s picture

hswong3i commented
Status: Reviewed & tested by the community » Fixed

i feel very sorry about this, it is my fault...

i get your meanings about such block handling, and so double check the correctness of block_admin_display_submit(). i found that with no error, even though checking with active, deactive, move, create and delete! i check the database too, and get similar result as #6:

41   user       0   garland   1   0       left   0   0   0
42   user       1   garland   1   0       left   0   0   0
43   user       2   garland   0   0              0   0   0
44   user       3   garland   0   0              0   0   0
39   comment    0   garland   0   0              0   0   0
40   node       0   garland   0   0              0   0   0

it seems too crazy that without error, and so i ask mike about this problem again. he replay me as:

this is the problem that i found during working with drupal-5.1 and oracle driver. i guess that will also happening with drupal-6.x-dev...

it is all due to my careless. i used to double checking it before submitting a bug report... it is now completely function with oracle driver :)

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)