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;

Files: 
CommentFileSizeAuthor
#23 original_title_undefined-1796292-23-just-tests.patch2.02 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 49,339 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#23 original_title_undefined-1796292-23.patch3.14 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 49,365 pass(es).
[ View ]
#23 interdiff-22-23.txt650 bytesYesCT
#22 original_title_undefined-1796292-22-justs-tests.patch2.01 KBEllaTheHarpy
FAILED: [[SimpleTest]]: [MySQL] 49,348 pass(es), 2 fail(s), and 4 exception(s).
[ View ]
#22 original_title_undefined-1796292-22.patch3.12 KBEllaTheHarpy
PASSED: [[SimpleTest]]: [MySQL] 49,342 pass(es).
[ View ]
#17 interdiff-4-13.patch2.01 KBYesCT
Test request sent.
[ View ]
#17 history.txt860 bytesYesCT
#16 original_title_undefined-just-tests-1796292-16.patch2.01 KBsuperspring
Test request sent.
[ View ]
#16 interdiff-4-13.txt1.11 KBsuperspring
#13 original_title_undefined-1796292-13.patch3.12 KBsuperspring
Test request sent.
[ View ]
#11 undef-s03-2012-11-15_2134.png157.8 KBYesCT
#9 undef-s01-2012-11-15_2016.png88.58 KBYesCT
#9 undef-s02-noerror-2012-11-15_2020.png80.74 KBYesCT
#4 original_title_undefined-1796292-4.patch1.11 KBsuperspring
PASSED: [[SimpleTest]]: [MySQL] 48,077 pass(es).
[ View ]
#3 original_title_undefined-1796292-3.patch1.4 KBsuperspring
PASSED: [[SimpleTest]]: [MySQL] 48,074 pass(es).
[ View ]

Comments

h4ck3rm1k3’s picture

larowlan’s picture

Issue tags:+#pnx-sprint

Tagging

superspring’s picture

Status:Active» Needs review
StatusFileSize
new1.4 KB
PASSED: [[SimpleTest]]: [MySQL] 48,074 pass(es).
[ View ]

Substituting the undefined variable with the actual title defined above.

superspring’s picture

StatusFileSize
new1.11 KB
PASSED: [[SimpleTest]]: [MySQL] 48,077 pass(es).
[ View ]

This patch is now split into the coding style improvements - #1840034: Improving readability of template_preprocess_forum_topic_list function. and the fix itself.

YesCT’s picture

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

superspring’s picture

mgifford’s picture

Sorry about that @superspring - not sure how that slipped by us. Thanks for alerting the thread.

superspring’s picture

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

YesCT’s picture

I checked this manually using the instructions from #8

The error does go away with the patch.

before

undef-s01-2012-11-15_2016.png

after

undef-s02-noerror-2012-11-15_2020.png

question

+++ b/core/modules/forum/forum.moduleundefined
@@ -1231,7 +1231,7 @@ function template_preprocess_forum_topic_list(&$variables) {
+        $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' => $variables['topics'][$id]->title));

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.

YesCT’s picture

For comparison, here is other similar code from that file:

      if ($variables['forums'][$id]->new_topics) {
        $variables['forums'][$id]->new_text = format_plural($variables['forums'][$id]->new_topics, '1 new post<span class="element-invisible"> in forum %title</span>', '@count new posts<span class="element-invisible"> in forum %title</span>', array('%title' => $variables['forums'][$id]->name));
YesCT’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new157.8 KB

OK, I found it in the invisible element and can see it is getting the title of the topic.
undef-s03-2012-11-15_2134.png

YesCT’s picture

Issue summary:View changes

Using Issue Summary Template.

webchick’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs tests

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

superspring’s picture

StatusFileSize
new3.12 KB
Test request sent.
[ View ]

Same patch as above but with a test (which fails without the code change).

superspring’s picture

Status:Needs work» Needs review
YesCT’s picture

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

superspring’s picture

StatusFileSize
new1.11 KB
new2.01 KB
Test request sent.
[ View ]

Adding the two new files to show the diffs.

YesCT’s picture

Issue tags:-Needs tests
StatusFileSize
new860 bytes
new2.01 KB
Test request sent.
[ View ]

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

+++ b/core/modules/forum/lib/Drupal/forum/Tests/ForumTest.phpundefined
@@ -443,6 +451,35 @@ private function doBasicTests($user, $admin) {
+    $edit['comment_body[' . LANGUAGE_NOT_SPECIFIED . '][0][value]'] = $this->randomName();

I checked. This seems to be the common right way.

+++ b/core/modules/forum/lib/Drupal/forum/Tests/ForumTest.phpundefined
@@ -443,6 +451,35 @@ private function doBasicTests($user, $admin) {
+    $this->drupalPost("node/$node->nid", $edit, t('Save'));

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.

YesCT’s picture

Status:Needs review» Reviewed & tested by the community

Ack! 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. :)

YesCT’s picture

And the tests only in #16 fails for the test bot. Looking good here.

xjm’s picture

Status:Reviewed & tested by the community» Needs review

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

+++ b/core/modules/forum/lib/Drupal/forum/Tests/ForumTest.phpundefined
@@ -443,6 +451,35 @@ private function doBasicTests($user, $admin) {
+   * Tests for notices on a rendered page.
+   */
+  function testUndefinedVars() {

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.

andypost’s picture

Status:Needs review» Needs work

Let's merge patch and test! This is a really annoying

EllaTheHarpy’s picture

Status:Needs work» Needs review
StatusFileSize
new3.12 KB
PASSED: [[SimpleTest]]: [MySQL] 49,342 pass(es).
[ View ]
new2.01 KB
FAILED: [[SimpleTest]]: [MySQL] 49,348 pass(es), 2 fail(s), and 4 exception(s).
[ View ]

I 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

YesCT’s picture

StatusFileSize
new650 bytes
new3.14 KB
PASSED: [[SimpleTest]]: [MySQL] 49,365 pass(es).
[ View ]
new2.02 KB
FAILED: [[SimpleTest]]: [MySQL] 49,339 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

the comment is reworded to say we are checking a forum displayed with a new post. and a new name for the function.

larowlan’s picture

Status:Needs review» Reviewed & tested by the community

This looks good to go and the new tests correctly catch the bug.
Great work all!

catch’s picture

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

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

Anonymous’s picture

Issue summary:View changes

Updated issue summary remaining tasks to show what is done to make it easier for someone coming into this issue.