Use of undeclared variable : $original_title in core/modules/forum/forum.module on line 1232

Tagging

Substituting the undefined variable with the actual title defined above.

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

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.

See #1060700: Text in new topic and new post links does not describe its purpose for where the bug was originally introduced.

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

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

I checked this manually using the instructions from #8

The error does go away with the patch.

before

after

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.

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));

OK, I found it in the invisible element and can see it is getting the title of the topic.

Using Issue Summary Template.

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.

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

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.

Adding the two new files to show the diffs.

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.

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

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

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.

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

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

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

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

Committed/pushed to 8.x, thanks!