1. I was testing my system to update a couple of sites from D6.16 to D6.26
  2. I cloned my dev machine to perform the test and install Wamp 2.2 on my tests device.
  3. I clone a site there and update it to D6.26
  4. I moved a inactive block to leftsidebar and recived a long list of warningns:
  5. warning: Illegal string offset 'region' in E:\www\biblioteca\modules\block\block.admin.inc on line 101.
    warning: Illegal string offset 'status' in E:\www\biblioteca\modules\block\block.admin.inc on line 101.
    warning: Illegal string offset 'status' in E:\www\biblioteca\modules\block\block.admin.inc on line 102.
    warning: Illegal string offset 'region' in E:\www\biblioteca\modules\block\block.admin.inc on line 102.
    warning: Illegal string offset 'region' in E:\www\biblioteca\modules\block\block.admin.inc on line 102.
    warning: Illegal string offset 'status' in E:\www\biblioteca\modules\block\block.admin.inc on line 103.
    warning: Illegal string offset 'weight' in E:\www\biblioteca\modules\block\block.admin.inc on line 103.
    warning: Illegal string offset 'region' in E:\www\biblioteca\modules\block\block.admin.inc on line 103.
    warning: Illegal string offset 'module' in E:\www\biblioteca\modules\block\block.admin.inc on line 103.
    warning: Illegal string offset 'delta' in E:\www\biblioteca\modules\block\block.admin.inc on line 103.
    
    ...
    + a lot of similar lines (at least 3 per defined block)
    
  6. I installed two new fresh sites to verify if (perhaps) i had a wrong cloned site
  7. both sites had the same issue
  8. I installed D 6.25 over the same sys and repeat a block move. Same issue again
  9. I created a new clon of my dev machine
  10. This time installed Wamp 2.1 instead Wamp 2.2
  11. I created 3 new drupal fresh sites (6.16, 6.25, 6.26)
  12. NO ISSUES THIS TIME
  13. other configuration info
  14. All the tests where performed over NT4. service-pack 2
    Embedded on a Linux machine (Ubuntu 11) over Oracle VM VirtualBox (4.1.18)
    [Wamp 2.2]
    apache 2.2.22
    php 5.4.3
    MySQL 5.5.24
    
    [Wamp 2.1]
    apache 2.2.17
    php 5.3.5
    MySQL 5.5.8
    

    Is it possible we need extra params over newer versions of apache, mysql and php, not needed before?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vigneshbabu’s picture

Hi,

Any idea on why this was happening?

Thanks in advance

-Vignesh

GDI’s picture

Version: 6.26 » 6.25

Have the same issue with Drupal 6.25.
I think this issue caused by PHP 5.4

Gulzar’s picture

see the warning goto mentioned line and remove "&"

thanks

raymacz’s picture

I have exactly the same issues as this one.. Hoping we have a fix soon... Thanks!

mdupont’s picture

Version: 6.25 » 6.x-dev

This is caused by PHP 5.4 that raises warnings for some behaviors that were valid with previous versions of PHP. Drupal 6 does not support PHP 5.4 and likely never will since EOL is not far away.

NaX’s picture

I think I found the problem. I changed block_admin_display_form_submit() to check for an array():

/**
 * Process main blocks administration form submission.
 */
function block_admin_display_form_submit($form, &$form_state) {  
  foreach ($form_state['values'] as $block) {
    if (is_array($block)) {
      $block['status'] = $block['region'] != BLOCK_REGION_NONE;
      $block['region'] = $block['status'] ? $block['region'] : '';    
      db_query("UPDATE {blocks} SET status = %d, weight = %d, region = '%s', throttle = %d WHERE module = '%s' AND delta = '%s' AND theme = '%s'", $block['status'], $block['weight'], $block['region'], isset($block['throttle']) ? $block['throttle'] : 0, $block['module'], $block['delta'], $block['theme']);
    }
  }
  drupal_set_message(t('The block settings have been updated.'));
  cache_clear_all();
}

The problem seems to be that block_admin_display_form_submit() handler is just looping through all the form values assuming each value is a valid block settings form array() and when it hits fields relating to the buttons and form ID then it errors because it is not an array.

I dont know if its nessary but the more correct way could be to loop through blocks.

/**
 * Process main blocks administration form submission.
 */
function block_admin_display_form_submit($form, &$form_state) {  
  $blocks = _block_rehash();
  foreach ($blocks as $block_data) {
    $key = $block_data['module'] .'_'. $block_data['delta'];
    $block = $form_state['values'][$key];
    $block['status'] = $block['region'] != BLOCK_REGION_NONE;
    $block['region'] = $block['status'] ? $block['region'] : '';    
    db_query("UPDATE {blocks} SET status = %d, weight = %d, region = '%s', throttle = %d WHERE module = '%s' AND delta = '%s' AND theme = '%s'", $block['status'], $block['weight'], $block['region'], isset($block['throttle']) ? $block['throttle'] : 0, $block['module'], $block['delta'], $block['theme']);
  }
  drupal_set_message(t('The block settings have been updated.'));
  cache_clear_all();
}

