[regression] Reimplement Entity links Display

hook_node_links_alter()!? Wow. It's like being back on D6 ;)

Looks like it. I found docs and a change record and everything!

That does mean though that only nodes and comments have links. I think in D7 you could technically stick a links item in the render array for an entity of any type, though I don't remember if anything decent happens...

Might it be an idea to split off the contextual links stuff into a separate issue?

Hi, I'd like to help with this issue. Please see first patch for node links support.
I'm still looking into Contextual links, will provide some thoughts later.

One code change besides the node links handling is:

 /**
@@ -182,7 +217,7 @@ function flag_entity_view(array &$build, EntityInterface $entity, EntityViewDisp
     $flag_type_plugin = $flag->getFlagTypePlugin();
 
     // Only add cache key if flag link is displayed.
-    if ($flag_type_plugin->showAsField() && !$display->getComponent('flag_' . $flag->id())) {
+    if (!$flag_type_plugin->showAsField() || !$display->getComponent('flag_' . $flag->id())) {
       continue;
     }
--

There was a bug that was causing showAsField to display Flag item on all view modes, even if it was not added to the Manage display field, and the order was wrong, I changed that condition here. Let me know if it's better to open a different ticket.

Added tests, please review

Thanks,
Alexei.

Finally was able to figure out contextual links caching and how to make it more granular, adding contextual links functionality with basic tests.
Please review.

Hi folks,

Added tests and rerolled according to latest changes.

I have one serious problem that I'd like to discuss, it is Contextual Links placeholder being cached. Looks like contextual links module assumes contextual links are static and rarely change. Contextual links placeholder is only invalidated with node view cache. We can either invalidate node view cache on Flag/Unflag action if we have flags in contextual links, or we need to make the node (entity) cache vary by flag state. For now I had to vary by flag state, but I had to return a previously deleted hook, which was previously used to provide cache granularity for flags as fields, which was probably removed as fields are now using #lazy_builder:

/**
 * Implements hook_entity_build_defaults_alter().
 */
function flag_entity_build_defaults_alter(array &$build, EntityInterface $entity, $view_mode = 'full', $langcode = NULL) {
  // Add the flag ID combined with the action to the cache key if render caching is enabled.
  if (isset($build['#cache']) && isset($build['#cache']['keys'])) {

    // Get all possible flags for this entity type.
    $flag_service = \Drupal::service('flag');
    $flags = $flag_service->getFlags($entity->getEntityTypeID(),
      $entity->bundle());

    foreach ($flags as $flag) {
      $flag_type_plugin = $flag->getFlagTypePlugin();

      // Only add cache key if flag link is displayed as contextual link.
      if (!$flag_type_plugin->showContextualLink()) {
        continue;
      }

      if (\Drupal::currentUser()->hasPermission('access contextual links'))

      $action = 'flag';
      if ($flag->isFlagged($entity)) {
        $action = 'unflag';
      }

      // @todo: Explore if it makes sense to use contexts for this.
      $build['#cache']['keys'][] = $flag->id . '-' . $action;
    }
  }

  return $build;
}

I personally prefer cache invalidation more than varying by flags state, but I don't think we should invalidate Node View cache on every flag/unflag event, this may sound like overkill for someone, unless we do it only in special cases when we know that we're displaying the flag in contextual links.

I've noticed that we are calling resetCache in FlagService::flag method:

    $this->entityManager
      ->getViewBuilder($entity->getEntityTypeId())
      ->resetCache([
        $entity,
      ]);

If this is due to node view caching fields, we already eliminated the need in this code, because we use #lazy_builder, I guess.
If this code is still needed for something else, then one more note is needed, the resetCache is needed also on FlagService::unflag action then, as this has same effect, need a cache invalidation.

If we want to invalidate Node View cache to solve contextual links problem, I don't believe this is FlagService's job to do that, I guess we can hook via hook_entity_insert/hook_entity_delete or by subscribing to FlagEvents::ENTITY_FLAGGED and FlagEvents::ENTITY_UNFLAGGED. I believe we may need to invalidate Node View cache only if we use Flags as Contextual Links, because Node Links and Flag Fields already use #lazy_builders, in that case I don't think FlagService should be checking for display type, it shouldn't know how the flag is being used.

