Node and Comment ops links should be display components (which can be disabled)

We're hoping to move comment links to a formatter. But plus one for general approach.

If you can also apply this to Comment's links, then I think this patch is good to go. #2 sounds great, but is further away. This is simple and at least ensures sanity, for e.g. #2274975: Display/hide links on Rendered Entity formatter very broken.

Please push this forward :)

More accurate title.

Based on profiling that I did today, if you build a view of nodes without links, then we currently spend ~10% of the page request rendering/processing links that we then don't display, which is stupid.

Also, it would be a nice and clean solution for the broken entity reference links setting which has multiple issues open right now: #2275667: Reference field display links option ignored and #2274975: Display/hide links on Rendered Entity formatter very broken.

Apart from adding it for comments as well, there's problem that right now, this would allow to move the component around on the page which is a lie because the location is hardcoded right now. We might get away with a description that says "If enabled, will always be shown at the bottom" or something :p

Also, with this, the separate settings in entity reference and views would be bogus and we could possibly just throw them out here?

Based on profiling that I did today, if you build a view of nodes without links, then we currently spend ~10% of the page request rendering/processing links that we then don't display, which is stupid.

To clarify: this happens only when you hide (don't print) the links in the theme layer.

Also, with this, the separate settings in entity reference and views would be bogus and we could possibly just throw them out here?

Yes!

Removed the setting from reference field formatter, added real entity id as the array key. Testbot will not like it as I have not touched the tests yet.

- Removed the "Display links" option from views settings
- Fixed my wrong assumption from NodeViewBuilder::buildComponents()

You still need to modify Node::baseFieldDefinitions() :) (And Comment::baseFieldDefinitions() also.)

Nope, *extra* fields have nothing to do with base field definitions, they're extra.

The main problem that's remaining is that it doesn't allow you to set the weight/order. And I see two solutions for that:

- Add the description as mentioned above, like "This will only affect if the links are shown or not and not their position."
- Kill the manual override of the links in node templates, the main difference would be that it would be inside the content diff and no longer below it. We should also make sure that by default, the links stay last when you add new fields and so on.

Right! :(

Sorry for the misdirection!

Just a theoretical question - does it matter that much to have the links at the bottom at any case? Why it cannot behave as any other display field. We display them with weight 100 by default, and let's leave it for the user to decide where to keep the links.

That's exactly the question, and I can't answer it.. the problem is the extra < div > that's around everything else but not links right now. So we need some frontend people to chime in and say if they see a problem here...

Another very interesting side effect that this is is that comments are now rendered as a field too, so they are *above* the links, and there is no way to change that without manually changing the template.

This issue would allow us to move the links above the comment field again. So it's actually not just "make sure it's always last"...

There's a 2 related issues:
1) removes "extra div" (#15) #1962846: Use field instance name for header of comment list, drop comment-wrapper template
2) general approach for out-of-bound components #1875974: Abstract 'component type' specific code out of EntityDisplay

+1 to make it a field. Links are strictly already rendered inside content

  <div class="node__content clearfix {{ content_attributes.class }}"{{ content_attributes|without('class') }}>
    {{ content|without('comment', 'links') }}
  </div>

  {% if content.links %}
    <div class="node__links">
      {{ content.links }}
    </div>
  {% endif %}

  {{ content.comment }}

Yeah, I was mostly worried about the UI problem that the links will be there but by default (in bartik) won't respect the weight only hidden or not.

But as you said, this is the same already for comments, so I don't think this is blocking anything after all :)

Re-rolled and updated default views. Still needs tests.

+++ b/core/modules/node/templates/node.html.twig
@@ -102,9 +102,13 @@
+<<<<<<< ours
...
+=======
...
+>>>>>>> theirs

wrong merge

Fixed merge, working on tests.

Rerolled last patch to current 8.0.x.

Applied the same pattern to comments, updating default views, thought I did that already.

Ups, copy & paste fail.

Much fail. such stupid.

Looks good to me.

I think we still need tests? No?

A tests to turn on/off the components makes sense, otoh some tests form comments are going to change to phpunit in #2318251: Make comment links functionality testable and convert CommentLinkTest to PHPUnit

Applied template to summary.

An excellent way to test if this works would be to update the default config for the Search-related view modes so that they do not show the comment links, and then take out the kludgy code in search-related code that was trying to hide these links... I'd hope that this would fix #1113832: [PP-1] Comment module renders "reply" and other links in search index/results, which has a "steps to reproduce" which could probably be turned into a test.

I should at least see if this patch is sufficient for the Search use case...

Never mind... berdir pinged me in IRC...

So for this to work for the Search use case, we would need to be able to specify, on a Node view mode, what view mode for Comments to use when building the Node page.

