Closed (fixed)
Project:
Drupal core
Version:
6.x-dev
Component:
theme system
Priority:
Normal
Category:
Bug report
Assigned:
Reporter:
Created:
11 Dec 2007 at 09:44 UTC
Updated:
18 Jan 2008 at 19:31 UTC
Jump to comment: Most recent file
The subjectproperty of the comment object is not set when previewing it on:
- comment module
- chameleon theme
- pushbutton theme
The error message is: notice: Undefined property: stdClass::$status
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | comment_status_bug_e.patch | 5.09 KB | theborg |
| #8 | comment_status_bug_d.patch | 4.28 KB | theborg |
| #6 | comment_status_bug_c.patch | 4.12 KB | theborg |
| #4 | comment_status_bug_b.patch | 4.1 KB | theborg |
| #3 | comment_status.patch | 2.35 KB | dvessel |
Comments
Comment #1
theborg commentedThe status property of the comment object is not set when previewing it on:
- comment module
- chameleon theme
- pushbutton theme
The error message is: notice: Undefined property: stdClass::$status
Comment #2
gábor hojtsyWhy not set the status instead, where it should be? We strive to have easy to do templating.
Comment #3
dvessel commented$comment->status set from "comment_form_add_preview()". Removed the isset() check from templates from other themes since it's no longer needed.
Comment #4
theborg commented-Added $status variable, string conversion of the $comment->status at the function:
template_preprocess_comment.This allows the template to theme 3 different status:
a) comment-published,
b) comment-unpublished
c) comment-preview
-Sets $comment->status for background theme compatibility.
-Modified core themes to include $status variable instead of $comment->status.
Comment #5
gábor hojtsy- What is a background theme compatibility?
- if() is used without a {} wrapper
- // comment is used without space after //, and // at end of line, instead of on its own line indented properly, and with a dot at the end
Comment #6
theborg commentedCode style corrected, thanks Gabor.
For background compatibility I mean that the comment->style is set to avoid errors on contributed themes that maybe are using it.
Comment #7
gábor hojtsytheborg: we don't build such backward compatibility cruft into our code. Instead, we document what changes are required in themes. Also, $status would need documentation in the default comment template file, modules/comment/comment.tpl.php.
Comment #8
theborg commentedRemoved isset() check.
Added documentation in the default comment template file.
Comment #9
theborg commentedComment #10
dvessel commentedWhy does $status have a space prepended. Your assuming it would always be positioned as a second or greater class but the naming gives no indication of it. Even if it's documented, it's just odd. Who knows, it may be used for other things.
The spacing should be created inside the .tpl since it may be used for something else..
For example, this look wrong.
Not sure what was wrong with my previous patch. It was a simple fix.
Comment #11
theborg commentedRemoved the space prepended, as dvessel noted the new status variable can be used for other purposes than the class attribute.
Added the space to core themes.
@dvessel: I've added the variable because I agree with Gábor that the less logic on the templates the better.
Comment #12
dvessel commentedI'm not seeing this bug. Must have been fixed elsewhere.
Comment #13
gábor hojtsyWell, even if any issue brought up here is fixed, the patches also introduce a new comment-preview class on the comment, so this can be recategorized as a feature for D7, even if the bug at hand was fixed in D6 already.
Comment #14
theborg commentedThe bug persits:
Create a css theme and you'll get:
notice: Undefined property: stdClass::$status inComment #15
dvessel commentedSorry about that. I wasn't testing properly.
The latest patch still works with 1 hunk offset. So marking as ready.
Comment #16
gábor hojtsyGreat, committed, thanks.
Comment #17
(not verified) commentedAutomatically closed -- issue fixed for two weeks with no activity.