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 ''.

CommentFileSizeAuthor
#16 block_subject.patch773 bytesPasqualle
#10 block_subject_default_d.patch869 bytestheborg
#6 block_subject_default_c.patch3.59 KBtheborg
#3 block_subject_default_b.patch768 bytestheborg
#1 block_subject_default_b.patch770 bytestheborg
block_subject_default_a.patch1.17 KBtheborg
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

theborg’s picture

theborg CreditAttribution: theborg commented
Title: Block module is not providing a default subject on inserts » Block module is not providing a default title on inserts
FileSize
block_subject_default_b.patch770 bytes

Another way would be committing 'none' if the user is not providing the title.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Needs work

Why not save the empty string? <none> is not translation compatible in itself. Also you have spacing issues in your patch.

theborg’s picture

theborg CreditAttribution: theborg commented
Status: Needs work » Needs review
FileSize
block_subject_default_b.patch768 bytes

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>&lt;none&gt;</em> to display no title, or leave blank to use the default block title.'),
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

theborg: Is actually <none> gets saved into the db, when you enter it on the web interface?

theborg’s picture

theborg CreditAttribution: theborg commented

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?

theborg’s picture

theborg CreditAttribution: theborg commented
Priority: Normal » Critical
FileSize
block_subject_default_c.patch3.59 KB

- 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.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Priority: Critical » Normal
Status: Needs review » Needs work

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?

theborg’s picture

theborg CreditAttribution: theborg commented

@Gábor:
So, are you ok with #3?

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

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.

theborg’s picture

theborg CreditAttribution: theborg commented
Status: Needs work » Needs review
FileSize
block_subject_default_d.patch869 bytes

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.

Pasqualle’s picture

Pasqualle
Location 🇭🇺 Budapest
CreditAttribution: Pasqualle commented

why is the check if ($block->module == 'block') necessary?

Pasqualle’s picture

Pasqualle
Location 🇭🇺 Budapest
CreditAttribution: Pasqualle commented

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

theborg’s picture

theborg CreditAttribution: theborg commented

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.

Pasqualle’s picture

Pasqualle
Location 🇭🇺 Budapest
CreditAttribution: Pasqualle commented

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..

Pasqualle’s picture

Pasqualle
Location 🇭🇺 Budapest
CreditAttribution: Pasqualle commented

repro steps:
1. remove <?php if (!empty($block->subject)): ?> - check from garland theme block.tpl.php
2. 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.

Pasqualle’s picture

Pasqualle
Location 🇭🇺 Budapest
CreditAttribution: Pasqualle commented
FileSize
block_subject.patch773 bytes

and the patch

mpare’s picture

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

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

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Fixed

Thanks, committed. This also fixes a pgsql error, as a NOT NULL value is required for any NOT NULL field.

Anonymous’s picture

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

Automatically closed -- issue fixed for two weeks with no activity.