Apparently this patch doesn't allow for that. Which seems like kind of a hole in the logic... but anyway this patch won't directly help us with the Search problem. Yet.

Added tests, and in the process found and removed some left-behind code around the removed views option. With help from @Berdir.

Also created a draft change notice at https://www.drupal.org/node/2324707

+++ b/core/modules/node/src/Tests/NodeLinksTest.php
@@ -0,0 +1,46 @@
+    $this->assertNoLink('Read more');
+  }
+} ¶

trailing whitespace here. And there should be an empty line between the closing } of the method and class.

Arla: when you create a change notice, you need to reference the issue that it is for... You probably did this but I think it maybe gets lost when you preview the change node.

Oh also the project on that change notice is set to "Node" and not "Drupal Core".

Thanks @jhodgdon, corrected it! Working on tests. The CommentLinksTest passes on my system, though...

Thanks Arla! It looks like 3 tests are failing... not just CommentLinksTest.

Updated the tests.

@Berdir showed me how those tests can be changed more reasonably (and cleaner!), so here's another update. In RowPluginTest, we now just don't check whether links are output, because it's no longer a feature of Views.

The interdiff is relative to #38 because that is clearer.

This tests passes locally for me - retesting

just a thought - probably off - but maybe something to do with testbot using a sub-dir?

When I run locally with run-tests.sh I get 6 fails. I'm not sure if I'm doing it right?

php core/scripts/run-tests.sh --class --verbose Drupal\\comment\\Tests\\CommentLinksTest

and the fails are:

Fail      Other      CommentLinksTest.  103 Drupal\comment\Tests\CommentLinksTe
    Link with label 1 comment found.
Fail      Other      CommentLinksTest.  105 Drupal\comment\Tests\CommentLinksTe
    Link with label Add new comment found.
Fail      Other      CommentLinksTest.  105 Drupal\comment\Tests\CommentLinksTe
    Link with label Add new comment found.
Fail      Other      CommentLinksTest.  118 Drupal\comment\Tests\CommentLinksTe
    "qnuUjYjt" found
Fail      Other      CommentLinksTest.  119 Drupal\comment\Tests\CommentLinksTe
    Link with label Reply found.
Fail      Other      CommentLinksTest.  126 Drupal\comment\Tests\CommentLinksTe
    "qnuUjYjt" found

Green :)
I use phing or the UI to run my tests.
See build.XML in github.com/nickschuch/vd8 for the commands phing uses

Nice.

We removed all the links: true|false from views defintions. Now links are showing in the default feed and the taxonomy feed (as <comments>http://d8/node/1#comments</comments>). Do we want to make them hidden by default again? Or, since there was no test for it, is the behaviour undefined? (Should it be?)

Yes, I guess we should hide them there and provide the configuration for that, and some tests would be great.

@Arla there's 2 issues with comments #2113323: Rename Comment::permalink() to not be ambiguous with ::uri() & #2010202: Deprecate comment_uri()

It seems that the Display links option on RSS rows did not have any effect before this patch. The tests (CommentRssTest and RssTest) test only one behaviour – ironically enough that links are displayed when Display links is set to false. To test, checkout 8.0.x and visit rss.xml, then change the Frontpage view and check Display links, (rebuild cache,) visit rss.xml, and note that the output is identical to before.

The problem is that node\Plugin\views\row\Rss (and probably comment\Plugin\views\row\Rss) handle the links in the wrong way. Rather than fixing that separately, I guess we should just extend the tests and implement within this issue.

Did some exploring with @Berdir about node links in RSS. hook_node_link_alter() is implemented by comment, book and statistics. They all do a hard-coded check to make exceptions for $view_mode == 'rss'. Book and statistics simply don't output anything for 'rss', comment does it by adding a <comments> element.

1. The <comments> element is then quite far from being a node link. For example it will not be affected by the weight of the Links field. The module merely utilises the hook to decide whether to add the element or not. Should the element be output by another, more logically chosen mechanism?
2. We could remove the hard-coded $view_mode checks in book and statistics, because that is now configurable. But since it is not configurable to show some links and some not, that would disallow for e.g. the case of showing comment links but not book and statistics links. Should we remove the checks?

1) now that comment is a field, could we move comment rss output to a formatter and make that the formatter for the rss view mode.
We could use hook_entity_view_display_alter to force this formatter, just like node does some brute-forcing of display settings for search view mode. If this is possible, we could split that into a separate issue.
2) yeah I think we're stuck with the checks

2) We could split each into a distinct component links__comment links__book links__statistics etc - would require some sort of discovery mechanism of course - ie the notion of EntityLinkPlugins