Hope it helps

RStrydom’s picture

FileSize
1.48 KB

#6 Patch works for me

I have re-rolled the patch, built against 6.28

RStrydom’s picture

FileSize
1.24 KB

Apologies.

attached re-rolled with correct syntax

NaX’s picture

Status: Active » Needs review
ibnkhaldun’s picture

I have the same issue on Drupal 6.28 !
I'll test your fix, just now ...

Why does the issue exists when there is a patch ?

ridavao’s picture

Title: I recived a long warnings' list on changing a block position » #8 solution works
Status: Needs review » Reviewed & tested by the community

the patch works perfect! thanks drupal community!

NaX’s picture

Title: #8 solution works » I recived a long warnings' list on changing a block position

Changing issue title back

pfaocle’s picture

Thanks for this, just applied it & seems to fix the issue. (Some trailing whitespace has been added in the patch though.)

TJ’s picture

#8: block.admin_.inc_.patch queued for re-testing.

ibnkhaldun’s picture

The problem re-apears on Drupal 6.29
I've updated a test site from D6.28 to D6.29 two days ago. So replaced the block module.
I remember this issue, went to bloks list and moved 1. Surprice...
warning: Illegal string offset 'region' in /home/user/lampstack-5.4.11-0/apache2/htdocs/herramientasd6/modules/block/block.admin.inc on line 101.
warning: Illegal string offset 'status' in /home/user/lampstack-5.4.11-0/apache2/htdocs/herramientasd6/modules/block/block.admin.inc on line 101.
warning: Illegal string offset 'status' in /home/user/lampstack-5.4.11-0/apache2/htdocs/herramientasd6/modules/block/block.admin.inc on line 102.
at least 30 warning rows. I'll retest the patch idea over 6.29

As I was almost sure the propoused patch is misdirected. I perform a test to read the $form_state['values'] content because NOT ALL of it's elements are block definitions. It means there are several cases where $block[$key] is undefined so raises errors.

// HERE IS THE $form_state['values'] content
<?php
$form_state['values'] = array(
  "user_0" => array(
    "module" => "user",
    "delta" => "0",
    "theme" => "garland",
    "weight" => "0",
    "region" => "left",
  ),
  "user_1" => array(
    "module" => "user",
    "delta" => "1",
    "theme" => "garland",
    "weight" => "0",
    "region" => "left",
  ),
  "system_0" => array(
    "module" => "system",
    "delta" => "0",
    "theme" => "garland",
    "weight" => "-6",
    "region" => "footer",
   ),
   "locale_0" => array(
     "module" => "locale",
     "delta" => "0",
     "theme" => "garland",
     "weight" => "-6",
     "region" => "footer",
  ),
  "comment_0" => array(
    "module" => "comment",
    "delta" => "0",
    "theme" => "garland",
    "weight" => "-6",
    "region" => "-1",
  ),
  "user_3" => array(
    "module" => "user",
    "delta" => "3",
    "theme" => "garland",
    "weight" => "-5",
    "region" => "-1",
  ),
  "menu_secondary-links" => array(
    "module" => "menu",
    "delta" => "secondary-links",
    "theme" => "garland",
    "weight" => "-4",
    "region" => "-1",
  ),
  "node_0" => array(
    "module" => "node",
    "delta" => "0",
    "theme" => "garland",
    "weight" => "-3",
    "region" => "-1",
  ),
  "book_0" => array(
    "module" => "book",
    "delta" => "0",
    "theme" => "garland",
    "weight" => "-2",
    "region" => "-1",
  ),
  "menu_primary-links" => array(
    "module" => "menu",
    "delta" => "primary-links",
    "theme" => "garland",
    "weight" => "-1",
    "region" => "-1",
  ),
  "user_2" => array(
    "module" => "user",
    "delta" => "2",
    "theme" => "garland",
    "weight" => "0",
    "region" => "-1",
  ),
  "op" => "Guardar bloques",
  "submit" => "Guardar bloques",
  "form_build_id" => "form-MDp7A-q38KEPNA9sdRAeMCaCbWAssDZmk7qGlYDJ7t8",
  "form_token" => "7c3d2c352dc01087b7f1723b9a10b5dc",
  "form_id" => "block_admin_display_form",
);
?>

And as you can see: the last 5 elements aren't arrays so aren't block definitions. Thus you got error when iterate over them.

I conclude:
The patch must test if gets or not an array So, pls return to first option at comment #6

NaX’s picture

@ibnkhaldun
I don't think just checking if it is an array is very safe or secure. I think the patch #8 is best because it only uses defined blocks. Maybe we can combine the two and check if the key exists in the array before updating. But if the block key does not exist that is an error and should not be suppressed or worked around. If the key does not exist and we prevent the warning then we should at least log to watchdog the error, because the error could point to problems higher up the submission stack.

