Updated: Comment #9
Problem/Motivation
On pages like https://drupal.org/book/export/html/1388470 the string 'Add new comment' is added by the comment module when having a config of COMMENT_FORM_SEPARATE_PAGE.
In #754760: "Add new comment" appears directly above comment form / "post comments" does not work without "access comments" permission this is solved for search_index and search_result but missed the print view mode.
Checking this for Drupal 8 gives similar result but when COMMENT_FORM_BELOW it also prints the whole comment form on the Printer friendly page.
Steps to reproduce
- Enable the book module
- Create a book
- Create a book content node 'Book 1'.
- In 'Book outline' choose 'Create a new book'.
- Save and publish.
- Add a child page
- Click on link 'Add child page' below 'Book 1'
- Set title to 'Book Child 1'
- Make sure to add to book outline. Choose 'Book 1'
- Save and publish
- Check printer friendly
- Click 'up' then 'Printer-friendly version' link from 'Book 1'
- Expect to see 'Book 1' + 'Book Child 1' (Bug: no 'Book child 1')
- Add comment field
- Go to /admin/structure/types/manage/book/fields
- 'Re-use existing field' Comment field
- Check for comment form
- Go back to 'Book 1' and click 'Printer-friendly version' link
- Check the comment form 'Add new Comment'
- Check for comment link
- Go back to /admin/structure/types/manage/book/fields/node.book.comment
- Uncheck 'Show reply form on the same page as comments' on
- Go back to 'Book 1' and click 'Printer-friendly version' link
- See link 'Add new Comment'
- Remove comment form from print display mode
- Go to /admin/structure/types/manage/book/display
- Add display mode 'print'
- Remove comments from 'Print' display on /admin/structure/types/manage/book/display/print
- Go back to 'Book 1' and click 'Printer-friendly version' link
- Now re-check 'Show reply form on the same page as comments'. This will 'fix' (no bug) the 'Add comment form' on the printer friendly page.
- Go back to 'Book 1' and click 'Printer-friendly version' link
Proposed resolution
Add the filter for print view mode similar to search_index and search_result.
Remaining tasks
Is it OK to add dependency to Comment module for the only Book Test class or should we write a new Test?- Write Drupal 7 tests: yes or no?
- Backport patch #61 to D7.
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#61 | 2141929-comment-book-59.patch | 5.25 KB | andypost |
#61 | 2141929-comment-book-59-fail.patch | 2.64 KB | andypost |
#61 | interdiff.txt | 1.17 KB | andypost |
core-comment-print-mode-D7-do-not-test.patch | 688 bytes | clemens.tolboom | |
Comments
Comment #1
clemens.tolboomComment #2
clemens.tolboomComment #4
clemens.tolboomFailed due to
So let's first true a re-test.
Comment #5
clemens.tolboomcore-comment-print-mode.patch queued for re-testing.
Comment #7
clemens.tolboomComment #8
clemens.tolboomcore-comment-print-mode.patch queued for re-testing.
Comment #9
helmo CreditAttribution: helmo commentedComment #10
webchickLet's get a test for this.
Comment #11
clemens.tolboomTests related to this issue
So we need a test for Book module with comments enabled. Unfortunately
function CommentLinksTest::testCommentLinks()
usesTestBase::generatePermutations()
which does not seem trivial to define a book + comment test for.Comment #12
clemens.tolboomI add this #1166114: Manage Displays Search Results doesn't manage the display of the search results as I think comments on book pages especially the printer friendly variant is a display mode.
Also #1338858: Include theme print stylesheets in Book printer friendly export makes book/export behavior weird.
Enabling comment module for BookTest and creating comment entity for book module gives
so we're good.
Comment #13
clemens.tolboomAttached patch fails for "Add new comment"
I'm not happy with the interdependency with comment module introduced by the patch. Can't we do better by creating a new test class like BookCommentTest?
Comment #15
clemens.tolboomAttached patch merges the fix from #0 and the test from #13
Comment #17
clemens.tolboomFails on
DefaultViewRecentComments: Drupal\comment\Tests\Views\DefaultViewRecentComments->testBlockDisplay()
DefaultViewsTest.php: Drupal\views\Tests\DefaultViewsTest->testDefaultViews()
Fatal:
Comment #18
clemens.tolboomRunning the failed tests locally gives no error ... is this a testbot hickup #9999 :(
XREF #2100577: Phase 1 - Decouple book module from menu inc
Comment #19
clemens.tolboom15: core-comment-print-mode-2141929-15.patch queued for re-testing.
Comment #20
clemens.tolboomComment #21
clemens.tolboomComment #22
larowlanOuch, this is becoming unwieldy. Can't the comment field just be removed from the display settings on the print view mode by the site builder?
Correct me if I'm wrong but the comment functionality isn't added to the book node type by default (ie when the book module is enabled) so the user would have had to add this field. Isn't setting the correct display settings part of that process?
Related #2114887: Maximum nesting level when attaching comment field to User.
Comment #23
clemens.tolboom@larowlan I totally agree with the
'They' did this for search_index and search_result so I blindly copied it over to print.
Comment #24
larowlanWhat about 'Can't the comment field just be removed from the display settings on the print view mode by the site builder?'
Comment #25
clemens.tolboomI will recheck this later on this week. IIRC the <list>comment is always printed.
Comment #26
clemens.tolboomComment #27
clemens.tolboomComment #28
clemens.tolboomComment #29
clemens.tolboom@larowlan I configured the 'print' display mode for D8 and feel quit stupid :-/
The 'Add new comment' form disappears as it's field is not displayed.
The link 'Add new comment' still appears.
The book tree for 'print friendly' is broken 'Book Child 1' is not displayed.
I'll try to find the link bug and check Drupal 7 again.
Comment #30
clemens.tolboomAdded the Display mode steps.
Comment #31
clemens.tolboomThis needs work:
- The 'Add new comment' form can be removed through the 'print' display mode settings
- The 'Add new comment' link is still open. This happens when 'Show comment form on another page'.
- How does this work for Drupal 7?
Comment #32
clemens.tolboomComment #33
clemens.tolboomComment #34
clemens.tolboomI've removed the form hiding parts.
In #1166114-107: Manage Displays Search Results doesn't manage the display of the search results I asked for a better solution for hiding the comment link as we are doing something wrong.
If display mode dictates not to show comments it should not display the link either. Should this be a pseudo field providing a link 'Add new comment'?
The code in comment_entity_view() currently is node specific and assumes too much. But it has a TODO refering to #1901110: Improve the UX for comment bundle pages and comment field settings
Same goes for CommentDefaultFormatter::viewElements() I guess. Why isn't this handled by display mode either?
I dunno how to solve this so hints are welcome.
Comment #35
clemens.tolboom(darn: I was distracted about preview is not rendering my comment :/)
Comment #37
jhodgdonIs there some way we could make this smarter? Because I'm finding we will need to add yet another exception to that line in comment.module in #1166114: Manage Displays Search Results doesn't manage the display of the search results.
It just seems stupid. We should have a better way to manage it.
Comment #38
clemens.tolboomAdded link to #1901110: Improve the UX for comment bundle pages and comment field settings which has most desired settings in it.
I think we can set version to D7 then.
Comment #39
andypostLooks as good workaround with test!
Just a minor nitpicks
Should be t('Add new comment')
Suppose better to add 'continue;' here
Comment #40
mgiffordAndy, what do you mean by "Suppose better to add 'continue;' here"?
Comment #41
mgiffordRelated issue on d.o waiting for this #2139847: Book printer friendly pages has 'Add Comment' all over
Comment #44
andypostThe proper fix should remove comment add form too.
@mgifford please check my grammar in comment formatter
PS: Working on test
Comment #45
mgiffordIt is
// We don't want this links if we're building the entity for:
It should be either (if plural links):
// We don't want these links if we're building the entity for:
or (if singular link):
// We don't want this link if we're building the entity for:
Small thing, but:
// display below the entity. Do not show the form for print view mode.
Should be capitalized:
// Display below the entity. Do not show the form for print view mode.
Comment #46
andypostChanged code comments a bit, added test. Suppose RTBC
Comment #47
larowlanAssuming bot agrees.
I'm on record above saying I don't like that comment module is aware of search and print view modes, but this is existing in HEAD and this patch solves the issue in the OP, fixing the wider problem is out of scope.
We have existing issues to make the formatter more configurable and I'll try and pick them up again next week.
Comment #51
mgifford46: 2141929-comment-print-46.patch queued for re-testing.
Comment #52
mgifford46: 2141929-comment-print-46-fail.patch queued for re-testing.
Comment #55
clemens.tolboomRemoved bug from steps to reproduce.
Testbot is buggy as the failed test CommentBookTest.php succeeds on local.
Comment #56
clemens.tolboom46: 2141929-comment-print-46.patch queued for re-testing.
Comment #58
andypost46: 2141929-comment-print-46.patch queued for re-testing.
Comment #61
andypostThe comment was saved unpublished so run-tests is less affected leaking from host site, this should be fine after #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead
So now it works.
Comment #63
andypostBack to RTBC
Comment #64
larowlan+1 RTBC as per #47
Comment #65
jhodgdonI noticed that this change:
is only being made for print mode. Can't we also make this for search indexing and search results? It shouldn't be there either. See
#1113832: [PP-1] Comment module renders "reply" and other links in search index/results
which has this marked as a related issue. The other changes in this issue were made for all three, such as:
It would be great if this one could be too. Is it too late to revisit the patch?
Comment #66
jhodgdonA suggestion for a test for the search-related behavior:
- enable search module
- make sure anonymous users have permission to post comments (without approval) and search
- make a node on a content type with comments enabled
- run cron to index
- search for the word "reply". If you get a result, then the "reply" link was rendered in the search indexing step and it shouldn't have been.
- search for a word that is in the node body or node title. Verify that the word "reply" is not shown on the screen.
Comment #67
andypost@jhodgdon
1) the formatter already checks search view modes just before the fixed code (see line 122)
!in_array($this->viewMode, array('search_result', 'search_index'))) {
and formatter's purpose to display "comment thread and form", so current patch hides the form
2) this is a fix amd a bit of optimization, suppose this covers the issue #1113832: [PP-1] Comment module renders "reply" and other links in search index/results
So patch does not need any changes, also it makes sense to add the test in following issue
Comment #68
andypostre-order patches
Comment #69
jhodgdonRE #67, oh, right, I see that... well we can work on the test in the other issue and see if it's still a problem after this patch, which I think will not fix the other issue.
The problem on that other issue is actually that the reply link is shown during search indexing and search results display, which is not affected by this patch, I think.
Comment #70
jhodgdonSo... Let's step back a moment and think about this really kind of kludgy code.
Field formatters can have settings.
So wouldn't it be more sensible rather than special-casing the search_index, search_results, and print display modes, to have settings in the Comment field formatter that say things like:
- If this setting is FALSE, skip rendering the comment form
- If this other setting is FALSE, skip rendering the "add" and "reply" links
This would allow us to remove the ad-hoc code in both of these places... just a thought, but maybe coming too late.
Comment #71
clemens.tolboomIn #12, #34 and #37 we scratched the 'can we do better'.
iirc the UL tag and content is still on the print-friendly page
Let's fix 'can we do better' through either #1113832: [PP-1] Comment module renders "reply" and other links in search index/results and/or #1166114: Manage Displays Search Results doesn't manage the display of the search results
Comment #72
catchOK I agree this is messy but also that it's OK to fix it with workaround + tests then follow-up with the refactoring. We likely won't be able to backport the refactoring.
Committed/pushed to 8.x, moving to 7.x for backport.
Comment #73
johnvI've marked [#583576s] as a duplicate.
Patch #61 is a D8-patch and needs backporting.
Comment #74
clemens.tolboom