Follow up for #731724: Convert comment settings into a field to make them work with CMI and non-node entities

Problem/Motivation

#731724: Convert comment settings into a field to make them work with CMI and non-node entities Moves comment to a field api field. We would love to get the threading and pager settings into the formatter, instead of the field settings, this would allow this to be used in views etc ie anywhere else display settings can be configured
Also the UX around field settings is poor.
Also there is lots of juggling code around view modes in the comment formatter.
Lets clean it up

Proposed resolution

Move threading and pager settings into formatter settings (from field settings).
Move comment_new_page_count and comment_num_new to the CommentManager service
Change arguments to comment_new_page_count to include passing the number-per-page and the threading mode as arguments instead of fetching them from the field.
Update formatter-output to include data-comment-per-page and data-comment-mode attributes. Send these attributes to CommentController::renderNewCommentsNodeLinks() allowing it call the comment_new_page_count function with the new arguments (note it is the only place this function is called).

Remaining tasks

  • Write the patch.
  • Take screenshots (show the current UI and annotate showing what will change).

User interface changes

Settings will be moved from field settings to formatter settings.

API changes

comment_new_page_count moved to manager service and new arguments.
Note CommentController::renderNewCommentsNodeLinks is the only place this function is called.
This will move settings from comment-field instance settings to one of comment-formatter or comment-type config-entity - so config schema changes

Related

Postponed on #2068331: Convert comment SQL queries to the Entity Query API and #1920046: Remove comment_entity_view and introduce second formatter for links

Files: 
CommentFileSizeAuthor
#102 interdiff.txt1.57 KBgoogletorp
#102 move_comment_field-1920044-102.patch87.34 KBgoogletorp
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 94,114 pass(es), 660 fail(s), and 295 exception(s).
[ View ]
#100 move_comment_field-1920044-98.patch87.55 KBgoogletorp
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 92,611 pass(es), 2,125 fail(s), and 569 exception(s).
[ View ]
#98 move_comment_field-1920044-98.patch87.55 KBgoogletorp
#86 interdiff-1920044-73-86.txt9.05 KBkerby70
#86 move_comment_field-1920044-86.patch89.81 KBkerby70
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch move_comment_field-1920044-86.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#74 comment-formatter-tune-up-1920044-73.patch94.48 KBroderik
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,572 pass(es).
[ View ]
#72 interdiff-1920044-71-72.txt13.88 KBroderik
#71 interdiff-1920044-69-71.txt16.63 KBroderik
#69 interdiff-1920044-67-69.txt1.8 KBroderik
#67 interdiff-1920044-65-67.txt3.76 KBroderik
#65 interdiff-1920044-63-65.txt666 bytesroderik
#63 interdiff-1920044-45-63.txt94.64 KBroderik
#45 1920044-comment-formatters-45.patch57.93 KBandypost
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,999 pass(es), 236 fail(s), and 21 exception(s).
[ View ]
#35 comment-formatter-tune-up-1920044.ff63dfa.patch60.36 KBlarowlan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,548 pass(es), 247 fail(s), and 20 exception(s).
[ View ]
#35 interdiff.txt5.57 KBlarowlan
#31 Screenshot 2014-06-23 15.24.37.png17.96 KBlarowlan
#31 Screenshot 2014-06-23 15.24.03.png17.05 KBlarowlan
#31 Screenshot 2014-06-23 15.23.58.png41.28 KBlarowlan
#31 Screenshot 2014-06-23 15.12.49.png12.62 KBlarowlan
#31 Screenshot 2014-06-23 15.12.33.png18.98 KBlarowlan
#31 comment-formatter-tune-up-1920044.1.patch57.37 KBlarowlan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,066 pass(es), 206 fail(s), and 16,433 exception(s).
[ View ]

Comments

dixon_’s picture

Title:Move comment threading and per page settings to Formatter settings» Move comment threading and per page settings to formatter settings
andypost’s picture

This change require to unify the output generation logic so formatter simply calls a function with parameters.
Also this change affects a way we generate a link to calculate a page for another formatter (links) so this a ONLY problem here

The number of page of the comment depends on both parameters so I think the Links formatter should get settings from "full" mode formatter and fallback to defaults when full is not CommentList

andypost’s picture

Another task here to implement node_title_list() theme-override to show the comment count

podarok’s picture

The number of page of the comment depends on both parameters so I think the Links formatter should get settings from "full" mode formatter and fallback to defaults when full is not CommentList

Looks like this workaround brokes standard drupal view modes
Possibly if this will be resolved in such way - should we provide a good docs and disabling some administration forms in core for comments?

andypost’s picture

Status:Postponed» Active
larowlan’s picture

Assigned:Unassigned» larowlan

Working on this one too

larowlan’s picture

Updated issue summary with discussions regarding CommentController::renderNewCommentsNodeLinks()

larowlan’s picture

Issue tags:+API change

Tagging for the changes to comment_new_page_count

larowlan’s picture

Issue summary:View changes

Updated issue summary.

Wim Leers’s picture

