Moving a forum with "Leave shadow copy" checked should leave a copy in the original forum

Not sure it's major

Might want to coordinate any tests here with #1578998: move forum topic and leave shadow cause duplicated topics show in list, as that one was noted as needing tests also.

In forum_node_presave(), $node->shadow doesn't exist. I think the checkbox (which is added in form_alter) needs to be handled differently.

In D7, the form_alter passed properties to the $node object in hook_node_presave(). D8 has hook_ENTITY_TYPE_presave() which passes in an entity interface. Is there a way to pass a value from a form to hook_ENTITY_TYPE_presave() in D8? or another way to try to fix it?

Looks good to me. Thank You larowlan for showing me the new way to pass the form values! How did you know about that! I will link to the change request for other readers: https://www.drupal.org/node/2420295. I think the shadow copy should show the title since it showed in D7; So I made that change and added a test since it was broken a long time ago.

Do we want the #entity_builder callback as it is now, as opposed to a #submit callback?

redo test-only patch which did not apply. It needed to be applied on top of #7.

Back to needs review since it was the test-only patch that failed.

Looks good to me, added beta eval

+++ b/core/modules/forum/forum.module
@@ -158,6 +159,7 @@ function forum_node_presave(EntityInterface $node) {
+          $node->setNewRevision(TRUE);

Do we explicitly need to set this? Do the tests actually fail without it? Can't see why so needs a comment if that's the case.

Added comment explaining reason this:

We need a new revision because {forum} table has a primary key of vid. You can *never* have two terms for the same revision, so not sure how 'shadow' ever worked. It was before my time maintaining forum. Forcing the new revision is simplest fix, avoids schema changes.

In all honesty my preference would be to just drop the whole 'shadow' feature.

Digging back through old logs. forum_schema had a PK on vid even when it was in forum.schema (i.e. before hook_schema got moved to .install files). That was in 2007.

So I figured, OK, perhaps forum always created new revisions and this got missed in the hook_node_info » node.type.forum.yml conversion. But nope, it didn't have revisions on before then either.

So maybe this never worked. Who knows.

If it never worked and noone noticed, maybe it's a good opportunity to remove it for realz? :)

There are a few bugs, probably some are duplicates/resolved once this is in.

https://www.drupal.org/project/issues/drupal?text=shadow&component=forum...

It worked in 2005/6 but it's possible it hasn't worked since. I do vaguely remember us breaking then fixing it once before more recently than that though.

I don't think anyone will miss this, assiging to webchick for a product decision.

I haven't seen this feature in active use since like 2008, so definitely would not cry at its loss.

I would have two questions though which is:

1) what's the impact on sites (such as Drupal.org) that do have shadowed forum topics? Do the topics start showing up in both places?
2) Is contrib able to add this functionality back for sites that need it?

reroll.

For those saying this never worked, I know it was working back when I was actively working on AF a few years ago and I used it on the forum I ran. It's nice when you move a post for the person who posted to be able to see where it went to.

Re-reviewed this, happy with the approach in lieu of dropping functionality, which is definitely out now.

Can't RTBC because I worked on the patch, maybe @Michelle can?

Patch needs short array code syntax

1. +++ b/core/modules/forum/forum.module
   @@ -445,7 +459,7 @@ function template_preprocess_forums(&$variables) {
   -          $variables['topics'][$id]->title = $topic->getTitle();
   +          $variables['topics'][$id]->title_link = $topic->getTitle();
   

   this change looks strange or out of scope

2. +++ b/core/modules/forum/src/ForumIndexStorage.php
   @@ -85,10 +85,23 @@ public function deleteRevision(NodeInterface $node) {
   +        ->fields(array(
   
   +++ b/core/modules/forum/src/Tests/ForumTest.php
   @@ -631,11 +631,16 @@ private function verifyForums(EntityInterface $node, $admin, $response = 200) {
   +      $forum_tids = db_query("SELECT tid FROM {forum} WHERE nid = :nid", array(
   

   needs reroll for short array

Was unable to apply patch on local on 8.4.x, So i have rerolled the patch #27

.

@pk188 Do you have anything specific related to #34. Because i don't know why you have deleted the patch file, as there was no such comment or something.
Let me know the improvements, If any.

@Vidushi Mehta I had made the patch and uploaded after few seconds of you, so by mistake your patch deleted by me. According to me there are no improvements.

#33.1 still a question

@andypost can u please describe what is left or incorrect.

Re-rolled #34

so only needs to fix a test

1) Drupal\Tests\forum\Functional\ForumTest::testForum
The forum topic is linked to a different forum and the shadow remains
Failed asserting that an array is empty.

I've added a new patch for Drupal 9.2.x.

I assume the test will still need to be fixed so leaving the issue status as is.

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.

I took the liberty of creating an MR against Drupal 11 so folks who need a patch can get one (copy the contents of https://git.drupalcode.org/project/drupal/-/merge_requests/6674.diff to your local project and apply the patch). There was a conflict with #57 and some recent changes to the module file.