Updated: Comment #N
Problem/Motivation
Lots of custom code to create /forum and /forum/x
Lets see if we can drop a lot of it and use views.
I say 'lets see' because my first attempt didn't get very far, so we'll need some custom handlers.
Proposed resolution
Attempt to replace /forum and /forum/x with views
Remaining tasks
Everything
User interface changes
Views instead of custom foo.
API changes
Most likely changes to ForumManagerInterface.
Comment | File | Size | Author |
---|---|---|---|
Screenshot 2014-02-28 12.16.44.png | 64.93 KB | larowlan |
Issue fork forum-2207263
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
xjmComment #2
xjmComment #3
xjmComment #5
oadaeh CreditAttribution: oadaeh commentedThe attached patch no longer applies, and when manually recreating it, no longer works as advertised. I started working on it, but haven't got far enough to address the functionality that no longer works.
Comment #7
rfmarcelino CreditAttribution: rfmarcelino commentedComment #9
rfmarcelino CreditAttribution: rfmarcelino commentedComment #10
xjmThis issue was marked as a beta target for the 8.0.x beta, but is not applicable as an 8.1.x beta target, so untagging.
As a disruptive change, this would need to be targeted for 8.2.x at this point.
Comment #12
larowlanComment #13
manuel garcia CreditAttribution: manuel garcia as a volunteer and at Appnovation commentedRerolled, omitted this change:
This is now already done on Drupal\taxonomy\TermViewsData::getViewsData()
Comment #14
manuel garcia CreditAttribution: manuel garcia as a volunteer and at Appnovation commentedI've been playing around with the patch, the view doesn't even show up. Looks to me like we might be better off with rebuilding the view by hand.
We should probably do that, export it and put it under
core/modules/forum/config/optional/views.view.forum_forums.yml
Comment #15
cebasqueira CreditAttribution: cebasqueira at CI&T commentedComment #16
cebasqueira CreditAttribution: cebasqueira at CI&T commentedComment #17
faline CreditAttribution: faline at CI&T commentedIt's works for me!
Comment #18
manuel garcia CreditAttribution: manuel garcia as a volunteer and at Appnovation commentedThanks @cebasqueira, for those wondering, patch #15 moves the file into the optional folder.
@faline, we still need to get the view re-done & re-exported for this to work, setting this to Needs work.
Comment #20
mohit_aghera CreditAttribution: mohit_aghera as a volunteer and at Axelerant commentedI tried to reimplement view again for 8.4.x branch.
I have attached implementation for view. I have noticed following things that needs some work on below topics, and need some help in those.
It causes some issues in displaying latest comment date.
View doesn't displays results when we don't have any containers around forum.
Comment #25
dillix CreditAttribution: dillix commented@mohit_aghera we need to resolve #2086125: Last read comment field/filter/argument uses still the node.changed instead of node_field_data.changed column for latest comment dates.
Comment #26
aaronmchaleAdded issue linked in #25 as related
Comment #29
vsujeetkumar CreditAttribution: vsujeetkumar at Material for Drupal India Association commentedPatch created after test fixed, Please review.
Comment #31
vsujeetkumar CreditAttribution: vsujeetkumar at Material for Drupal India Association commentedMore test fixed, Please review.
Comment #33
vsujeetkumar CreditAttribution: vsujeetkumar at Material for Drupal India Association commentedMore test fixing, Please review.
Comment #35
vsujeetkumar CreditAttribution: vsujeetkumar at Material for Drupal India Association commentedTry to fix tests, Please review.
Comment #37
manuel garcia CreditAttribution: manuel garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedThanks for working on this @vsujeetkumar, I thought I'd give it another push:
uuid
should be omitted, since this is a default view.Updating
Upgrade7Test
andUpgrade6Test
view counts since we are adding a new view here.I also imported, saved and re-exported the view to get the correct configuration which should also get
\Drupal\KernelTests\Config\DefaultConfigTest
passing.Comment #39
manuel garcia CreditAttribution: manuel garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedLet's try that again.
Comment #40
lendudeI haven't even applied the patch yet, so these are just general observations.
We need screenshot of how the new view looks, to compare to the non-view situation. Also some form of test coverage would be nice, but not sure how that should look. Should all functional tests pass when switching to the view-version? How do we want to be sure we are not losing any functionality?
Also, I think the scope of the issue is a little too big. 'create /forum and /forum/x' should probably be split up into two issues, or is there a hard reason that these need to be moved over at the same time?
Comment #41
larowlanNo, not really, if there's no economy-of-scale
Comment #47
dillix CreditAttribution: dillix commentedShould we change project from 'Drupal Core' to 'Forum' since Forum has splitted from core?
Comment #49
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #50
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 #52
quietone CreditAttribution: quietone at PreviousNext commentedComment #53
larowlanComment #54
quietone CreditAttribution: quietone at PreviousNext commentedAnd no longer a child of the core issue.
Comment #55
solideogloria CreditAttribution: solideogloria commentedThings to fix (work in progress):
Comment #56
solideogloria CreditAttribution: solideogloria commentedNone of the existing patches are against the contrib module.
Comment #58
solideogloria CreditAttribution: solideogloria commentedI added patch #39 to the MR, then made some initial fixes (fixing #1, 2, and 4 in my earlier comment) and re-exported config.