Let me know if this needs more work ;)

Thanks,
Alexei

Ooops, small correction, added base_path() to URL checks in contextual links test.
I must consider running tests locally under "/something" directory to catch those earlier =)

Thanks for working on this!

I had a bit of a look at this one yesterday, and it does look rather more complex than I would have thought.

So on the one hand I am wondering whether this issue should really be split into two -- one for node/comment links, and one for contextual.

But on the other hand, both of these features are going to have to deal with the same problem: the links template (links.html.twig -- I guess we don't say theme_links() in D8 any more) no longer accepts HTML links.

On flag D7, we use that to shove our own completely rendered Flag link into the links array:

      $element['#links']['flag-' . $name] = array(
        'title' => $flag->theme($flag->is_flagged($entity_id) ? 'unflag' : 'flag', $entity_id),
        'html' => TRUE,
      );

Just passing in the URL like this isn't going to work with our JS links:

+++ b/flag.module
@@ -149,7 +149,42 @@ function flag_entity_extra_field_info() {
+    $link = $link_type_plugin->buildLink($action, $flag, $entity);
+    $links['node']['#links']['flag-' . $flag->get('id')] = [
+      'url' => $link['#url'],
+      "title" => $link['#title'],
+      "attributes" => $link["#attributes"],
+    ];
+  }

So that's our first problem. Second is that we now have a lazy builder, and I couldn't get that to work with the links array.

1. +++ b/flag.module
   @@ -149,7 +149,42 @@ function flag_entity_extra_field_info() {
   +    // Only add link if flag link is displayed.
   +    if ($flag_type_plugin->showInLinks($context['view_mode']) === 0) {
   +      continue;
   +    }
   

   I happened to look at this today. showInLinks() returning a view mode name is just weird! I'll file a subissue to deal with that since this needs more work anyway.

2. +++ b/flag.module
   @@ -149,7 +149,42 @@ function flag_entity_extra_field_info() {
   +    $action = 'flag';
   +    if ($flag->isFlagged($entity)) {
   +      $action = 'unflag';
   +    }
   +
   +    // If the user does not have permission, go to the next foreach loop and
   +    // don't display this flag.
   +    if (!$flag->hasActionAccess($action)) {
   +      continue;
   +    }
   

   This bit might need a reroll -- the API has been simplified here and you no longer need to check for access or pass in $action if you're getting the completely built link.

3. +++ b/flag.module
   @@ -178,7 +213,7 @@ function flag_entity_view(array &$build, EntityInterface $entity, EntityViewDisp
        // Only add cache key if flag link is displayed.
   -    if ($flag_type_plugin->showAsField() && !$display->getComponent('flag_' . $flag->id())) {
   +    if (!$flag_type_plugin->showAsField() || !$display->getComponent('flag_' . $flag->id())) {
   

   Good catch! -- but this should be filed as a separate bug please.

I've file this core issue: #2587417: Unable to use lazy builders for individual links in hook_node_links_alter(), hook_comment_links_alter().

I've been thinking about this some more and I now reckon we *should* split off the fix for contextual links. My reasoning is that contextual links definitely can't have a JS link in them -- they are only going to be the 'reload' type (though what will probably happen is that they'll bypass the link type plugins entirely). So they're something we can fix now, whereas the entity links stuff requires more work.

Hi joachim,
Thanks for your feedback.

Changed title to reflect issue split.

Fired a separate issue for contextual links: #2599304: [regression] Reimplement Contextual Link Display
Fired a separate issue for pseudofield display settings bug #2599074: Flag link pseudo field is displayed on all view modes even if 'Display link as field' setting is disabled

I'm not sure this should have been closed - "Display in Entity Links" doesn't appear to have been fixed (at least, in the current -dev) - instead, we split off a second issue for the contextual links, and then the issue was accidentally closed. Reopening and setting as a beta blocker.

This was possibly closed in favour of #2703229: [regression] Flags not appearing in node and comment links section?