PHP notice when moving a forum topic and leaving behind a shadow copy

#1: forum.patch queued for re-testing.

Tested and confirm it worked on Drupal 7.8. How can I help get this into the next D7 release? I've never tried installing D8 yet, but if that would help let me know.

Tested in Drupal 8.x-dev and works perfectly. As mentioned, it also works perfect in Drupal 7.8.

How could this slip through testing? Does this mean the forum moving is not tested?

want to see testbot comment :)

will add a test later.

not sure if this helps, I was testing the patch at http://drupal.org/node/1578998#comment-6110650 and then got the above error.
I applied cleanly the patch in #9 above and the error was gone.

droplet, thanks for working on this.

Has this been applied to version 7.x? I see comment #6 says it works, but I also see "needs backport" and am getting the error in 7.16.

Still doesn't seem to have been committed?

in 7.32 error is still there:

Notice: Undefined property: stdClass::$new_replies in template_preprocess_forum_topic_list() (line 1205 of /modules/forum/forum.module).

The real problem is that the shadow copy sits in the same forum as the moved topic where the old forum shows the same: The moved topic and the shadow copy.

Same here, getting the error in 7.32. But applying manually the last submitted patch commented in #9 it works fine!

Thanks.

Hei,
I have generated the patch for the current branch 7.x

+++ b/modules/forum/forum.module
@@ -980,18 +980,12 @@ function forum_get_topics($tid, $sortby, $forum_per_page) {
+  // folder is new if topic is new or there are new comments since last visit

Could you start the comment with capitals and end with a period, please? I know that was wrong in original patch but we should try to follow coding standards.

Remember to change issue status to "Needs review" in order to let testbot start to test it.

Thanks for the re-roll :)

Thanks Ruben :)
I have modified the patch and changed the status to "Needs review".

+++ b/modules/forum/forum.module
@@ -980,18 +980,12 @@ function forum_get_topics($tid, $sortby, $forum_per_page) {
+  // Folder is new if topic is new or there are new comments since last visit.

Good! Now it looks like you should fix indentation of the comment to same level than 'if' statement.

Thanks for your tips rteijeiro
Indentation added to the comment.

It looks like the path where you created the patch is not correct. Try to create the patch from drupal root.

If it's a D7 patch then it probably won't apply to D8, that's why it makes sense to name it "do-not-test", except there's a "t" missing.

Totally diff story in D8 now #2390065: Moving a forum with "Leave shadow copy" checked should leave a copy in the original forum.

There may be a more serious bug in Drupal 8 also (that is masking this one), but the code that leads to the PHP notice looks the same in Drupal 8 (ForumManager::getTopics()).... It needs the same change to make sure $topic->new_replies always gets set, doesn't it?

+++ b/core/modules/forum/forum.module
@@ -954,16 +954,11 @@ function forum_get_topics($tid, $sortby, $forum_per_page) {
+    if ($user->uid || $topic->forum_tid == $tid) {

Won't the forum_tid always be the same as the $tid since it is filtered in the query?

The original problem doesn't seem to exist anymore in D8. Rerolled, and removed the $tid check.

This issue is not held up because #9 did not work but because we don't have tests.

I don't think any of the patches after #9 are correct.

#34 is a reroll of 9, plus the change described in #34. We ARE adding a test here: #2390065 Does that test cover this issue also?

Verified that the patch for D7 in comment 36 worked for me using Drupal 7.38. I am able to move a thread, leaving a shadow. Viewing the moved thread, and/or clicking on "This topic has been moved" both result in viewing the thread without the error.

Re-uploading patch for the bots.

Still needs work as per #36

I think we can use the test from https://www.drupal.org/node/2390065?

Keeping bugs in in production, holding up a thoroughly tested D7 patch for five years while waiting for D8 tests. This is just wrong.

As what I can remembered #31, it's the different story in D8. the tests can be alone or merged soon. let's review the current issue or add a test and then move forward.

Join the party and tell something we never noticed before, we can do it better: #2706483: [policy, no patch] Policy to help less interested Patch move forward.

It was 7 months ago, so I don't remember, but It looks like I postponed the issue on #2390065 because I think it has the TEST that is needed for this issue to move forward.

@droplet But this was about the D7 patch which was thoroughly tested, so this is more about the backport and/or test policy and not lack of interest. Keeping bugs in the codebase because of test coverage.

Someone could just copy the test from https://www.drupal.org/node/2390065 and add it to the patch for this issue and we would be done.

Not writing a test is the same as lack of interest.

I have just commented out the 'Leave shadow copy' function in the core module. The issue I had was exactly same as #16.

I am using the 7.59 core module. It seems like the latest drupal 7 core module is still having this issue?

I understand it is not right thing to do to change the core. Can we disable or let user to have the option to disable the 'Leave shadow copy' check box in the next release?

Or we could use the 7 year old, tested patch...

Unless you believe that commiting code without tests would do more harm to the code base than leaving bugs unpatched for 7 years...

Forum is approved for removal. See #1898812: [policy] Deprecate forum module for removal in Drupal 11

This is now Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

It will be moved to the contributed extension once the Drupal 11 branch is open.