Yep, CommentController:renderNewCommentsNodeLinks() should then be updated to either receive the number of comments per page (meaning that they'd be stored in the client-side, meaning that a malicious user could easily try to overload the server by manipulating this number) or the formatter ID with which that setting is associated. The latter seems to be the safer option.

I'd say the route's path should be updated from

path: '/comments/render_new_comments_node_links'

to:

path: '/comments/render_new_comments_node_links/{formatter}'
larowlan’s picture

But what if views uses it as a formattter, we don't have a formatter id?

Wim Leers’s picture

So how does Views do it then? If Views is not using the same render pipeline, then quite possibly it should provide a similar yet different route that accepts the view ID and display ID, and retrieves the settings from there?

andypost’s picture

Not sure I get the new idea about node-links but originally the was idea to implement second formatter to use for teaser to display links only.
Also logic to count page and query thread could be changed slightly in #2068331-30: Convert comment SQL queries to the Entity Query API

larowlan’s picture

larowlan’s picture

Issue summary:View changes

Updated issue summary.

larowlan’s picture

Issue summary:View changes

uis

clemens.tolboom’s picture

From what I read in #1901110-6: Improve the UX for comment bundle pages and comment field settings from one of the screenshot has a 'Allow comments link' but that's removed from that issue as #1901110-13: Improve the UX for comment bundle pages and comment field settings mentioned this issue.

Reading here I don't read about hiding the 'Add new comment' link per display mode as #2141929: Comment link or form is added to print view mode. and #1166114-107: Manage Displays Search Results doesn't manage the display of the search results.

Am I wrong or can I try to fix for only D7 #2141929: Comment link or form is added to print view mode.?

larowlan’s picture

Status:Postponed» Active

Kicking this back off

larowlan’s picture

Title:Move comment threading and per page settings to formatter settings» Move comment field settings that relate to rendering to formatter options
Priority:Normal» Critical
Issue summary:View changes
andypost’s picture

I still think we need some kind of "inheritance" for this settings.
At least "per-page" should be inherited for formatter from field or instance or comment bundle

larowlan’s picture

by inheritance you mean a default value?

andypost’s picture

exactly default value

andypost’s picture

To properly retrieve settings from formatter within other code this code needs to provide view mode or entity display with settings for formatter.
So the only question how to pass data-* attributes from formatter to node links

Wim Leers’s picture

#20: Can you explain what the problem is with the data-* attributes?

andypost’s picture

The problem is that comment field and node links are laying in different elements.
So when comment field is displayed (currently with field and comment wrappers) then some JS needs to pull this into node links.
But comments could be attached to any entity so this kind of formatter should work everywhehe

Wim Leers’s picture

#22: so… are you saying that comment-new-indicator.js and node-new-comments-link.js should be updated to work with entities in general and not Node entities in specific? Is that all, or is there more?

andypost’s picture

Yes, both should be updated. Also code in node links should be tuned.
I just have no idea how to display this links for entities that have no "node-links"

catch’s picture

I think node links are more the problem than the other entity types, that's Drupal 4.7 (or earlier) stuff we've never refactored.

xjm’s picture

Priority:Critical» Major
Issue tags:+Usability, +Needs issue summary update, +minor version target, +Needs screenshots

We discussed this today with @Dries, @alexpott, @catch, and @webchick. This was elevated to critical because #1901110: Improve the UX for comment bundle pages and comment field settings was critical. However, we agreed this morning that while the usability issues with the current UI are pretty significant, they are not release-blocking. We also agreed that we could add an improved UI in a minor release (e.g. 8.1.0) if it is not ready for 8.0-rc1. I'm introducing a "minor version target" tag for that.

The issue summary could use an update here, especially to include the scope of the duplicate issues. @yoroy provided UX feedback with screenshots in #1901110: Improve the UX for comment bundle pages and comment field settings that should be documented and incorporated in this issue as well -- the current summary is very weak at explaining the UI changes. Let's add a screenshot at least showing the current UI and with maybe an annotation showing what will change.

TallDavid’s picture

Issue summary:View changes
larowlan’s picture

larowlan’s picture

larowlan’s picture

Close to a patch here...

larowlan’s picture

Status:Active» Needs review
StatusFileSize
new57.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,066 pass(es), 206 fail(s), and 16,433 exception(s).
[ View ]
new18.98 KB
new12.62 KB
new41.28 KB
new17.05 KB
new17.96 KB

Lets see how broken tests are.

Screenshots from manual testing.

Comment-field settings


These could probably be moved to the comment-type settings eventually.

Comment formatter summary


Comment formatter settings form


Options for links


Comment link formatter summary

larowlan’s picture

Issue tags:-Needs screenshots

Status:Needs review» Needs work

The last submitted patch, 31: comment-formatter-tune-up-1920044.1.patch, failed testing.

larowlan’s picture

hmm comment_get_display_ordinal() and comment_get_display_page() going to be an issue. Called from the CommentForm submit handler...
Might need to inject the view_mode into the form when its displayed... should be doable, as form is tied to a formatter.

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new5.57 KB
new60.36 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,548 pass(es), 247 fail(s), and 20 exception(s).
[ View ]

Should fix a few tests

Bojhan’s picture

Interesting, this move makes sense. It will be a bit of a search for our existing users, but that should be fine.

1) I am not sure what "Show links" does frankly?
2) Can we remove the description for threading? We can probably just move most of that into the label.
3) We can remove the description for Show links

Status:Needs review» Needs work

The last submitted patch, 35: comment-formatter-tune-up-1920044.ff63dfa.patch, failed testing.

larowlan’s picture

I'm no longer convinced that 'no per page' and 'threading mode' belong on formatter, but certain that links do (so they can be controlled with formatter options/disabled etc). In fact I think 'no per page' and 'subject' etc all belong on comment-type settings form now, not in field settings and only 'pager id' and 'link style' are formatter settings with no settings for field or instance at all - thoughts?
Moving those options to the comment-type is consistent with node-type etc.

Assigning to andypost for feedback.

larowlan’s picture

