Move comment field settings that relate to rendering to formatter options

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

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

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?

Closed as duplicate #1830966: Define comment_per_page_options as config

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}'

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?

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

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.?

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

exactly default value

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

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

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

#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?

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"

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.

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.

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

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?

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: Deprecate comment_uri() & #2113323: Rename Comment::permalink() 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

Forget to point to a duplicate issue #1901110-6: Improve the UX for comment bundle pages and comment field settings my initial approach for the subject

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

re-roll, pushed to github

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

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?

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.

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

Duplicate code still there, I now think I should

• nuke the new CommentLinkBuilder service ( :-/ )
• tie larowlan's fresh CommentLinksTest into CommentDefaultFormatter::buildLinks()
• port more changes from review #2318251-23: Make comment links functionality testable and convert CommentLinkTest to PHPUnit to CommentDefaultFormatter::buildLinks() because I see I forgot some.
• (then formulate opinion on #38-#42 if I that's crystallized)

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.

(coming up)

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.

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.

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.

silly little mistake, comments still pending when green.

finding individual issues to fix among the failures...

.

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

Comments and github push will still follow when green.

...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)

reroll. See #72;

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

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

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')

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

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

restoring tag

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.

Fixed merge issue from reroll in #92.

If at first you don't succeed :(

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

Reduced a lot of the fails.

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

This 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.

This could potentially be implemented in a minor with BC, so moving to 8.2.x.

Updating tag.