[drupal-devel] [bug] Blocks with no title kill Drupal

Fri Apr 15 17:18:51 UTC 2005

Issue status update for http://drupal.org/node/19965

 Project:      Drupal
 Version:      cvs
 Component:    block.module
 Category:     bug reports
 Priority:     critical
 Assigned to:  Robin Monks
 Reported by:  budda
 Updated by:   Steven
 Status:       patch

That patch was already committed. So the code I added back then is now
redundant as $block['info'] must always be set.

The MySQL problem just seems to be that you're keying on the title or
description. Why not have a block id? We do this everywhere else...

Steven

Previous comments:
------------------------------------------------------------------------

April 4, 2005 - 19:03 : budda

When setting more than one block's title to '' (blank) Drupal dies, and
doesn't catch the SQL query error nicely. The error is as follows:
user error: Duplicate entry '' for key 2
query: UPDATE boxes SET title = '', body = ' ...<cut>... in
/var/www/html/drupal-cvs/includes/database.mysql.inc on line 66.

Using 4.6 build from 4/4/2005

The page ends with:
warning: Cannot modify header information - headers already sent by
(output started at /var/www/html/drupal-cvs/includes/common.inc:384) in
/var/www/html/drupal-cvs/includes/common.inc on line 192.

------------------------------------------------------------------------

April 8, 2005 - 10:08 : robertDouglass

There should be a check before the insert is attempted and if the block
title already exists the user should be notified with form_error().

------------------------------------------------------------------------

April 8, 2005 - 11:46 : Poolio

this certainly doesn't solve the problem, but I've worked around this
issue by using html tags in the title field for blocks that I don't
want to show a title.

------------------------------------------------------------------------

April 9, 2005 - 02:31 : Robin Monks

Attachment: http://drupal.org/files/issues/block.module.blank.title.patch (955 bytes)

A small patch that checks to ensure the title does not already exit in
the Blocks list.  Also, if the field was left blank, in adds an HTML
comment with the current date to the second to ensure that a duplicate
title isn't created.

This is my first patch, so I probably did something wrong, but I'm
willing to fix it too.

Robin Monks
MozNetwork.org

------------------------------------------------------------------------

April 9, 2005 - 03:22 : Morbus Iff

I'm, personally, fine with how you've fixed the blank block title.
You've really solved two problems in one: duplicate titles (which is
the cause of this Issue), and the desire to have blank (or no) titles
(which is the reason for this Issue) for the end user. With that said,
I'm not a fan of your error message (but, then again, anal wording is
my specialty, so don't think I'm picking on you):

I'd rename it to "That block title is already in use; please choose
another."
The goals here:

The user may not remember all the previous titles he's used.
Using "unique" starts forcing him to remember. We shouldn't do that.
The user already knows he entered it - we don't have to repeat that to
him.
It's active, not passive (no use of "have").
Use a semi-colon, not a comma.
It's much shorter and concise.

------------------------------------------------------------------------

April 9, 2005 - 08:27 : chx

Attachment: http://drupal.org/files/issues/box_title.patch (1.49 KB)

Why display any message at all?

------------------------------------------------------------------------

April 9, 2005 - 16:11 : Morbus Iff

Displaying no message at all would only work if the user wasn't typing
in ANY title whatsoever (since then we'd make one for them based on the
date). But, if a user names a block "Users" one week, and then creates
another block called "Users" three weeks later, than an whoopsidaisy
error needs to be shown - we shouldn't modify the text randomly.

------------------------------------------------------------------------

April 9, 2005 - 17:10 : chx

Well, let's discuss the main point: why shoiuld be the title unique?

------------------------------------------------------------------------

April 9, 2005 - 17:38 : Morbus Iff

Are you asking why Drupal enforces them being unique, or why, visually
and to the end-user, they should be unique?

------------------------------------------------------------------------

April 9, 2005 - 17:43 : chx

I guess Drupal forces them 'cos for some reasons it is thought that for
the users they should be unique.

This is not the case, however. Yes, it's confusing if they are on the
same page. But I can think on circumstances where they would not be on
the same page, so the force from Drupal seems unnecessary.

------------------------------------------------------------------------

April 9, 2005 - 21:00 : robertDouglass

agree with chx on the unique point. Like the functionality of adding
html comments and timestamp (why timestamp? why not a block id?) if
left blank if they must be uniqe. Would be in favor of making the
visible title completely optional. Administrators have description to
keep them separate on the admin screen.

