Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
In forum.module the lines
$variables['topics'][$id]->new_text = format_plural($topic->new_replies, '1 new post<span class="element-invisible"> in topic %title</span>', '@count new posts<span class="element-invisible"> in topic %title</span>', array('%title' => $original_title));
contains an undefined variable.
Proposed resolution
Use this definition for the variable
$variables['topics'][$id]->title
Remaining tasks
none.
- (done) patch in #4 comes back green from the test bot
- (done) steps to reproduce #8
- (done) manual testing #9 and #11
User interface changes
None.
API changes
None.
Original report by h4ck3rm1k3
The line here :
$variables['topics'][$id]->new_text = format_plural($topic->new_replies, '1 n\
ew post in topic %title', '@count new posts in topi\
c %title', array('%title' => $original_title));
Maybe it could be taken from this?, I would have to test it :
$variables['topics'][$id]->title;
so suggestion :
$original_title = $variables['topics'][$id]->title;
Comment | File | Size | Author |
---|---|---|---|
#23 | original_title_undefined-1796292-23-just-tests.patch | 2.02 KB | YesCT |
#23 | original_title_undefined-1796292-23.patch | 3.14 KB | YesCT |
#23 | interdiff-22-23.txt | 650 bytes | YesCT |
#22 | original_title_undefined-1796292-22-justs-tests.patch | 2.01 KB | shnark |
#22 | original_title_undefined-1796292-22.patch | 3.12 KB | shnark |
Comments
Comment #1
h4ck3rm1k3 CreditAttribution: h4ck3rm1k3 commentedhttps://github.com/h4ck3rm1k3/drupal/commit/1e2119bf7c66713a1aab8cbd8ea1...
Comment #2
larowlanTagging
Comment #3
superspring CreditAttribution: superspring commentedSubstituting the undefined variable with the actual title defined above.
Comment #4
superspring CreditAttribution: superspring commentedThis patch is now split into the coding style improvements - #1840034: Improving readability of template_preprocess_forum_topic_list function. and the fix itself.
Comment #5
YesCT CreditAttribution: YesCT commentedpatch code looks good.
next step:
a manual test that the error is there without the patch
and that the patch fixes the error
maybe provide steps to reproduce to make it super easy for anyone to jump into the issue to do the manual testing.
Comment #6
superspring CreditAttribution: superspring commentedSee #1060700: Text in new topic and new post links does not describe its purpose for where the bug was originally introduced.
Comment #7
mgiffordSorry about that @superspring - not sure how that slipped by us. Thanks for alerting the thread.
Comment #8
superspring CreditAttribution: superspring commented@YesCT, to reproduce this I:
- Enabled the forum module
- Create a forum to post in
- Created a second user to post in the forum and posted a new reply to a topic.
- As the first user I viewed the topic which shows the error
"Notice: Undefined variable: original_title in template_preprocess_forum_topic_list() (line 1234 of core/modules/forum/forum.module)."
With the patch this error no longer shows.
Comment #9
YesCT CreditAttribution: YesCT commentedI checked this manually using the instructions from #8
The error does go away with the patch.
before
after
question
I expected to see 1 new post in topic %title
but I just see "1 new post". Why is that? Do I need to check by looking at a different page?
I'm going to investigate.
Comment #10
YesCT CreditAttribution: YesCT commentedFor comparison, here is other similar code from that file:
Comment #11
YesCT CreditAttribution: YesCT commentedOK, I found it in the invisible element and can see it is getting the title of the topic.
Comment #11.0
YesCT CreditAttribution: YesCT commentedUsing Issue Summary Template.
Comment #12
webchickLet's get some test coverage for this bug. Sounds like it's pretty elaborate to reproduce, but maybe a simpler subset could be formulated as a test.
Comment #13
superspring CreditAttribution: superspring commentedSame patch as above but with a test (which fails without the code change).
Comment #14
superspring CreditAttribution: superspring commentedComment #15
YesCT CreditAttribution: YesCT commentedIs this test checking for the correct " in forum %title" in the span?
Or is it checking that there is no error on the page (no Use of undeclared variable)?
Also, please upload each: tests only, whole patch, and interdiff, for example:
original_title_undefined-1796292-13-justtests-shouldfail.patch
original_title_undefined-1796292-13.patch
interdiff-4-13.txt
The first shows the test bot will fail
The second should trigger the status of the issue to be needs review
And the interdiff helps people review.
Comment #16
superspring CreditAttribution: superspring commentedAdding the two new files to show the diffs.
Comment #17
YesCT CreditAttribution: YesCT commentedThe test this adds: checking that page making the comment and the page when other user looking at the comment happen, assertResponse 200, and that there are no php errors doing either of those two things.
I reviewed the code and it looks fine.
@webchick, is that the test you had in mind? It does not check that the title in the invisible span is exactly the same title of the post. That might be overly specific.
I checked. This seems to be the common right way.
I also checking the t() and looking in #1803844: Remove t() from test assertion messages that t is fine.
the interdiff was the original patch, and not the diff between patch 13 and 4. (the interdiff is that test were added to 13). Adding the interdiff just for completeness. Also adding a history in case someone wants to see how I made the interdiff.
Comment #18
YesCT CreditAttribution: YesCT commentedAck! oops. that interdiff should have been called .txt not .patch! so it would not have been tested. sheesh. sorry! *wishes for that withdraw test request to bot button*
The patch in #13 is the one to commit and it comes back from the test bot passing. At least we can say that and smile. :)
Comment #19
YesCT CreditAttribution: YesCT commentedAnd the tests only in #16 fails for the test bot. Looking good here.
Comment #20
xjmThanks @YesCT and @superspring.
Let's get everything rolled together in one patch and attach it here at the end of the issue with a test-only patch followed by a combined patch; the PIFT bug combined with the unclear patch naming is really obfuscating things here.
Let's change the name of this method and its documentation to indicate what we're testing for, rather than what we're testing for the absence of.
Comment #21
andypostLet's merge patch and test! This is a really annoying
Comment #22
shnark CreditAttribution: shnark commentedI made a just tests patch and a patch with the fix and the test, based on comment #13
next is the change that xjm requested in comment #20
Comment #23
YesCT CreditAttribution: YesCT commentedthe comment is reworded to say we are checking a forum displayed with a new post. and a new name for the function.
Comment #24
larowlanThis looks good to go and the new tests correctly catch the bug.
Great work all!
Comment #25
catchCommitted/pushed to 8.x, thanks!
Comment #26.0
(not verified) CreditAttribution: commentedUpdated issue summary remaining tasks to show what is done to make it easier for someone coming into this issue.