So this is 'in an ideal world solution' from my point of view

/**
 * @file
 * Contains \Drupal\Core\Entity\Display\EntityLinkPluginInterface.
 */

namespace Drupal\Core\Entity\Display;

use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
use Drupal\entity\EntityViewModeInterface;

/**
 * Defines an interface for entity link plugins.
 */
interface EntityLinkPluginInterface extends ContainerFactoryPluginInterface {

  /**
   * Determines if this plugin provides links for the given entity type ID.
   *
   * @param string $entity_type_id
   *   Entity Type ID under consideration.
   *
   * @return bool
   *   TRUE if the plugin provides links for the given entity type ID.
   */
  public function applies($entity_type_id);

  /**
   * Build links for a given array of entities.
   *
   * @param \Drupal\entity\EntityViewModeInterface $view_mode
   *   The view mode the entities are to be output in.
   * @param \Drupal\Core\Entity\EntityInterface[] $entities
   *   Entities being built.
   *
   * @return array
   *   A valid render array containing the links.
   */
  public function buildLinks(EntityViewModeInterface $view_mode, $entities);

}

Then in (eg) node_entity_extra_field_info (note psuedo code, untested):

  $extra = array();
  $manager = \Drupal::service('plugin.manager.entity_links');
  foreach ($manager->getDefinitions() as $id => $definition) {
    $instance = $manager->createInstance($id, $definition);
    if ($instance->applies('node')) {
      foreach (node_get_types() as $bundle) {
        $extra['node'][$bundle]['display']['links__' . $id] = array(
          // Link display definition here..
        );
      }
    }
  }

Then again in NodeViewBuilder::buildLinks() we use the plugin manager again, this time to call EntityLinkPluginInterface::buildLinks.

Then we remove hook_node_links, hook_node_links_alter etc in favour of plugins.

actually, we'd retain the alter hook, but for actual alters, not for 'adding'

1) I've thought about a formatter too, I'd suggest we create a follow-up issue for that. Then we simply don't touch that here and it is still displayed as long as you have the links extra field set to visible.

2) Yeah, maybe, but that's IMHO going much further than we want to go in this issue and it has problems on it's own.. having that IMHO doesn't make much sense without having some notiion of grouping (which would be great to have but we don't), I don't know what would happen if you decide to move them around and so on.

IMHo, on those links on RSS, it's either way not a regression compared to what we have now. If we keep the hiding, then they will simply never be displayed on the rss output, just like now. And if we remove that logic, then we can still configure it so that they don't show up by default, just like now.

There's issue already, added to related #1920044: Move comment field settings that relate to rendering to formatter options
The same troubles are described in #1113832: [PP-1] Comment module renders "reply" and other links in search index/results

The main trouble here that we can't use disabled view mode (rss, search*) to render entity #2324719-2: Node indexing - should use view mode for comments, not hook

#67/andypost: So maybe we need to fix that, so that you can use a disabled view mode to render an entity, and it will fall back? Why will it not do that anyway, and shouldn't it? It seems like something we need.

I don't understand the last few comments, how is that related to this issue? I don't know what you mean with disabled view modes, the only thing that exists are view modes that have no corresponding display config entity, which means that it falls back to the default. Which is perfectly OK.

IMHO, the current patch is a large step forward and can be committed like this. Changing the comment behavior can be a follow-up and I'm not completey sure that a formatter is the right step, because it could be something that's additional, what if you want to display that tag *and* the comments in an rss feed?

Same for the second point, it's not optimal that those links are still hardcoded and hidden on the rss view mode, but that's the same as it is now.

@Berdir yep, that's out of scope, take a look at #2324719-2: Node indexing - should use view mode for comments, not hook - that means disabled entity displays for view modes

RTBC then?

Re-roll, context conflict in EntityReferenceEntityFormatter.

Committed to 8.0.x.

I think we need to tidy up our definition of a "task". My personal opinion is that it is a feature because it actually changes the behavior and affects contributed modules. Applying a pattern to convert something from the old way to the new way can be a feature -- and isn't always a task. At what point in the release cycle do we stop applying patterns?

Great!

On task/feature, agreed, I made this a (major) task for three reasons:

a) It solves known bugs in entity_reference, I just marked #2274975: Display/hide links on Rendered Entity formatter very broken as a duplicate
b) It unifies two different features into one, so now it works the same way in views and entity references, and also everywhere else where entities are displayed.
b) It's also performance relevant, if you used that checkbox in a view of nodes for example, then it still built them (including post render callbacks as they are personal), now we save this. Based on my testing, on overview pages with a lot of nodes without links, that was ~10% request or so.

Also published the change record.

Fix media tag.