------------------------------------------------------------------------

April 9, 2005 - 21:48 : Robin Monks

Attachment: http://drupal.org/files/issues/drupal_0.patch (3.67 KB)

This new patch no longer requires a unique value for title, and also
checks to ensure the Block Description is not a duplicate or empty
value.

Big thanks go out to chx for assistance.

Robin

------------------------------------------------------------------------

April 11, 2005 - 16:20 : Anonymous

Shouldn't we add a test too to check info unicity ?

------------------------------------------------------------------------

April 11, 2005 - 16:38 : chx

<?php
if (empty($edit['info']) || db_num_rows(db_query("SELECT info FROM
{boxes} WHERE info='%s'", $edit['info']))) {

?>

To me, this seems like a check for info unicity... not to mention that
the UNIQUE(info) DB constraint is not touched.

------------------------------------------------------------------------

April 11, 2005 - 16:46 : tostinni

Ups, sorry for the anonymous and the message, I didn't saw this
verification, I was only remembering block.module code.

That's all fine for me.

------------------------------------------------------------------------

April 11, 2005 - 18:27 : njivy

+1 to Robin Monks' patch.  When I install Drupal, the first thing I
always do is remove the /unique key/ constraint on block titles.

I have not tested the other parts of Monks' patch, though, including
the uniqueness constraint on block descriptions.

------------------------------------------------------------------------

April 11, 2005 - 21:04 : drumm

Code looks okay. +1

"You must enter unique data into the block description field." doesn't
sound like the friendliest of error messages.

------------------------------------------------------------------------

April 11, 2005 - 21:16 : Dries

I'm OK with this patch but suggest that we look into improving the error
message.

Small nit: fix the spacing around '=' in the SQL query that checks for
duplicates.

------------------------------------------------------------------------

April 12, 2005 - 01:40 : Robin Monks

Attachment: http://drupal.org/files/issues/drupal_1.patch (3.97 KB)

Here is a new patch with the changes and with the updates.inc file
changed to reflect patchs applied after the original patch was made.

Robin

------------------------------------------------------------------------

April 12, 2005 - 20:52 : Dries

Committed to HEAD.  Thanks. The patch did not apply against DRUPAL-4-6. 
Possibly needs to get backported.

------------------------------------------------------------------------

April 13, 2005 - 17:36 : Robin Monks

Attachment: http://drupal.org/files/issues/4.6 blank title (3.63 KB)

Here is the patch backported for 4.6.

Robin
MozNetwork.org

------------------------------------------------------------------------

April 13, 2005 - 17:45 : Robin Monks

Attachment: http://drupal.org/files/issues/4_0.6 blank title (3.62 KB)

That patch had a bug in it, new patch attached.

Robin

------------------------------------------------------------------------

April 14, 2005 - 13:38 : killes at www.drop.org

Attachment: http://drupal.org/files/issues/block-fix.patch (673 bytes)

The patch got apparently into core. It introduces a problem with php 5.
$edit is not defined as an array if the form is called for the fiest
time (default in switch).

This patch fixes this.

------------------------------------------------------------------------

April 15, 2005 - 17:28 : Steven

Sorry for dropping in so late, but I disagree that block descriptions
should be filled in.

A while ago I even made a patch to display the block title in the admin
list if the description was empty: I never fill in descriptions for my
own blocks, I know what they are.

It seems a big usability hit to force people to enter descriptions.

Also, the updates seem to re-add the index on (title) but not on
(info). I think the two versions (database.sql and updates.inc) are now
out of sync.

------------------------------------------------------------------------

April 15, 2005 - 18:58 : RobinMonks at moznetwork.org

>Sorry for dropping in so late, but I disagree that block descriptions
should be filled in.

>A while ago I even made a patch to display the block title in the
admin list if the description was empty: I never >fill in descriptions
for my own blocks, I know what they are.

  Then please submit it!  But from a usibility standpoint, it's better
to show a kind error now (when 4.6 is going out the door), then to
argue about this and have this bug still in 4.6.

> It seems a big usability hit to force people to enter descriptions.

  Point taken, but ATM entering two blank descriptions makes MySQL go
mad, and that seems worse to me.  We can always remove then need of a
unique info later.

>Also, the updates seem to re-add the index on (title) but not on
(info). I think the two versions (database.sql >and updates.inc) are
now out of sync.

  The title was never remove from info if you look closely, and as a
matter 'o fact, that index is just left there from redundancy as the
module check for dupes on-the-fly.

Robin