Implement generic entity link view field handlers

Can we get an actual issue summary? ... The patch looks interesting, but it would be good to know what problem it is supposed to be solving. :)

Ah thanks. That is kind of what I thought by reading the patch; good to confirm that this is the intention.

A few notes on the current patch:

a) The error here is real:

Fatal error: Uncaught exception 'LogicException' with message 'Missing phpDoc on Drupal\views\Tests\Handler\FieldEntityTest.' in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/src/TestDiscovery.php:394

This file in the patch has a pretty-much empty test in it with no docs header; test runner cannot handle this.

b) Generally I think this patch looks good! Making the link stuff standardized across entities seems like an excellent idea.

c) It also looks like you're doing something about consolidating bundle stuff? Such as in NodeViewsData:

-    $data['node_field_data']['type']['field']['id'] = 'node_type';

That may be missing from the issue summary? Or should that be a separate fix? Doesn't seem related to these links....

d)

+/**
+ * Field handler to present a bundle to the entity_bundle.
+ *
+ * @ingroup views_field_handlers
+ *
+ * @ViewsField("entity_view")
+ */
+class Entity extends FieldPluginBase {

The docs header is wrong, I think... what is this actually?

e)

+/**
+ * Field handler to present a link to the entity_link.
+ *
+ * @ingroup views_field_handlers
+ *
+ * @ViewsField("entity_link")
+ */
+class EntityLink extends FieldPluginBase {

The first line here... what is this supposed to mean? I don't understand it.

f) Looks like a few tests still need to be filled in?

It seems like maybe on this issue, we should limit it to making a unified method for entity view/edit/delete/translate links (is that the full list?). And if there are other nice things in entity_views, we can get them on other issues. That should keep things manageable?

I guess my other question after looking at this patch is whether we could maybe get rid of some of the specific handler plugins, like aggregator/src/Plugin/views/field/TitleLink.php and ... are the specific types of links needed, like DeleteLink and EditLink, or could they be taken care of with one master EntityLink class that has some ... what's it called, not configuration but when you specify the options for the handler in the hook_views_data()? Might make the patch even smaller.

Is this same as #2363811: Add a generic entity bundle field?

it's clear that contrib (ex-core in this case) needs this, see #2409887: Views integration

First attempt at reroll, tried to remove the bundle and view stuff, removed the empty test classes.

Config schema fixes.

More schema fixes.

+100000000! This is great (plus or minus a few test failures).

Looks awesome, even though I'm probably not the right person for a thorough review.

Just noticed :