+++ b/core/modules/comment/src/CommentForm.php
@@ -395,8 +395,9 @@ public function save(array $form, array &$form_state) {
-      $field_definition = $this->entityManager->getFieldDefinitions($entity->getEntityTypeId(), $entity->bundle())[$field_name];
-      $page = comment_get_display_page($comment->id(), $field_definition);
+      $display_settings = entity_get_display($entity->getEntityTypeId(), $entity->bundle(), 'default')
+        ->getComponent($field_name);
+      $page = comment_get_display_page($comment->id(), $display_settings);

This change is why I'm not certain that they belong on the formatter. The comment-form needs to know the number per page and threading mode in order to create the link fragment after a new comment is added - to redirect to that comment. On one hand we're already assuming that the comment came from the full-entity view mode (because we're using the commented entity's url info to build the link) - is it a bridge too far to assume that implies the default view mode? Or should we remove this redirect all together and make it configurable per-comment-type? Options would be 'commented entity in default view mode', 'comment reply form' and 'none'.

jhodgdon’s picture

This seems like a can of worms. In order to make this permalink to the comment, you have to know how/where the comments are displayed, which is the "full" view mode setting on the entity bundle, I guess? This does seem like a formatter setting for that same type of comment?

larowlan’s picture

Assigned:larowlan» andypost

So after discussing with @jhodgdon on irc, I think the best option here is to continue with the patch approach but make the redirect location configurable. In some instances the full view mode of the entity will be correct, but we shouldn't assume that.

Thoughts on that too?

andypost’s picture

Assigned:andypost» larowlan

Yep, this is "pandora box" of comment field.

Each time the comment field is rendered the formatter should know the view mode.
Each comment entity render should care about user permissions to render links properly.
The comment permalink still not solved:
#2010202: Comment permalinks are lacking context and present a Usability regression (for e.g. drupal.org) & #2113323: Deprecate Comment::permalink() method and rename it to not be ambiguous with ::uri()
and each calculation needs a lot of resources.

Also comment render cache is broken now #2254181: Comment indentation is incorrect for comments following a replying-comment: don't render cache comments for which threading is enabled
And finally we are in progress to convert queries to EQ.

As we discussed this options my vision was to place:
1) threading to comment type.
2) per page and form placement to formatter.
3) pager_id was introduced as quick fix but still there, no idea how to fix that properly.

"show_links" - nice idea for formatter! just take care about render cache.

Overall +1 to current approach, will try to help.

