Block module is not providing a default title on inserts
theborg - December 11, 2007 - 10:57
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | block.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | theborg |
| Status: | closed |
Description
Block module is not providing a default subject on inserts.
When adding a block and not giving it a subject the module commits a null to the database. Then if this block is enabled and the theme doesn't take care of checking subject it yields an error:
notice: Undefined property: stdClass::$subject
This patch defaults subject to ''.
| Attachment | Size |
|---|---|
| block_subject_default_a.patch | 1.17 KB |
#1
Another way would be committing 'none' if the user is not providing the title.
#2
Why not save the empty string?
<none>is not translation compatible in itself. Also you have spacing issues in your patch.#3
Correction of the spacing issues.
I am defaulting to
<none>because is the standard way of creating a block without a title.'#description' => $module == 'block' ? t('The title of the block as shown to the user.') : t('Override the default title for the block. Use <em><none></em> to display no title, or leave blank to use the default block title.'),#4
theborg: Is actually
<none>gets saved into the db, when you enter it on the web interface?#5
Yes, it is a way to say that the block doesn have a title, as you can see in blocks table.
@gabor: Do you consider it better to have an empty value?
#6
- Added an explanation to make aware the user of the title possibilities when creating a block:
a) Title is blank => uses description as a title
b)
<none>=> no title is displayed- Added translation of
none.- Added a constant BLOCK_TITLE_NONE that defaults to -1 to block module.
Marking this critical because it produces an error if the theme template is not taking care of block->subject and therefore the user can not build css-only themes.
#7
A little theme error for block titles does not make this critical.
- We don't do br tags in form item descriptions.
- Instead of concatenating t()-ed text, which breaks the flow, and makes it impossible to reorder things in languages which requires the contents to be reordered, use placeholders, such as @none if you think this needs a placeholder.
-
<none>was not translatable before, why make it translatable now?- storing -1 in a string field to denote 'no title' is not exactly elegant, why is the previous way buggy?
#8
@Gábor:
So, are you ok with #3?
#9
Hm, my problem was that I did not understand why we need to fiddle with this in the validation. How does a NULL end up in the title value? On first thought it is because it is unset. But then it would give you a notice on submission. So when I looked into block_admin_configure(), I realized that there is actually an SQL query filling in the default values, which does not produce values if there was no data for the block at hand. So why not fix this at the root of the problem and only fill in the data from the database, if it was actually found there? db_fetch_array() seems to return FALSE if there was no result, right? So why not check for that and fill in an array with sensible defaults in this case.
I wonder why you did not receive a notice here for nonexistent blocks anyway. Hm.
#10
Revised this issue and got some conclusions:
1) NULL is not commited, that was _my fault_, sorry, I didn't notice that the table doesn't allow nulls on this values and that you always get an empty string from the form api.
2) The blocks creation form allows the user to enter no title, so its correct to think that the block should display no title.
3) The blocks configuration form allow the user to override the title of system blocks and this is when
<none>gets commited.So, this patch only takes care of setting $block->subject to avoid notices letting the user decide to put a title or none at all.
#11
why is the check
if ($block->module == 'block')necessary?#12
I would use something like this:
If (!$block->title) {$block->title = '<none>'}$block->subject = $block->title == '<none>' ? '' : check_plain($block->title);
it would make these variables consistent
#13
Because the default title for system blocks is an empty string, so it can be dynamically calculated (like user name for the navigation block). To override this you must enter
<none>. In the case of custom blocks an empty title means no title.#14
ok, got it, (the user module sets the $block->subject variable, so it is wrong to change it)
but still does not seems good
then it should be something like isset($block->subject) check instead of module check
will try to debug..
#15
repro steps:
1. remove
<?php if (!empty($block->subject)): ?>- check from garland theme block.tpl.php2. admin/build/block/add - create block with description and body, but without title
3. admin/build/block - set region for the new block
notice: Undefined property: stdClass::$subject in ...\themes\garland\block.tpl.php on line 6.
#16
and the patch
#17
Patch applies cleanly and appears to fix issue. I've created blocks with and without title, moved around, switched themes, switched themes back, deleted and created blocks, and edited existing blocks. Patch appears to fix stated issues without ill effect. Tested against fresh update of cvs head.
-mpare
#18
Thanks, committed. This also fixes a pgsql error, as a NOT NULL value is required for any NOT NULL field.
#19
Automatically closed -- issue fixed for two weeks with no activity.