Make block configuration understandable

This patch marks a significant usability improvement. Excellent job. I had a quick look at the patch:

1. Taken from the patch:

+ else if ($op == 'configure' && $delta = 3) {

There are a number of '$delta = 3's that probably should be '$delta == 3's.

2. I would use 'save' rather than 'configure save'.

3. block_box_add() is a little weird:

3.a. Is the $bid parameter being used?

3.b.

 +      drupal_set_message(block_box_save($edit));
+      drupal_goto('admin/block'); 

I would move the drupal_set_message() and the drupal_goto() inside block_box_save(), if possible without having to add extra code.

3.c.

 +      $form = block_box_form();
+      $form .= form_submit(t('Save block'));
+      $output .= form($form); 

Any reason we can't move all form stuff into block_box_form()?

3.d. It would be good if we could get rid of the block vs box thing.

4. We need proper database updates.

5. [?+ php block['pages_show'] = $old_blocks[$module][$delta]->pages_show;
+ $block['pages'] = $old_blocks[$module][$delta]->pages; ?]
What is the difference between 'pages' and 'pages_show'? It is not clear from the names so maybe we should try to come up with more explanatory names.

6. The changes will need to be documented in a new 'Converting modules from 4.5.x to CVS' page.

7. Has the block API Doxygen comments?

this is an improvement.

one nit: don't use a form_group() around all the form fields on a page. There is no need to group if everything is of the same type. If you need to name that single group of forms, use the page title

I agree, this is a very nice patch.

1) One thing I do miss is that there is no (longer?) a clear overview of blocks (which blocks are always enabled, which are contextual, ...). However, this is really the fault of the giant configuration table. Reworking that is a different patch.

2) I have a usability issue with block.module which could go in this patch: the description field is really only useful for module-defined blocks where the title is not descriptive. Custom blocks have static titles, so the description is really redundant. I know I never fill it in on my own site, only to curse myself when I see the administration screen and have a bunch of unknown blocks sitting there. So either:
- Get rid of the description field for custom blocks and use the title instead.
- Keep the description field, but use the block title if the description field was left empty.

I prefer the first.

3) Here's a reworked contextual help text for the block admin. The giant paragraph that we have now is not going to be read by anyone:

Blocks are the boxes in the left and right side bars of the web site. They are made available by modules or created manually.

Only enabled blocks are shown. You can position the blocks by deciding which side of the page they will show up on (region) and in which order they appear (weight).

If you want certain blocks to disable themselves temporarily during high server loads, check the 'Throttle' box. You will need to enable the auto-throttle on the [[throttle configuration page]] after having enabled the throttle module.

4) The general module help needs work:
"If the block\'s throttle checkbox is checked, the throttle level must me low." (me -> be)

The paragraph about "A block is visible when:" is also structured oddly. The part about the throttle is confusing and the last option does not match the preceding fragment at all (it starts a new sentence).

Maybe it's better to say:

A block's visibility depends on:
- Its enabled checkbox. Disabled blocks are never shown.
- Its throttle checkbox. Throttled blocks are hidden during high server loads.
- Its path options. Blocks can be configured to only show/hide on certain pageS.
- User settings. You can choose to let your users decide whether to show/hide certain blocks.
- Its function. Dynamic blocks (such as those defined by modules) may be empty on certain pages and will not be shown.

This is a bit vaguer, but shorter and easier to understand. Where the exact options are located is not so important, because they are in a very logical place, and people are bound to run into them when making their own blocks.

I'm OK with removing the description.

I think - to be more consistent with the rest of drupal - that if the throttle.module is not enabled, you should _not_ display the throttle fields at all. We do this all the time inside drupal, right?

I suggest you do something like:

- $header = array(t('Block'), t('Enabled'), t('Throttle'), t('Weight'), t('Region'), array('data' => t('Operations'), 'colspan' => 2));
+ $header = array(t('Block'), t('Enabled'), (module_exist('throttle') ? t('Throttle')), t('Weight'), t('Region'), array('data' => t('Operations'), 'colspan' => 2));

and:

-    $rows[] = array($block['info'], array('data' => form_checkbox(NULL, $block['module'] .']['. $block['delta'] .'][status', 1, $block['status']), 'align' => 'center'), array('data' => form_checkbox(NULL, $block['module'] .']['. $block['delta'] .'][throttle', 1, $block['throttle'], NULL, module_exist('throttle') ? NULL : array('disabled' => 'disabled')), 'align' => 'center'), form_weight(NULL, $block['module'] .']['. $block['delta'] .'][weight', $block['weight']), form_radios(NULL, $block['module'] .']['. $block['delta'] .'][region', $block['region'], array(t('left'), t('right'))), l(t('configure'), 'admin/block/configure/'. $block['module'] .'/'. $block['delta']), $operation);
+    $rows[] = array($block['info'], array('data' => form_checkbox(NULL, $block['module'] .']['. $block['delta'] .'][status', 1, $block['status']), 'align' => 'center'), (module_exist('throttle') ? array('data' => form_checkbox(NULL, $block['module'] .']['. $block['delta'] .'][throttle', 1, $block['throttle'], NULL)), 'align' => 'center'), form_weight(NULL, $block['module'] .']['. $block['delta'] .'][weight', $block['weight']), form_radios(NULL, $block['module'] .']['. $block['delta'] .'][region', $block['region'], array(t('left'), t('right'))), l(t('configure'), 'admin/block/configure/'. $block['module'] .'/'. $block['delta']), $operation);

The above patch is untested, and i'm not fully trusted with the fact that it works, but your should get an idea of what I mean..

I tried the patch: running the update.php script disabled all blocks. Can't look into it right now (lunch) but it is probably an easy fix.
user error: Unknown column 'pages_show' in 'field list' query: INSERT INTO blocks (module, delta, status, weight, region, pages_show, pages, custom, throttle) VALUES ('user', '3', 0, 5, 1, 0, '', 0, 1) in /mnt/redhat/foo/cvs/web/drupal.org/cvs/includes/database.mysql.inc on line 125.

Drumm, I know how the current overview looks and that's why I am asking you to make this fix.. The throttle-checkbox should _not_ be displayed instead of being disabled when the module is not enabled..

If a module is disabled, we don't display their help text and configuration options at all, right? So why are we doing that for the throttle module?

Stefan.

Committed. The UI could do with a tad more polish though, but that can be part of a new patch. Please make sure to update the documentation.

The upgrade simply dropped all the stored regexes. I understand that an upgrade patch was difficult to provide, but we should at least document this.

Attached patch does this.

I'll add this to the release announcement together with the other upgrade issues. Better to centralize everything imho.

Yes