+++ b/core/modules/views/src/Plugin/views/field/EntityLink.php
@@ -0,0 +1,121 @@
+  protected function renderLink($entity, ResultRow $values) {
+    if ($entity->access('view')) {

+++ b/core/modules/views/src/Plugin/views/field/EntityLinkDelete.php
@@ -0,0 +1,46 @@
+  protected function renderLink($entity, ResultRow $values) {
+    // Ensure user has access to delete this node.
+    if ($entity && $entity->access('delete')) {

+++ b/core/modules/views/src/Plugin/views/field/EntityLinkEdit.php
@@ -0,0 +1,46 @@
+  protected function renderLink($entity, ResultRow $values) {
+    // Ensure user has access to delete this node.
+    if ($entity && $entity->access('update')) {

So does renderLink() need to check the $entity or not ?

Some test fixes, tricky stuff.

Related #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency

I related #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality to this issue as well. The summary on that issue lists all of the specific field handler plugins in Core that we'd like to standardize and get rid of if possible, several of which are links. For instance, in the Node section there are:
node_link
node_link_edit
node_link_delete
node_revision_link
node_revision_link_revert
node_revision_link_delete

User, Comment, and Taxonomy have additional link fields, and some of the others have things similar to the generic "node", which prints the node title and allows you to make it a link (which #2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency is taking out anyway in favor of using the generic "field" handler).

There is also content_translation_link, which is added to most entities.

So... It looks like the latest patch here doesn't actually get rid of most of the handler classes (yet), and I don't think it covers all of the cases above, but it looks like a good start. One idea would be to reduce this patch further by having it create the infrastructure for doing this, and actually only remove the specific handlers for one entity (like Node), and then do the others in follow-up patches, rather than trying to do all of them at once? Since we have that other meta issue now, we won't lose track of the fact that not everything was converted to use the new system.

#2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality is now a critical meta-issue.

On that issue, we need to convert a lot of entity link views pseudo-fields to use some kind of entity-aware rendering in Views rather than one-off plugins. This issue would presumably solve the problem, and due to the number of entities and pseudo-fields that are depending on this, I think this issue is at least Major if not Critical.

Start with a reroll. There have been quite some changes in core since the last patch.

conflict fail, resolving conflict in YAML files is HORRIBLE :)

Yeah, there are certain things that we will still need specialist field implementations for, like most of these, but the entity system is such that we should be able to solve most cases generically.

So... I took a look at this patch.

A few comments/concerns:

a) The aggregator stuff:
- The aggregator title field plugin is going away on #2456701: Replace aggregator_title_link Views formatter with Field API formatter, which is about 99% RTBC and should be completed today, so all the Aggregator code in the patch is not necessary.

b) But anyway, it's kind of a different beast from the others, so I think it wouldn't have been good to include it here... I think the point of this issue is to replace the generic "Make a link to X" fields, the ones that allow you to define the link text in Views (with some plain text or presumably a views-token-replacement-thing). So for instance, the "make a generic link to the node" field just makes a link to node/12345 and you define the link text. Every other plain-text field (not only Title but any now) also allows you to turn it into a link to the entity, but we have these special fields for making plain-old links to view, edit, etc.

So to me, fields like Title (or in this case Aggregator Item Title), are not the same, because instead of putting in some plain-text for the link text, they are instead getting the link text from a specific field, and then allowing you to make it into a link, right? To me, that's different, and I think this patch should just be dealing with those plain-text-make-a-link pseudo-fields, not the this-is-a-field-turn-it-into-a-link... besides which all text fields already have this ability now in Views.

Hope that made sense...

c) I don't think we actually want to turn on the links for all entities in all cases that the entity defines in its canonical link list. So I think maybe we just need a generic method on EntityViewsData that will create such a link field, and then let each specific entity ViewsData class call it to get those methods if it makes sense. ?? Not sure about this, but right now, for instance, Node, Comment, and User have these links, and Taxonomy has some, but File doesn't and Aggregator doesn't... But then again maybe it's good to turn them on generically? OK I think I'm undecided on this, but probably agree with turning it on generically and standardizing all entities. ;)

d) Is it going to be possible to do something about things that aren't canonical entity-defined links? For instance the Node views data defines links for viewing/reverting revisions. These are just defined as generic routes in node.routing.yml, and they're not in the links section of the entity annotation on the Node class. Can anything be done for them?

Working on this at Drupal Dev Days...

Sitting with kgoel... here are some questions / comments [including the open questions from #35]:

1. Do we want the call to EntityViewsData::addEntityLinks() to be done by default for all entities in EntityViewsData::getViewsData(), or should we (at least initially) just make the method exist, and let Node, Comment, User, and Taxonomy call it in their own Entity ViewsData classes, to reproduce what is in Core now?

2. Here is a list of the existing entity link pseudo-views-fields and plugins. Can we replace all of them in this patch?

- plugin node_link (pseudo-field: view_node)
- node_link_edit (pseudo-field: edit_node)
- node_link_delete (pseudo-field: delete_node)
- node_path (pseudo-field: path)
- node_revision_link (pseudo-field: link_to_revision on node_revision)
- node_revision_link_revert (pseudo-field: revert_revision on node_revision)
- node_revision_link_delete (pseudo-field: delete_revision on node_revision)

- user_link_edit (pseudo-field: UserFieldData - edit_node, which despite the name is actually a link to edit the user)
- user_link_cancel (pseudo-field: UserFieldData - cancel_node, which despite the name is actually a link to cancel the user)

- comment_link: Used in CommentViewsData for pseudo-field view_comment
- comment_link_edit: Used in CommentViewsData for pseudo-field edit_comment
- comment_link_delete: Used in CommentViewsData for pseudo-field delete_comment
- comment_link_approve: Used in CommentViewsData for pseudo-field approve_comment
- comment_link_reply: Used in CommentViewsData for pseudo-field replyto_comment

- term_edit_link - field edit_term

- content_translation_link (Used in pseudo-fields called translation_link in: NodeViewsData, UserViewsData, BlockContentViewsData, CommentViewsData, TermViewsData)

Note: Other entities do not currently have these link fields: File, AggregatorItem, AggregatorFeed, BlockContent

3. Since all text/string fields on Entities (such as Title) now have the ability to link to the entity (canonical link), do you agree that converting title and similar fields is out of scope for this issue? I think we should limit this issue to the plain "link" fields that let you define the link text, and exclude fields like "title with ability to make link" from this one.

4. Note: In the list in item 2 here, some of the link fields are not actually part of the entity-defined link list. For example, on Node, the revision view, delete, and revert links are not part of the Node link annotation, which looks like this:

 *   links = {
 *     "canonical" = "/node/{node}",
 *     "delete-form" = "/node/{node}/delete",
 *     "edit-form" = "/node/{node}/edit",
 *     "version-history" = "/node/{node}/revisions",
 *   }

The revision links are just regular routes, defined in node.routing.yml, like this:

node.revision_revert_confirm:
  path: '/node/{node}/revisions/{node_revision}/revert'
...

Can we handle them in this issue, or is that out of scope?

Proposed answers to the questions in #37:

1. Go ahead and do this for all entities. Will add some links, but that is OK.

2. See item 1 and 4.

3. Limit to the fields listed in 2 or a subset; others out of scope.

4. Limit to fields defined in entity annotation, not routing.yml files like revision revert/delete for node.

Given these answers, the patch kgoel is about to post will contain:
- A reroll of the previous patch
- Leaving out aggregator stuff [out of scope text fields and covered elsewhere anyway]
- Leaving out the changes to the Drupal\node\Plugin\views\field\Node class [out of scope text fields]
- Added content translation to the mix. Used existing content translation plugin, but edited to subclass the new EntityLink class instead of what it did before ==> no new schema or views changes needed since ID is same and so are options (phew!)

Previous patch didn't exactly apply; interdiff difficult; please excuse the mess. ;)

Need to add couple lines for content_translation in EntityViewsDataTest.php

@kgoel interdiff please. :)

+++ b/core/modules/views/tests/src/Unit/EntityViewsDataTest.php
@@ -712,6 +712,33 @@ protected function assertField($data, $field_name) {
+    $this->assertEquals('entity_link', $data['entity_test']['view_entity_test']['field']['id']);
+    $this->assertEquals('entity_link_edit', $data['entity_test']['edit_entity_test']['field']['id']);
+    $this->assertEquals('entity_link_delete', $data['entity_test']['delete_entity_test']['field']['id']);

Let's add assert for translation link.

@jibran - Please see jhodgdon's comment...

Previous patch didn't exactly apply; interdiff difficult; please excuse the mess. ;)

Let's add assert for translation link.

Please see my comment- https://www.drupal.org/node/2322949#comment-9838445

Those line were not there before when I was last here. j/k :P
Don't mind me carry on :)

This patch is looking like it's pretty close, assuming the bot agrees. One change crept in by mistake:

+++ b/core/modules/node/src/Plugin/views/field/Node.php
@@ -37,9 +37,6 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
-  /**
-   * {@inheritdoc}
-   */
   protected function defineOptions() {
     $options = parent::defineOptions();

This shouldn't have been removed in the patch.

Several of the link plugins that are mentioned in comment #37 item 2 are not actually removed or replaced in this patch... so I think there is a bit more work to do.

Closer!

Looks like this still needs some work though:

1. +++ b/core/modules/comment/config/schema/comment.views.schema.yml
   @@ -8,49 +8,10 @@ views.field.comment_depth:
   -views.field.comment_entity_link:
   -  type: views_field
   -  label: 'Comment link'
   -  mapping:
   

   (and following lines) - I think we need to keep this one. This is some kind of weird link thing that shows links to the node the comment is on or something.

2. +++ b/core/modules/comment/config/schema/comment.views.schema.yml
   @@ -8,49 +8,10 @@ views.field.comment_depth:
   -views.field.comment_link_approve:
   -  type: views.field.comment_link
   -  label: 'Comment approve link'
   -
   

   We'll need this: this class (and the reply plugin below) are replaced with new code, but still exist as special plugins.

3. +++ b/core/modules/comment/config/schema/comment.views.schema.yml
   @@ -8,49 +8,10 @@ views.field.comment_depth:
   -views.field.comment_link_reply:
   -  type: views.field.comment_link
   -  label: 'Comment reply link'
   -
   

   See comment above.

4. +++ b/core/modules/comment/src/CommentViewsData.php
   @@ -102,46 +102,6 @@ public function getViewsData() {
   -    $data['comment']['approve_comment'] = array(
   -      'field' => array(
   -        'title' => t('Link to approve comment'),
   -        'help' => t('Provide a simple link to approve the comment.'),
   -        'id' => 'comment_link_approve',
   -      ),
   -    );
   -
   -    $data['comment']['replyto_comment'] = array(
   -      'field' => array(
   -        'title' => t('Link to reply-to comment'),
   -        'help' => t('Provide a simple link to reply to the comment.'),
   -        'id' => 'comment_link_reply',
   -      ),
   -    );
   -
   

   Still need these, see above.

5. +++ b/core/modules/node/config/schema/node.views.schema.yml
   @@ -94,37 +94,13 @@ views.argument_validator.node:
    views.field.node:
   -  type: views_field
   +  type: views.field.entity_link
      label: 'Node'
   

   We're not doing anything to this plugin any more, so just leave this as it was.

6. +++ b/core/modules/taxonomy/src/TermViewsData.php
   --- a/core/modules/user/config/schema/user.views.schema.yml
   +++ b/core/modules/user/config/schema/user.views.schema.yml
   

   Need to also get rid of the section for the now-deleted views.field.user_link too.

7. +++ b/core/modules/views/config/schema/views.field.schema.yml
   @@ -171,11 +171,22 @@ views.field.url:
    views.field.language:
      type: views_field
      label: 'Language'
   +
   +views.field.entity_link:
   +  type: views_field
   +  label: 'Entity link'
      mapping:
        native_language:
          type: boolean
          label: 'Display in native language'
   +    text:
   +      type: label
   +      label: 'Text to display'
    
   

   This is garbled. It looks like in the reroll something happened? There's a mix of stuff from the language field and the entity link field here....

8. +++ b/core/modules/views/config/schema/views.field.schema.yml
   @@ -171,11 +171,22 @@ views.field.url:
   +views.field.entity_link_delete:
   +  type: views.field.entity_link
   +  label: 'Entity delete link'
   +  mapping:
    views.field.bulk_form:
   

   This is also messed up...

   ok this schema file just needs to be redone.

9. +++ b/core/modules/views/src/Plugin/views/field/EntityLink.php
   @@ -0,0 +1,120 @@
   +    // @FIXME
   +    // $this->additional_fields['langcode'] = ['table' => 'node_field_data', 'field' => 'langcode'];
   +  }
   

   Hm... something missing here?

I am not sure about #9, need to look into that more.

OK, I think this is all good now except for the @fixme comment and maybe some test fails.

Some class names have changed (Link became EntityLink). Rerolled.

Should pass better: type-hinting renderLink on EntityInterface on other classes exteding EntityLink.

Extra hinting on NodeNewComment.

Also renamed $data to $entity since it implements EntityInterface: this is better for DX.

Gaaah, missed interdiff again.

Manually tested and I do see view, edit and delete links on entities page.

This is blocking #2450897: Cache Field views row output, so promoting to critical, I'll try to complete the work here, since Daniel believes this is almost ready.

Here is a quick review:

1. +++ b/core/modules/views/config/schema/views.field.schema.yml
   @@ -223,6 +240,7 @@ views.field.field:
          type: label
          label: 'Separator'
   +      label: 'Text to display'
   

   Is this correct? Does not look so.

2. +++ b/core/modules/views/src/EntityViewsData.php
   @@ -230,6 +232,53 @@ public function getViewsData() {
   +    if ($this->entityType->hasLinkTemplate('drupal:content-translation-overview')) {
   +      $data['translation_link'] = [
   +        'field' => [
   +          'title' => $this->t('Link to translate @entity_type_label', $t_arguments),
   +          'help' => $this->t('Provide a translation link to the @entity_type_label.', $t_arguments),
   +          'id' => 'content_translation_link',
   +        ],
   +      ];
   +    }
   +  }
   

   This should be added by the Content Translation module.

3. +++ b/core/modules/views/src/Plugin/views/field/EntityLink.php
   @@ -0,0 +1,111 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function init(ViewExecutable $view, DisplayPluginBase $display, array &$options = NULL) {
   +    parent::init($view, $display, $options);
   +  }
   +
   ...
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function defineOptions() {
   +    $options = parent::defineOptions();
   +    return $options;
   +  }
   +
   

   Looks like these methods are redundant...

More accurate title

This merges the work I did in #2450897: Cache Field views row output, basically it provides automatic route access checking.

#81.2 still to be addressed. Let's see how many things I broke.

This should fix the failing tests.

This implements #81.2, working on some additional test coverage now.

Some fixes/clean-ups...

This adds a couple of small fixes and the missing test coverage.

Edit: I don't know why but the patch appears only as attachment in the issue summary.

Attaching again, just in case

Except what Daniel found, this would be RTBC for me.

This should address #92. I was performing some manual testing and I noticed revision links are broken in HEAD, I implemented a quick fix, but deferred providing a proper fix and test coverage to #1863898: Add test coverage for Views revision link handlers.

1: That's the only class where doing what you suggest makes sense, for consistency with the others I'd keep it as-is.
6: There's a @todo (see the PHP doc) to add the current user PluginBase, so I'd keep it as-is for now.

Green, back to RTBC

1. +++ b/core/modules/comment/src/Tests/CommentAnonymousTest.php
   @@ -138,9 +138,7 @@ function testAnonymous() {
   -    $this->assertText('You are not authorized to post comments', 'Error attempting to post comment.');
   -    $this->assertNoFieldByName('subject[0][value]', '', 'Subject field not found.');
   -    $this->assertNoFieldByName('comment_body[0][value]', '', 'Comment field not found.');
   +    $this->assertResponse(403, 'Error attempting to post comment.');
   
   +++ b/core/modules/comment/src/Tests/CommentNonNodeTest.php
   @@ -344,9 +344,7 @@ function testCommentFunctionality() {
   -    $this->assertText('You are not authorized to post comments', 'Error attempting to post comment.');
   -    $this->assertNoFieldByName('subject[0][value]', '', 'Subject field not found.');
   -    $this->assertNoFieldByName('comment_body[0][value]', '', 'Comment field not found.');
   +    $this->assertResponse(403, 'Error attempting to post comment.');
   

   So are we going to leave the post comments check in CommentController::getReplyForm()? And if so should we be testing it?

2. +++ b/core/modules/views/src/Plugin/views/field/EntityLinkDelete.php
   @@ -0,0 +1,44 @@
   +use Drupal\Core\Entity\EntityInterface;
   

   Not used.

3. Looks like a change record will be necessary.

This reverts the changes to the comment reply access control to avoid regressions, as I don't completely understand the current logic and I suspect there might be something I'm missing. Moreover those changes are a bit out of scope here and can be easily addressed as a follow-up.

Interdiff is not in last patch - NW

The reason behind reply form to check access within controller is to Give user ability to login and back to reply form, because visitors could came to the replay form via email notifications and getting 403 does not tell exactly what's up...

+++ b/core/modules/comment/comment.routing.yml
@@ -57,7 +57,7 @@ comment.reply:
-    _access: 'TRUE'
+    _permission: 'post comments'

it's not reverted

@larowlan found these two related issues:

#56942: Comment cids should belong to the associated node in comment/reply
#50835: Reply to non existent comments

I think at least the check introduced in the first one was lost in translation. We should probably open a follow-up to verify the new code is still addressing the original use cases and improve it to just use access callback where needed.

Sorry, forgot to commit. This time for reals.

https://www.drupal.org/node/2483117

This looks very close!

1. +++ b/core/modules/contact/src/Plugin/views/field/ContactLink.php
   @@ -101,26 +40,25 @@ public function access(AccountInterface $account) {
   +    // Check access when we pull up the user account so we know if the user has
   +    // made the contact page available.
   

   The description makes sense, but doesn't explain where that is then happening.

   Could use an @see.

2. +++ b/core/modules/views/src/Plugin/views/field/EntityLink.php
   @@ -0,0 +1,53 @@
   + * Contains \Drupal\views\Plugin\views\field\EntityLinkBase.
   ...
   +class EntityLink extends LinkBase {
   

   Docs mismatch.

3. +++ b/core/modules/views/src/Plugin/views/field/LinkBase.php
   @@ -0,0 +1,197 @@
   + * Contains \Drupal\views\Plugin\views\field\EntityLinkBase.
   ...
   +abstract class LinkBase extends FieldPluginBase {
   ...
   +   * Constructs a ContactLink object.
   

   More docs mismatches.

4. +++ b/core/modules/views/src/Plugin/views/field/LinkBase.php
   @@ -0,0 +1,197 @@
   +  protected function currentUser() {
   

   Why not inject, this, like AccessManager?

   Beecause the todo will move it to PluginBase, and so injecting it here would cause more work elsewhere?

Why not inject, this, like AccessManager?
Beecause the todo will move it to PluginBase, and so injecting it here would cause more work elsewhere?

Yep, and it wouldn't just be more work, it'd also be an API break :)

And this...

Thanks!

Checked that #103 is also addressed. Back to RTBC per #102.

+++ b/core/modules/contact/src/Plugin/views/field/ContactLink.php
@@ -101,26 +40,26 @@ public function access(AccountInterface $account) {
+    // Check access when we pull up the user account so we know if the user has
+    // made the contact page available. See contact_form_user_form_alter() and
+    // contact_user_profile_form_submit().

Don't really understand the comment. When do we 'pull up the user account'? Was the comment just moved without updating it?

Struggled to find much else though, this really cleans a lot up.

That comment was originally in the ::renderLink() method, but you are right that now it makes no sense, it's only confusing, removed it. I also removed the ::access() method as its parent implementation is more correct.

RTBC, looks great!

Committed/pushed to 8.0.x, thanks!

Yay, thanks!

Published the CR

Retroactively tagging, as this was blocking #2450897: Cache Field views row output.

Closed #2035587: Move Views Translation link definitions from specific entity modules to Content translation module as a duplicate adding credit