PS: about dancing with node view modes - please try to use entity display properly, I mean to get rid of unset() and move comment field to "hidden"

  1. +++ b/core/modules/comment/comment.install
    @@ -31,6 +31,32 @@ function comment_install() {
    +  // Book is being enabled.
    +  if (in_array('book', $modules)) {
    +    // Disable comments in print view mode.
    ...
    +  // Comment is being enabled, check if book exists.
    +  if (in_array('comment', $modules) && \Drupal::moduleHandler()->moduleExists('book')) {
    +    // Disable comments in print view mode.

    better to merge

  2. +++ b/core/modules/comment/comment.module
    @@ -262,26 +262,28 @@ function comment_permission() {
       $flat = $mode == COMMENT_MODE_FLAT ? TRUE : FALSE;

    please, fix that

  3. +++ b/core/modules/comment/comment.module
    @@ -353,157 +355,17 @@ function comment_entity_build_defaults_alter(array &$build, EntityInterface $ent
    +      $variables['content']['links'] += $variables['content'][$field_name][0]['links'];

    suppose better to unset a whole field unset($variables['content'][$field_name]);

  4. +++ b/core/modules/comment/comment.module
    @@ -1148,15 +1010,15 @@ function comment_get_display_ordinal($cid, FieldDefinitionInterface $field_defin
    + * @param array|NULL $display_settings
    ...
    +function comment_get_display_page($cid, $display_settings) {
    ...
    +  $comments_per_page = $display_settings['settings']['per_page'];

    you get warning with NULL

andypost’s picture

andypost’s picture

Comment subject visibility is tricky in #2292821-7: Use widget for comment subject field

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new57.93 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,999 pass(es), 236 fail(s), and 21 exception(s).
[ View ]

re-roll, pushed to github

Status:Needs review» Needs work

The last submitted patch, 45: 1920044-comment-formatters-45.patch, failed testing.

andypost’s picture

Facing with #2324719: Node indexing - should use view mode for comments, not hook shows that we need to move per page settings to formatter to get rid of view mode check

jhodgdon’s picture

I took a look at some of this patch... mostly looks good! At the top though, I'm not sure why there are two nearly identical blocks in function comment_modules_installed($modules) ... couldn't those two if() statements be combined?

larowlan’s picture

Issue tags:+beta deadline

We really need to sort this before beta

larowlan’s picture

This will move settings from comment-field instance settings to one of comment-formatter or comment-type config-entity - to be explicit.
Still chasing feedback on #38 and #39

larowlan’s picture

Issue summary:View changes
roderik’s picture

Status:Needs work» Needs review
StatusFileSize
new51.61 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,749 pass(es), 94 fail(s), and 142 exception(s).
[ View ]

In the process of rerolling. This patch now has duplicated code in CommentDefaultFormatter (from this issue) and CommentLinkBuilder (from 8.0.x / #2318251: Make comment links functionality testable and convert CommentLinkTest to PHPUnit) which I'll dedupe out soon -- in the meantime, the testbot can figure out whatever I messed up, overnight.

Also on github when push finishes.

Status:Needs review» Needs work

The last submitted patch, 52: comment-formatter-tune-up-1920044-52.patch, failed testing.

roderik’s picture

Status:Needs work» Needs review
StatusFileSize
new52.82 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,812 pass(es), 62 fail(s), and 1 exception(s).
[ View ]
new6.54 KB
new1.42 KB

still 'needs work' but I'm using the testbot again to see what sticks.

Duplicate code still there, I now think I should

Status:Needs review» Needs work

The last submitted patch, 54: comment-formatter-tune-up-1920044-54.patch, failed testing.

catch’s picture

OK the API change here is not going to affect much/anything - definitely not out of beta scope.

However I would not want to deal with the risk/complexity of having to write an upgrade path for this during beta.

So... I'm retagging as 'D8 upgrade path' / minor version target. If this gets in before we support beta-to-beta updates then I think it's an OK change to make. After that don't fancy it before release candidate.

If it misses that window, and we can get the upgrade path right and fully tested it could still get into 8.1.x (or later) as a UI improvement - but that gives us an extra six months or so to ensure the upgrade path is spot on and test it on real installs.

roderik’s picture

Assigned:larowlan» roderik

(coming up)

larowlan’s picture

Hi @roderik, I'm still chasing feedback on #38 and #39, so don't sink too much time into this in case we about face. Also w.r.t. links I think it makes sense to separate the concerns now that links are a distinct display component in core. I.e. we don't need to worry about making them configurable any more, they already are in head.

roderik’s picture

Status:Needs work» Needs review
StatusFileSize
new22.12 KB
new3.41 KB
new38.89 KB
new48.94 KB
new10.23 KB
new92.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,855 pass(es), 1,298 fail(s), and 565 exception(s).
[ View ]

So, it's about time I uploaded the current status. Don't worry if there's wasted work - this was useful as practice in config entities/field settings/formatters/unit tests etc. if nothing else.

Unfortunately there's a lot of stuff to split out, which I tried and only half finished.

1 - reroll

3 interdiffs:
1a is unimportant, just reroll/test fixes from last patches.

1b is a few things I thought should be fixed. (Primarily: I think the teaser vs full node links were switched - but it's a bit hard to test still)

1c unifies the duplicate code in CommentLinkBuilder and CommentDefaultFormatter - it creates a new method CommentLinkBuilder::buildCommentedEntityFieldLinks() (with arguments tuned to CommentDefaultFormatter's use case).

Status:
CommentLinkBuilderTest fails - I believe that is because of the new CommentDefaultFormatter::LINKS_NONE formatter-setting which the test does not yet account for: CommentDefaultFormatter::LINKS_NONE outputs no link but (in at least one case) the test expects an 'access denied' message.

So, the checking logic still needs to be reviewed.

2 - feedback on #38 - #41

I believe that the # of comments does belong (should stay) in the formatter settings. (I think there is a theoretically valid use case for having small/big thread pages on different display modes.)

I believe that the threading mode should indeed move to comment-type settings. Not 100% sure on it, but my feeling is mixing up threaded/non-treaded rendering of the same comments is a can of worms and we shouldn't enable regular people to do that. (Why: because if you render threaded comments as 'flat', you don't see what the parents are / what comments you're replying to - sometimes leading to creepy situations. Which would require a solution like https://www.drupal.org/project/flatcomments in core to keep lost data / frustration down.)

In other words: agreed with andypost/#41.

'form_location' also seems like a formatter setting to me. (and maybe the new 'redirect' setting? But see below.)

Attached interdiff '2' moves the threaded mode to a comment-type setting, and can be used as a basis for moving other settings over (which I haven't thought about yet).
It also moves and renames the constants from COMMENT_MODE_* to THREADING_MODE_* (the best I could come up with after not liking 'MODE_*, DISPLAY_MODE_*, LIST_MODE_*')

CommentLinkBuilderTest has not been adjusted yet (to mock comment type settings).

3 - configurable redirect location

As mentioned in #41
Attached interdiff '3' is a first stab at this.
But I should have read properly: options should be as per #39 "Options would be 'commented entity in default view mode', 'comment reply form' and 'none'.".
I implemented "to what view mode do you want to redirect", which is a silly question. What could be asked as a question (besides the above options) is "what view mode does /node/NNN actually render? I need to know just for determining the destination page on redirect". But I'm not sure that is understandable/useful.

There is another issue with the patch:

  • It works for setting 'form on same page', and for that case, there is some sense in implementing the 'redirect' option as a formatter setting.
  • it does not yet work for 'form on separate page' and also - for that case, it being a formatter setting would be weird:
    • the standalone form would know which view mode to lift the setting from
    • for all other view modes, the option will be useless.

    ...unless you want to implement an URL (GET) parameter for the standalone form URL...

So I'm uploading the patch for completeness / as maybe a little starting point, but not sure of its use.

The full patch in this issue

... includes inderdiffs 1 and 2, not the last interdiff (3). I already know it will fail CommentLinkBuilderTest, and for the rest... I'm just keeping my fingers crossed.

Pushing to github in case someone wants to cherry-pick.

TODO

  • work out the above confusion still
  • fix CommentLinkBuilderTest for its checking logic and for being able to mock comment-type settings
  • migration of settings (in followup, I think)
  • change record for constants changes (after we decide that all constants are in the right files)
  • Plugin/views/field/NodeNewComments.php is a bit yucky, with its 'threading_mode' and 'per_page' settings. But it's broken anyway, because it should know the comment field(name) it is operating on. Once it knows that, at least the 'threading' option can be deleted again. Do in followup, I think.

Status:Needs review» Needs work

The last submitted patch, 59: comment-formatter-tune-up-1920044-59.patch, failed testing.

roderik’s picture

Status:Needs work» Needs review
StatusFileSize
new97.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

That code was so broken, I don't even...

Here's a new patch including everything but the redirect location (which I'll look at while test bots are picking this apart.)

Pushed to github. I can later provide any interdiffs you want (probably from 45 to the most recent one), and review my own comments.

Status:Needs review» Needs work

The last submitted patch, 61: comment-formatter-tune-up-1920044-61.patch, failed testing.

roderik’s picture

Status:Needs work» Needs review
StatusFileSize
new87.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,813 pass(es), 94 fail(s), and 38 exception(s).
[ View ]
new94.64 KB

Regardless whether this will be (partly or fully) unused, I must not leave a mess in an issue like this.

Testing; comments will follow if it's green.

The interdiff is bigger than the patch because the comment-links stuff that was part of #45 is removed from the field formatter.

Status:Needs review» Needs work

The last submitted patch, 63: comment-formatter-tune-up-1920044-63.patch, failed testing.

roderik’s picture

Status:Needs work» Needs review
StatusFileSize
new666 bytes
new87.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,733 pass(es), 85 fail(s), and 36 exception(s).
[ View ]

silly little mistake, comments still pending when green.

Status:Needs review» Needs work

The last submitted patch, 65: comment-formatter-tune-up-1920044-65.patch, failed testing.

roderik’s picture

Status:Needs work» Needs review
StatusFileSize
new3.76 KB
new90.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,917 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

finding individual issues to fix among the failures...

Status:Needs review» Needs work

The last submitted patch, 67: comment-formatter-tune-up-1920044-67.patch, failed testing.

roderik’s picture

Status:Needs work» Needs review
StatusFileSize
new90.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
new1.8 KB

.

Status:Needs review» Needs work

The last submitted patch, 69: comment-formatter-tune-up-1920044-69.patch, failed testing.

roderik’s picture

Status:Needs work» Needs review
StatusFileSize
new16.63 KB
new94.99 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,529 pass(es).
[ View ]

getting funny warnings at drupalPostAjaxForm() but hoping they might be local to my installation...?

Comments and github push will still follow when green.

roderik’s picture

StatusFileSize
new13.88 KB
new94.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch comment-formatter-tune-up-1920044-72.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

...so. Ignore comments #48 - #71 (except the reference below).

1 - reroll plus fixes (plus removing the links stuff that already have their own formatter)

Pushed to github because the individual interdiffs are pretty much useless (too confusing). I will split stuff up if it helps to review/push things through. (Or if you think parts of this should not go in.)

Also: will update issue summary after there is agreement on below.

2 - feedback on #38 - #42

As detailed in #59, agreed with andypost #42:

  • # comments in formatter settings
  • threading mode in comment type settings (the current patch implements this)

Form location also seems like a formatter setting to me **. However this has implications: when displaying a non-'full' view mode, we first need to check that view mode's formatter settings to see if comments are displayed there. If not, we must check the 'full' view mode's formatter settings to determine whether the "Add new comment" link should link to the form on the full node page (as is the default now/in D7?) or on a separate page.

This is done in interdiffs #69 + #71.
(And it has added a @todo, which is actually a @to-review, to the patch.)

And it introduces some breakage - see bottom of this comment.

3 - configurable redirect location / issues raised in #39

Maybe I'm just slow (like I was in understanding #58)... but I don't think we have a problem here anymore. Just recapping #39 to be sure:

This change is why I'm not certain that they belong on the formatter. The comment-form needs to know the number per page and threading mode in order to create the link fragment after a new comment is added - to redirect to that comment. On one hand we're already assuming that the comment came from the full-entity view mode (because we're using the commented entity's url info to build the link) - is it a bridge too far to assume that implies the default view mode?

Yes that's a bridge too far, but that is no problem. We can just go with the full mode settings. (As done in this comment's interdiff.)

There are more places (CommentLinkBuilder, CommentController::renderNewCommentsNodeLinks()) that take the settings from 'full' view mode because that is the view mode which is hardwired to the entity's URL. AFAICS that's fine.

Or should we remove this redirect all together and make it configurable per-comment-type? Options would be 'commented entity in default view mode', 'comment reply form' and 'none'. (...)So after discussing with @jhodgdon on irc, I think the best option here is to continue with the patch approach but make the redirect location configurable.

Great. But since that's new functionality and it doesn't seem to influence the patch much, it can be a followup - right?

-----

** I think we have to either make 'form location' a formatter setting, or decide to decouple the rendering of the comment form from view modes & only display the form on the entity-url page (irrespective of view mode).

Why that reasoning?
...try this for fun: in HEAD, you can change the 'frontpage' view to display nodes in 'full' view mode. So you'll get comment forms rendered with every displayed node, and (if there are comments already) an "Add new comment" link. But that link redirects to the entity-url, instead of jumping to the form just below it!

The only way to fix that is either of above choices, I think.

This patch takes the first choice and fixes the wrong redirect.

But it (the ability to render & link to the form on any view mode) highlights some breakage. If there are multiple comment forms on the same page, each form should get their own unique #comment-form-XXX HTML id, so the "Add new comment" link can link to the right location on the page.

I'll fix that (here or in followup) when the patch gets reviewed. It won't affect many parts of the patch.

So, todo:

  • unique #comment-form-XXX HTML ids
  • change record, should include constants changes (after we decide that all constants are in the right files)
  • comment form redirect options (in followup, I think)
  • Plugin/views/field/NodeNewComments.php is a bit yucky, with its 'threading_mode' and 'per_page' settings. But it's broken anyway, because it should know the comment field(name) it is operating on. Once it knows that, at least the 'threading' option can be deleted again. Do in followup.
  • migration of settings (in followup, I think)

Status:Needs review» Needs work

The last submitted patch, 72: comment-formatter-tune-up-1920044-72.patch, failed testing.

roderik’s picture

Status:Needs work» Needs review
StatusFileSize
new94.48 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,572 pass(es).
[ View ]

reroll. See #72;

larowlan’s picture

greeeen - review time

larowlan’s picture

Spent about 10 minutes manually testing, couldn't fault it.

Will upload screencast and start code review next

  1. +++ b/core/modules/comment/comment.install
    @@ -32,6 +32,23 @@ function comment_install() {
    + * Implements hook_modules_installed().
    + */
    +function comment_modules_installed($modules) {
    +  // Book was just installed, or Comment was just installed while Book
    +  // is (already/newly) installed too.
    +  if (in_array('book', $modules)
    +      || (in_array('comment', $modules) && \Drupal::moduleHandler()->moduleExists('book'))) {
    +    foreach (\Drupal::service('comment.manager')->getFields('node') as $field_name => $detail) {
    +      foreach ($detail['bundles'] as $bundle) {
    +        $display = entity_get_display('node', $bundle, 'print');
    +        $display->removeComponent($field_name)->save();
    +      }
    +    }
    +  }
    +}
    +
    +/**

    Can you elaborate why this is needed?

  2. +++ b/core/modules/comment/comment.module
    @@ -473,17 +470,18 @@ function comment_node_update_index(EntityInterface $node, $langcode) {
    +        $display_settings = entity_get_display($node->getEntityTypeId(), $node->bundle(), 'default')
    +          ->getComponent($field_name);

    So we're defaulting to 'default' here - but theoretically we could be using a 'search result indexing' view mode yeah?

  3. +++ b/core/modules/comment/src/CommentForm.php
    @@ -384,14 +384,23 @@ public function save(array $form, FormStateInterface $form_state) {
    +      // Find the current display page for this comment. We link to it using
    +      // the standard entity urlInfo, i.e. the full/default view mode.
    +      $page_number = 0;
    +      $display = $this->entityManager->getStorage('entity_view_display')
    +        ->load($entity->getEntityTypeId() . '.' . $entity->bundle() . '.full');
    +      if (!$display) {
    +        $display = $this->entityManager->getStorage('entity_view_display')
    +          ->load($entity->getEntityTypeId() . '.' . $entity->bundle() . '.default');

    I can live with this.

  4. +++ b/core/modules/comment/src/CommentLinkBuilder.php
    @@ -9,16 +9,25 @@
    +

    nitpick: extra blank line

  5. +++ b/core/modules/comment/src/CommentLinkBuilder.php
    @@ -9,16 +9,25 @@
    + * be added to the regular links displayed with the commentED entity. (This is

    Thanks for more documentation, not sure on the capital ED emphasis though. We should add @see for the referenced functions/methods so api.drupal.org can link.

  6. +++ b/core/modules/comment/src/CommentLinkBuilder.php
    @@ -101,86 +118,116 @@ public function buildCommentedEntityLinks(FieldableEntityInterface $entity, arra
    +          // We create links to the first new comment using the standard entity
    +          // urlInfo, i.e. the full/default view mode.
    +          $display = $this->entityManager->getStorage('entity_view_display')
    +            ->load($entity->getEntityTypeId() . '.' . $entity->bundle() . '.full');
    +          if (!$display) {
    +            $display = $this->entityManager->getStorage('entity_view_display')
    +              ->load($entity->getEntityTypeId() . '.' . $entity->bundle() . '.default');
    +          }
    +          $formatter_options_full = $display
    +            ? $display->getComponent($field_name): NULL;
    +

    We've copied this twice - is it worth putting into a method somewhere?

  7. +++ b/core/modules/comment/src/CommentLinkBuilder.php
    @@ -101,86 +118,116 @@ public function buildCommentedEntityLinks(FieldableEntityInterface $entity, arra
    +              $display = $this->entityManager->getStorage('entity_view_display')
    +                ->load($entity->getEntityTypeId() . '.' . $entity->bundle() . '.' . $view_mode);
    +              if (!$display) {
    +                $display = $this->entityManager->getStorage('entity_view_display')
    +                  ->load($entity->getEntityTypeId() . '.' . $entity->bundle() . '.default');
                   }

    here it is again, so 3 times = ideal candidate for a method somewhere

  8. +++ b/core/modules/comment/src/CommentLinkBuilder.php
    @@ -101,86 +118,116 @@ public function buildCommentedEntityLinks(FieldableEntityInterface $entity, arra
    +              // Show "add comment" link, unless the form is directly below it
    +              // (that is, if there are no comments displayed in between).
    +              if ($fallback_to_full
    +                  || !$formatter_options
    +                  || $formatter_options['settings']['form_location'] == CommentItemInterface::FORM_SEPARATE_PAGE
    +                  || (!empty($entity->get($field_name)->comment_count)
    +                      && $this->currentUser->hasPermission('access comments'))) {

    This so feels like a product and not a framework doesn't it.
    This doesn't belong in core in my opinion. None of this link stuff does. If anywhere it should be in standard, but not in comment.

Note to self: up to CommentManager

larowlan’s picture

Review part 2 - will upload screencast tomorrow.

So yes - this works - and has tests - but at what cost - I think that some time ago (during the Drupal 7 cycle?) that comment links lost their way and attempt to solve too many use-cases, many of them specific to a particular site/product. I'm voting that we wind a lot of the comment link features back - alternate formatters can answer a lot of these question 'Same page' formatter 'Different page' formatter. We're trying to solve every use case in core - but we should only be ensuring that sites working with Drupal can solve their particular use-cases with our APIs - thoughts?

Also @roderik++ for all the effort you've put in here.

  1. +++ b/core/modules/comment/src/CommentLinkBuilder.php
    @@ -9,16 +9,25 @@
    @@ -32,6 +41,13 @@ class CommentLinkBuilder implements CommentLinkBuilderInterface {

    Hate to see what the cyclic complexity of CommentLinkBuilder is with this patch, it was already off the charts. Again - I think we should consider dialing back a lot of this - even splitting it into contrib. Are these links really the 90% use-case? (nothing to do with this patch, other than it gets worse to deal with here)

  2. +++ b/core/modules/comment/src/CommentManager.php
    @@ -130,6 +131,8 @@ public function addDefaultField($entity_type, $bundle, $field_name = 'comment',
    +      $comment_type->save();

    Why is the save() call added? Can't we do this in the original creation/default config?

  3. +++ b/core/modules/comment/src/CommentViewBuilder.php
    @@ -354,13 +358,26 @@ public static function attachNewCommentsLinkMetadata(array $element, array $cont
    +    $page_number = 0;
    +    $display = \Drupal::entityManager()->getStorage('entity_view_display')
    +      ->load($entity->getEntityTypeId() . '.' . $entity->bundle() . '.full');
    +    if (!$display) {
    +      $display = \Drupal::entityManager()->getStorage('entity_view_display')
    +        ->load($entity->getEntityTypeId() . '.' . $entity->bundle() . '.default');
    +    }

    yep, another time, def time to put this somewhere

  4. +++ b/core/modules/comment/src/Controller/CommentController.php
    @@ -296,11 +299,27 @@ public function renderNewCommentsNodeLinks(Request $request) {
    +      $page_number = 0;
    +      $display = $this->entityManager->getStorage('entity_view_display')
    +        ->load('node.' . $node->bundle() . '.full');
    +      if (!$display) {
    +        $display = $this->entityManager->getStorage('entity_view_display')
    +          ->load('node.' . $node->bundle() . '.default');
    +      }

    And again. Can we load multiple instead of load and then use reset() - would that make it simpler?

  5. +++ b/core/modules/comment/src/Controller/CommentController.php
    @@ -296,11 +299,27 @@ public function renderNewCommentsNodeLinks(Request $request) {
    +        $page_number = $this->entityManager->getStorage('comment')

    Perhaps we put the juggling of formatter options, field definitions etc in a method on the CommentType? Just talking out loud here.

  6. +++ b/core/modules/comment/tests/src/Unit/CommentLinkBuilderTest.php
    @@ -123,7 +216,72 @@ public function testCommentLinkBuilder(NodeInterface $node, $context, $has_acces
    +    if ($view_mode == 'full') {
    +      $this->entityViewDisplay->expects($this->any())
    +        ->method('getComponent')
    +        ->willReturn(isset($form_location)
    +          ? array(
    +            'settings' => array(
    +              'form_location' => $form_location,
    +              // This only influences the page argument to the new-comments
    +              // url, which we're not testing, so return static value.
    +              'per_page' => 50
    +            )
    +          )
    +          : NULL);
    +      $this->entityStorage->expects($this->any())
    +        ->method('load')
    +        ->with($node->getEntityTypeId() . '.' . $node->bundle() . '.full')
    +        ->willReturn($this->entityViewDisplay);
    +    }
    +    else {
    +      // From the field formatter settings for the current display, the class
    +      // should be using 'form_location'.
    +      $this->entityViewDisplay->expects($this->any())
    +        ->method('getComponent')
    +        ->willReturn(isset($form_location)
    +          ? array(
    +            'settings' => array(
    +              'form_location' => $form_location,
    +              'per_page' => 50
    +            )
    +          )
    +          : NULL);
    +      // From the field formatter settings for the full display, the class
    +      // should be using 'form_location'.
    +      $this->entityViewDisplayFull->expects($this->any())
    +        ->method('getComponent')
    +        ->willReturn(isset($form_location_full)
    +          ? array(
    +            'settings' => array(
    +              'form_location' => $form_location_full,
    +              'per_page' => 50
    +            )
    +          )
    +          : NULL);
    +      $this->entityStorage->expects($this->any())
    +        ->method('load')
    +        ->willReturnMap(array(
    +          array(
    +            $node->getEntityTypeId() . '.' . $node->bundle() . '.full',
    +            $this->entityViewDisplayFull
    +          ),
    +          array(
    +            $node->getEntityTypeId() . '.' . $node->bundle() . '.' . $view_mode,
    +            $this->entityViewDisplay
    +          ),
    +        ));
    +    }
    +    // This setting influences the page argument to the 'new comments' url,
    +    // which we're not testing, so return static value.
    +    $this->commentType->expects($this->any())
    +      ->method('getThreadingMode')
    +      ->willReturn(CommentTypeInterface::THREADING_MODE_THREADED);
    +
    +    $context = array('view_mode' => $view_mode);

    This makes it clear how complex the link building is - anyone else agree it should get dropped/split into contrib?

  7. +++ b/core/profiles/standard/standard.install
    @@ -25,6 +27,15 @@ function standard_install() {
    +  $comment_type = CommentType::load('comment');
    +  $comment_type->setThreadingMode(CommentTypeInterface::THREADING_MODE_THREADED);

    Yeah this definitely should just go in the default config yml that provides that view mode.

  8. +++ b/core/profiles/standard/standard.install
    @@ -25,6 +27,15 @@ function standard_install() {
    +  foreach (array('search_result', 'search_index') as $view_mode) {
    +    $display = entity_get_display('node', 'article', $view_mode);
    +    $display->removeComponent('comment');
    +    $display->save();
    +  }

    I like this.

larowlan’s picture

To answer the cyclic-complexity - CommentLinkBuilderTest has 2020 combinations.

Yes two thousand and twenty that is not a typo.
Yeah it only takes 1.35 minutes to run because its decoupled and php unit but c'mon - comment module is doing too much - especially as this functionality is only used for nodes.

CRAP index is 37.5
Cyclic complexity is 37 (similar because we have good coverage).

Here's the code-coverage to verify we have most of it with this patch

The method buildCommentedEntityLinks() has an NPath complexity of 1045460. The configured NPath complexity threshold is 200.

larowlan’s picture

NPathComplexity

The NPath complexity of a method is the number of acyclic execution paths through that method. A threshold of 200 is generally considered the point where measures should be taken to reduce complexity.

Comment module, we need to talk

larowlan’s picture

Sorry to go off topic about links - have spoken with @alexpott and will open an issue to propose removing a lot of that logic

http://youtu.be/mj00GpaU52Y is the url to the manual testing screencast

larowlan’s picture

Actually this patch makes it worse - on HEAD

The method buildCommentedEntityLinks() has a Cyclomatic Complexity of 27.
The method buildCommentedEntityLinks() has an NPath complexity of 1204.

So its bad in HEAD, but worse with this patch.

larowlan’s picture

Also the phpunit combinations go from 580 in HEAD to 2020 here.

larowlan’s picture

jhodgdon’s picture

Should we postpone this issue on the "reduce complexity" issue then?

idebr’s picture

Status:Needs review» Needs work
Issue tags:+Needs reroll

Was going to give this a manual test, but it no longer applies :<

kerby70’s picture

StatusFileSize
new89.81 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch move_comment_field-1920044-86.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new9.05 KB

Attached is as much of a reroll as I could get through. A few method calls look like they have moved so it still needs work.

I did not find the right place for these changes.

diff --git a/core/modules/comment/src/CommentManager.php b/core/modules/comment/src/CommentManager.php
index 0094a01..c29f894 100644
--- a/core/modules/comment/src/CommentManager.php
+++ b/core/modules/comment/src/CommentManager.php
@@ -7,6 +7,7 @@

namespace Drupal\comment;

+use Drupal\comment\Plugin\Field\FieldFormatter\CommentDefaultFormatter;
use Drupal\comment\Plugin\Field\FieldType\CommentItemInterface;
use Drupal\Component\Utility\String;
use Drupal\Component\Utility\Unicode;
@@ -130,6 +131,8 @@ public function addDefaultField($entity_type, $bundle, $field_name = 'comment',
           '%entity_type' => $entity_type,
         )));
       }
+      $comment_type->setThreadingMode(CommentTypeInterface::THREADING_MODE_THREADED);
+      $comment_type->save();
     }
     else {
       // Silently create the comment-type for the calling code.
@@ -138,6 +141,7 @@ public function addDefaultField($entity_type, $bundle, $field_name = 'comment',
         'label' => Unicode::ucfirst($comment_type_id),
         'target_entity_type_id' => $entity_type,
         'description' => 'Default comment field',
+        'threading_mode' => CommentTypeInterface::THREADING_MODE_THREADED,
       ))->save();
     }
     // Make sure the field doesn't already exist.
@@ -198,6 +202,10 @@ public function addDefaultField($entity_type, $bundle, $field_name = 'comment',
           'label' => 'above',
           'type' => 'comment_default',
           'weight' => 20,
+          'settings' => array(
+            'per_page' => 50,
+            'form_location' => CommentItemInterface::FORM_BELOW,
+          )
         ))
         ->save();
       // The comment field should be hidden in all other view displays.

diff --git a/core/modules/forum/forum.install b/core/modules/forum/forum.install
index 248568b..6953124 100644
--- a/core/modules/forum/forum.install
+++ b/core/modules/forum/forum.install
@@ -5,10 +5,11 @@
  * Install, update, and uninstall functions for the Forum module.
  */

+use Drupal\comment\CommentTypeInterface;
+use Drupal\comment\Entity\CommentType;
use Drupal\comment\Plugin\Field\FieldType\CommentItemInterface;
use Drupal\field\Entity\FieldConfig;
use Drupal\field\Entity\FieldStorageConfig;
-use Drupal\comment\CommentManagerInterface;
use Drupal\taxonomy\Entity\Term;

/**
@@ -42,9 +43,9 @@ function forum_install() {
       Drupal::service('comment.manager')->addDefaultField('node', 'forum', 'comment_forum', CommentItemInterface::OPEN, 'comment_forum');

       // Add here because we don't have param in addDefaultField function.
-      $field = FieldConfig::loadByName('node', 'forum', 'comment_forum');
-      $field->settings['default_mode'] = CommentManagerInterface::COMMENT_MODE_FLAT;
-      $field->save();
+      $comment_type = CommentType::load('comment_forum');
+      $comment_type->setThreadingMode(CommentTypeInterface::THREADING_MODE_FLAT);
+      $comment_type->save();

       // Hide label for comment field.
       entity_get_display('node', 'forum', 'default')

diff --git a/core/profiles/standard/standard.install b/core/profiles/standard/standard.install
index c2b5582..87c4845 100644
--- a/core/profiles/standard/standard.install
+++ b/core/profiles/standard/standard.install
@@ -4,6 +4,8 @@
  * Install, update and uninstall functions for the standard installation profile.
  */

+use Drupal\comment\CommentTypeInterface;
+use Drupal\comment\Entity\CommentType;
use Drupal\comment\Plugin\Field\FieldType\CommentItemInterface;

/**
@@ -25,6 +27,15 @@ function standard_install() {

   // Add comment field to article node type.
   \Drupal::service('comment.manager')->addDefaultField('node', 'article', 'comment', CommentItemInterface::OPEN);
+  $comment_type = CommentType::load('comment');
+  $comment_type->setThreadingMode(CommentTypeInterface::THREADING_MODE_THREADED);
+  $comment_type->save();
+  // Set some display options for comments in search results.
+  foreach (array('search_result', 'search_index') as $view_mode) {
+    $display = entity_get_display('node', 'article', $view_mode);
+    $display->removeComponent('comment');
+    $display->save();
+  }

   // Hide the comment field in the rss view mode.
   entity_get_display('node', 'article', 'rss')

andypost’s picture

I'm sure we need to postpone that to find a way to investigate #81 and only then continue here,
#1548204: Remove user signature and move it to contrib and #2428509: Comment links weight should be managed by view display will simplify render also there's special #2002094: Improve performance of comment.html.twig that could be converted to meta

mgifford’s picture

Assigned:roderik» Unassigned

Just unassigning issues that haven't been developed for a bit in the D8 queue.

googletorp’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll

Status:Needs review» Needs work

The last submitted patch, 86: move_comment_field-1920044-86.patch, failed testing.

jhodgdon’s picture

Issue tags:+Needs reroll

restoring tag

googletorp’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new87.78 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Invalid PHP syntax in core/modules/comment/src/CommentLinkBuilder.php.
[ View ]

Ah sorry, didn't notice the rerolled patch needed rerolling.

Tried to do my best to reroll this, a lot of things has changed, but this can give us a starting point.

Status:Needs review» Needs work

The last submitted patch, 92: move_comment_field-1920044-92.patch, failed testing.

googletorp’s picture

Status:Needs work» Needs review
StatusFileSize
new87.5 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Invalid PHP syntax in core/modules/comment/src/CommentLinkBuilder.php.
[ View ]

Fixed merge issue from reroll in #92.

Status:Needs review» Needs work

The last submitted patch, 94: move_comment_field-1920044-94.patch, failed testing.

googletorp’s picture

Status:Needs work» Needs review
StatusFileSize
new87.18 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Invalid PHP syntax in core/modules/comment/tests/src/Unit/CommentLinkBuilderTest.php.
[ View ]

If at first you don't succeed :(

Status:Needs review» Needs work

The last submitted patch, 96: move_comment_field-1920044-96.patch, failed testing.

googletorp’s picture

Status:Needs work» Needs review
StatusFileSize
new87.55 KB
googletorp’s picture

googletorp’s picture

StatusFileSize
new87.55 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 92,611 pass(es), 2,125 fail(s), and 569 exception(s).
[ View ]

Trying to reload file to get it tested, something seems a bit of with test bot not testing patch form #98

Status:Needs review» Needs work

The last submitted patch, 100: move_comment_field-1920044-98.patch, failed testing.

googletorp’s picture

Status:Needs work» Needs review
StatusFileSize
new87.34 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 94,114 pass(es), 660 fail(s), and 295 exception(s).
[ View ]
new1.57 KB

Reduced a lot of the fails.

Got some issues with $node->rss_elements not sure if the issue is the test or the code.

Status:Needs review» Needs work

The last submitted patch, 102: move_comment_field-1920044-102.patch, failed testing.

larowlan’s picture

Given #2559833: Redirect comment form to current page, when form is in block I think we actually might need to make the redirect path configurable