Just my 2 cents.

ibnkhaldun’s picture

@NaX
As i can see, the loop over $form_state['values'] is the main why to retrive submited values. So $blocks = _block_rehash(); is an unnecesary extra proccess. We only need to exclude 5 keys. Other are blocks. So adding a conditional to exclude is safe.

<?php
/**
 * The patch I'm currently using on my production sites
 * Process main blocks administration form submission.
 */
function block_admin_display_form_submit($form, &$form_state) {
  foreach ($form_state['values'] as $block) {
    if(is_array($block)){
      $block['status'] = $block['region'] != BLOCK_REGION_NONE;
      $block['region'] = $block['status'] ? $block['region'] : '';
      db_query("UPDATE {blocks} SET status = %d, weight = %d, region = '%s', throttle = %d WHERE module = '%s' AND delta = '%s' AND theme = '%s'", $block['status'], $block['weight'], $block['region'], isset($block['throttle']) ? $block['throttle'] : 0, $block['module'], $block['delta'], $block['theme']);
    }
  }
  drupal_set_message(t('The block settings have been updated.'));
  cache_clear_all();
}
?>

A second way is to exclude using explicit keys:

<?php  
/**
 * Process main blocks administration form submission.
 */
function block_admin_display_form_submit($form, &$form_state) {
  $to_exclude = array("op",  "submit", "form_build_id", "form_token", "form_id");
  foreach ($form_state['values'] as $key => $block) {
    if(!in_array($key, $to_exclude)){
      $block['status'] = $block['region'] != BLOCK_REGION_NONE;
      $block['region'] = $block['status'] ? $block['region'] : '';
      db_query("UPDATE {blocks} SET status = %d, weight = %d, region = '%s', throttle = %d WHERE module = '%s' AND delta = '%s' AND theme = '%s'", $block['status'], $block['weight'], $block['region'], isset($block['throttle']) ? $block['throttle'] : 0, $block['module'], $block['delta'], $block['theme']);
    }
  }
  drupal_set_message(t('The block settings have been updated.'));
  cache_clear_all();
}
?>
NaX’s picture

@ibnkhaldun
Here is a scenario for you, let me now what you think.

You have two admin's both working on blocks. One deletes a block the other re-orders the blocks. The second admin loads the page before the first admin deletes the block, but submits the re-order update after the block is deleted. With the array method the user will get a SQL error trying to update a record that does not exist. With the Block rehash method only blocks will be updated that exist in the system at that time and form values that are no-longer relevant will be ignored. Maintain referential integrity, security and not scaring the user will irrelevant error messages.

What do you think? Do you think this scenario is relevant?

ibnkhaldun’s picture

Completely on agree.

Then wath about if 1 admin deletes a block while the 2nd updates it?
It's the same scenario 2 admins working, each one his own row ... a extrange site but possible

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: block.admin_.inc_.patch, failed testing.

Jamie Holly’s picture

IMHO this issue should pertain just to the warning, since that can quickly add up to a bunch of writes to the dblog and/or error logs. To suppress that, the is_array method works great. As far as the scenario of two admins doing block work at the same time, I can easily see that happening, but that should honestly be made into a separate issue.

mw4ll4c3’s picture

Status: Needs work » Needs review
Issue tags: +PHP 5.4, +E_STRICT
FileSize
862 bytes

Here's a clean, freshly rolled patch for the scalar key warnings. Just an is_array() one-liner, no _rehash().

100% with Jamie; anything more should be its own issue, if anyone is inspired. I agree the oversight here could have once lead to unexpected behavior, but that behavior was established long ago. Anyone doing anything "interesting" with the form either added their own submitter to the chain, or found a better home for their changes, long ago.

ibnkhaldun’s picture

Status: Needs review » Reviewed & tested by the community
ibnkhaldun’s picture

I think the patch on #22 is good enough. It's clean and does not run expensive reloads.

jonathan1055’s picture

Title: I recived a long warnings' list on changing a block position » Illegal string offset 'region' and 'status' when saving block admin

Patch in #22 is more approriate for this issue. I agree - RTBC

xaa’s picture

#22 working. thank you. (with drupal6.33)

JamesK’s picture

#22 still is fine on 6.34

ibnkhaldun’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

I'd changed the status to be reported. I think It is good enough!

jonathan1055’s picture

Status: Patch (to be ported) » Reviewed & tested by the community

Hi, I think you mis-understand 'Patch to be ported' means it has been committed to one branch of code and needs to be ported to another branch too. This change has not been committed yet, so it needs to stay as RTBC

ibnkhaldun’s picture

Oh! sorry i'll return to previous stage!

mw4ll4c3’s picture

Thank you all for keeping up on this!

Since you guys are working in similar environments, you should know I have a few similar fixes for some common mods that you may need (they could probably stand some expansion, a reroll, or just an RTBC)

Status: Reviewed & tested by the community » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.