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
Reproduce:
1. Apply this patch #2350309: Forum index links head to taxonomy/term/{term} instead of forum/{term} or goto forum/1
2. Add a forum topic
3. Edit a forum topic, select moving to another Forums, and Ticked "Leave shadow copy"
Result:
Forum topic moved. No shadow copy in original forum.
Expected:
Forum topic moved. and Leaving a shadow copy with link in original forum.
Proposed resolution
Remaining tasks
User interface changes
Beta phase evaluation
Issue category | Bug because functionality was lost in NodeNG conversion (with follow-up #1956848: NodeNG conversion follow-up: Only first forum tid is saved into {forum_index}) |
---|---|
Issue priority | Major because loss of functionality/regression |
API changes
Comment | File | Size | Author |
---|---|---|---|
#57 | 2390065-forum-shadow-copy-57.patch | 4.98 KB | jaapjan |
#46 | forum-shadow-2390065-46.patch | 5.02 KB | JvE |
Issue fork drupal-2390065
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
larowlanNot sure it's major
Comment #2
David_Rothstein CreditAttribution: David_Rothstein commentedMight 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.
Comment #3
lokapujyaIn forum_node_presave(), $node->shadow doesn't exist. I think the checkbox (which is added in form_alter) needs to be handled differently.
Comment #4
lokapujyaIn 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?
Comment #5
lokapujyaComment #6
larowlanComment #7
larowlanComment #9
lokapujyaLooks 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?
Comment #12
lokapujyaredo test-only patch which did not apply. It needed to be applied on top of #7.
Comment #13
lokapujyaComment #15
jhedstromComment #16
rpayanmComment #18
jhedstromBack to needs review since it was the test-only patch that failed.
Comment #19
larowlanLooks good to me, added beta eval
Comment #20
catchDo 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.
Comment #21
larowlanAdded 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.
Comment #22
larowlanDigging 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.
Comment #23
fubhy CreditAttribution: fubhy commentedIf it never worked and noone noticed, maybe it's a good opportunity to remove it for realz? :)
Comment #24
larowlanThere are a few bugs, probably some are duplicates/resolved once this is in.
https://www.drupal.org/project/issues/drupal?text=shadow&component=forum...
Comment #25
catchIt 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.
Comment #26
webchickI 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?
Comment #27
lokapujyareroll.
Comment #28
MichelleFor 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.
Comment #32
larowlanRe-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?
Comment #33
andypostPatch needs short array code syntax
this change looks strange or out of scope
needs reroll for short array
Comment #34
Vidushi Mehta CreditAttribution: Vidushi Mehta at gai Technologies Pvt Ltd commentedWas unable to apply patch on local on 8.4.x, So i have rerolled the patch #27
Comment #35
pk188 CreditAttribution: pk188 at OpenSense Labs commented.
Comment #36
pk188 CreditAttribution: pk188 at OpenSense Labs commentedComment #37
Vidushi Mehta CreditAttribution: Vidushi Mehta at gai Technologies Pvt Ltd commented@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.
Comment #38
pk188 CreditAttribution: pk188 at OpenSense Labs commented@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.
Comment #39
andypost#33.1 still a question
Comment #40
Vidushi Mehta CreditAttribution: Vidushi Mehta at gai Technologies Pvt Ltd commented@andypost can u please describe what is left or incorrect.
Comment #44
JvE CreditAttribution: JvE at One Shoe commentedRe-rolled #34
Comment #46
JvE CreditAttribution: JvE at iO commentedComment #47
longwaveComment #49
andypostso only needs to fix a test
Comment #57
jaapjan CreditAttribution: jaapjan at 25Knots for One Shoe commentedI'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.
Comment #58
eelkeblokComment #62
quietone CreditAttribution: quietone at PreviousNext commentedForum 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.
Comment #66
eelkeblokI 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.
Comment #67
quietone CreditAttribution: quietone at PreviousNext commented