Members and Media tabs appear on non-Repository Item pages

[kayakr]

In Islandora VM built via claw-playbook, if you add a non-Repository Item node, the Members and Media tabs are visible.

Is there a use case for adding Repository Items to a Basic Page or Article, or should this default to being limited to Repository Item?

[mjordan]

I've got some code here that limits tabs based on the content type. It's currently hard coded to 'islandora_object' but I intend to make the applicable content types an admin settings option.

[mjordan]

Sorry, forgot to mention that the access function linked to above needs to be registered here.

[kayakr]

@mjordan That looks like a very useful module; what else do you have up your sleeve?

[mjordan]

Some things that are mature enough to share are listed in the 8.x section of https://github.com/Islandora-Labs/islandora_awesome.

[dannylamb]

In addition to using access restrictions to hide them, there's also the possibility of exporting those views as blocks instead of giving them tabs for pages. If placed as blocks you'd definitely have better control over their visibility, but then that ties things to a theme, so we might have to shuffle around where some config lives (e.g. move some things from islandora_core_feature into islandora_demo).

[mjordan]

@dannylamb since multiple modules might use this setting, does it make sense to make it an admin option on the base islandora module? That way, any contrib or other core module that depends on islandora can assume it's there.

[mjordan]

Update - my code that uses AccessResult still shows the local task tab for admin users, since they have all permissions. I'll continue to investigate.

[mjordan]

I'm having no luck with implementations of hook_local_tasks_alter() to remove the tab. I think I'm going to try blocks as @dannylamb suggests. Doesn't solve the problem @kayakr points out though.

[dannylamb]

@mjordan Being user 1 overrides returning AccessResult::FORBIDDEN? 💩

It makes sense b/c you always need one account that can get into everything. Just crappy because abusing the permissions system seems to be the only real recourse we have in D8.

[dannylamb]

@mjordan And was it a timing issue with hook_local_tasks_alter()? I mean, did the task exist to unset when that alter fires? Or was it something else?

[mjordan]

During May 7 Islandora 8 call, @dannylamb suggested trying blocks in other admin themes such as adminimal. Also, I said I'd retry unsetting withing hook_local_tasks_alter().

[dflitner]

This is a regression from Drupal 7. You should be able to limit visibility of tabs based on content type. Here is a the issue thread where someone has been working on a patch - https://www.drupal.org/project/drupal/issues/2778345

[mjordan]

@dflitner thanks, I see that the approach you are suggesting uses a plugin, as does the solution documented at https://www.drupal.org/docs/8/api/menu-api/providing-module-defined-local-tasks. I had some success with these (after a lot of trial and error making the necessary changes to my links.task.yml file). But that apporoach required a plugin class file, and I wanted to see if I could get away with just a simple hook_local_tasks_alter() implementation. Success there as well but I'm seeing some caching behavior I can't explain.

@dannylamb it doesn't appear to be a timing issue, but a caching issue. Here's what I'm seeing with hook_local_tasks_alter(). It appears that the alter hook is only fired the first time after the cache is cleared. Here's my implementation:

function mymodule_local_tasks_alter(&$local_tasks) {
  dd("alter hook fired");
  $node = \Drupal::routeMatch()->getParameter('node');
  if ($node instanceof \Drupal\node\NodeInterface) {
    $nid = $node->id();
    if ($nid == 5) {
      dd("Hi from node 5");
    }
    else {
      dd("Hi from some node other than 5");
    }
  }
}

When I clear my cache and hit a node page, this works fine, but only the first time I view a node. Any subsequent visit to a node page does not fire this implementation. Can someone try this to give me a sanity check? I do have a version of this that unset()s the menu item I want to hide, so once I figure out why the implementation is not firing on every node, I should be able to only show the tabs on the configured content types.

[DiegoPino]

@mjordan totally normal behavior. Hook is fired correctly, but rendered output of a node (any entity) is always cached in D8 and that cache (for good reasons) will never contain dd/dpm output so you will see it only once just after clearing your caches. You can disable your caches, or rely on the core debug() but sometimes its much better and more reliable to use https://symfony.com/doc/current/components/var_dumper.html and make your messages go to the logs where you will always find them.

[mjordan]

@DiegoPino but this behavior also extends to local task tabs - I am seeing the same thing happen with them not showing up based on dynamic logic (node is of content type) as I'm seeing with dd(). Since the goal here is to only render a local task tab if the node has one of the configured content types (and ideally by role as well) hook_local_tasks_alter() is not the right approach to this problem.

[DiegoPino]

@mjordan not sure what you want to accomplish, remove what you have or add something?

If you want to add a Tab/local task ,static, simplest, guessing that is what you do here already, you need a route item which you already have i guess. So If you are already generating for every node media and members tabs and you don't want to change the plugin logic of those routes to be dynamic (using a deriver) then the local_task_alter allows you to remove/tweak the tab using a certain condition. dd($local_tasks) to see what you have there, all keyed by the plugin that generates the task.

But maybe you want just to deal with this on the cosmetic side and if you don't want to code plugins
You need the menu local task alter (both hooks are just cosmetic, the routes still exist), where you can basically work with a render array, see
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Menu%21menu.api.php/function/hook_menu_local_tasks_alter/8.7.x (see the cache statement at the end to force cache be user dependent)

If you want to change the existing logic and make all custom and dynamic, you can always use a deriver there like
https://api.drupal.org/api/drupal/core%21modules%21views%21src%21Plugin%21Derivative%21ViewsLocalTask.php/function/ViewsLocalTask%3A%3AalterLocalTasks/8.2.x

Everything becomes clearer if you look at the task manager and how it generates and deals (and alters) tasks
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Menu%21LocalTaskManager.php/class/LocalTaskManager/8.2.x

[mjordan]

@DiegoPino goal is to show specific tabs only on nodes of specific content types. For the Media and Members tabs, @dflitner's approach is applicable; I think all we'd need to do is modify those two views so they incorporate that configuration. But what I am looking for is a way for modules to define a tab and then have that tab appear only on nodes that model "Islandora" objects.

For example, in my Islandora Whole Object module, I have a route that renders a tab. Some nodes (ones that have a specific content type) should have the tab and some shouldn't (ones that don't have that content type). Using hook_local_tasks_alter(), I want to remove unset()) that tab on nodes that are not in a configured list of "Islandora content types". What I've seen with hook_local_tasks_alter() is that the logic works, but only the first time you hit a node after clearing the cache. On subsequent hits, the logic doesn't work.

As an alternative to using hook_local_tasks_alter(), I have created a deriver class to add the local task tab to nodes of the configured content types (opposite approach of the hook_local_tasks_alter()), but I am seeing the same caching behavior - the logic in the deriver class shows the local task tab as expected first time after the cache is cleared but not subsequently.

Here's my deriver class:

<?php

namespace Drupal\islandora_whole_object\Plugin\Derivative;

use Drupal\Component\Plugin\Derivative\DeriverBase;


/**
 * Defines dynamic local tasks.
 */
class IslandoraContentTypes extends DeriverBase {

  /**
   * {@inheritdoc}
   */
  public function getDerivativeDefinitions($base_plugin_definition) {
    $this->derivatives = [];
    $node = \Drupal::routeMatch()->getParameter('node');
    if ($node instanceof \Drupal\node\NodeInterface) {
      $config = \Drupal::config('islandora_whole_object.settings');
      $types = $config->get('islandora_node_types');
      $node_type = $node->getType();

      if (in_array($node_type, array_values($types), TRUE)) {
        $this->derivatives['islandora_whole_object'] = $base_plugin_definition;
        $this->derivatives['islandora_whole_object']['title'] = "I'm a tab";
        $this->derivatives['islandora_whole_object']['base_route'] = 'entity.node.canonical';
        $this->derivatives['islandora_whole_object']['route_name'] = 'islandora_whole_object.whole_object';
      }
    }
    return $this->derivatives;
  }
}

and here's my islandora_whole_object.links.task.yml file:

islandora_whole_object:
  deriver: \Drupal\islandora_whole_object\Plugin\Derivative\IslandoraContentTypes
  weight: 100

Any suggestions on why this deriver is also being squashed by the cache?

[whikloj]

@mjordan when you say

On subsequent hits, the logic doesn't work.

So the tab disappears the first time you view the page but not after that? Because my understanding is that the page would be produced, rendered and then cached. Your code would not run after that but the rendered page should still have the tab removed.

Alternatively, you could try invalidating the cache for the page altered.
Cache::invalidateTags("node:5");

[DiegoPino]

@mjordan this https://drupal.stackexchange.com/questions/273062/dynamic-local-tasks-caching-problems

[dflitner]

I have a site that I updated to 8.7 and then applied patch #35 from that issue I linked above. The patch works and the Members and Media tabs no longer show on anything except repository items after I make the configuration change that I showed in the screenshot.

However, as someone notes in the next comment of the issue queue, a link to a taxonomy term list in a menu will also vanish after applying this patch. For anonymous users, the link will appear only on taxonomy/term/% and no other page. Admins can still see the link on any page.

The way the patch works is to activate on views that are set to display a 404 or access denied if the filter doesn't validate. So the workaround for disappearing taxonomy menu items is to change the filter to display contents of "no results found" or any other setting.

I have no idea how this might affect the broader issue that Mark is working on.

[dannylamb]

I'm going to export those views as block and jam in some action links by hand (unfiltered text in a header FTW) and see how they look. Then we could control them with context at least. Plus we'd still have the routes, just not the tabs that lead to them.

Might go nowhere, but it's worth exploring until https://www.drupal.org/project/drupal/issues/2778345 lands.

[mjordan]

@dannylamb agreed, this shouldn't hold up the release.

[dannylamb]

Uh... maybe not. Not only does it look terrible without the admin theme, it also messes with tab placement.

It does occur to me that we could easily make blocks with menu links for adding media and members instead of relying on the tabs, and then display those blocks on certain pages with context.

[mjordan]

Since I was involved in this discussion, in the interests of shortening the 1.0.0 milestones list, I'd like to propose that we mark it as a known issue. We can tackle it for the next release.

[mjordan]

I think I've got a solution to this, although it's a bit of a hack, using Javascript and CSS properties.

This implementation of hook_page_attachments() loads a Javascript file:

/**
* Implements hook_page_attachments().
*/
function mymodule_page_attachments(array &$attachments) {
  $current_path = \Drupal::service('path.current')->getPath();
  $path_args = explode('/', ltrim($current_path, '/'));
  if ($path_args[0] == 'node') {
    $node = \Drupal::routeMatch()->getParameter('node');
    if ($node) {
      $type = $node->getType();
      // The list of Islandora content types would come from whatever solution we find for #1255.
      if (!in_array($type, array('islandora_object'))) {
        $attachments['#attached']['library'][] = 'mymodule/mymodule';
      }
    }
  }
}

Here's the Javascript file:

(function (Drupal, $) {
  "use strict";
  $(".tabs__tab a[data-drupal-link-system-path$='members']").css("display","none")
  $(".tabs__tab a[data-drupal-link-system-path$='media']").css("display","none")
}) (Drupal, jQuery);

And of course the mymodule.libraries.yml file:

mymodule:
  version: 1.x
  js:
    js/notabs.js: {}
  dependencies:
    - core/jquery
    - core/jquery.once

Here are the tabs for an Islandora node:

And the tabs for an Article node:

Tagging @rosiel ....

[mjordan]

The Javascript above works with Seven as the admin theme, but when i switched to Bartik as the admin theme, it stopped working. Different markup. This Javascript works with both themes, and probably with any theme (famous last words):

(function (Drupal, $) {
  "use strict";
  $("a[data-drupal-link-system-path$='members']").css("display","none")
  $("a[data-drupal-link-system-path$='media']").css("display","none")
}) (Drupal, jQuery);

[mjordan]

As per today's tech call, I looked for a way to load Javascript using Context for D8 and didn't find one.

I've packaged up the code above into a small test module, which is attached. To test it, copy the zip file to your Vagrant's /var/www/html/drupal/web/modules/contrib directory and unzip it. Then enable it with drush en -y islandora_1057. When you visit a node of content type islandora_object, you'll see the Media and Members tabs; when you visit a node of any other content type, you won't.

islandora_1057.zip

If we can't find another way to eliminate those tabs, I can add this code to the Islandora module and open a PR.

[dflitner]

I thought we might simplify this a bit, if we must use CSS to hide the tabs. We could add a context from existing reactions to add a body class to all content types that are not repository items. Then add the appropriate CSS to the theme.

I'm not sure we need to use Javascript to insert the CSS.

[joecorall]

Instead of this javascript approach, we could add a custom callback to the View routes. In this patch on the islandora core module, it checks if the current node has the field field_member_of, and if not, it hides the tabs.

That patch can be applied by running this command in the islandora core module directory:

cd /var/www/html/drupal/web/modules/contrib/islandora
git apply -v /path/to/downloaded/patch/hide-media-tabs.patch

[seth-shaw-unlv]

Interesting approach. However, a content type may be a target of field_member_of but not have the field itself. This would result in this hypothetical content type not having a tab to manage members. Granted, this is perhaps an edge case insignificant enough to ignore for pragmatism, but I thought I would mention it.

[joecorall]

That's a good point. We definitely could change the conditional logic. For instance, I have a custom module that does this same sort of thing, but hides the "Members" tab for non-collection nodes. I just based the requirement of field_member_of off of the definition offered in What even IS Islandora 8? thread

According to Danny, field_member_of is required to be on a content type for that content type to be Islandora

[whikloj]

I was able to do this with a custom access check

8.x-1.x...whikloj:issue-1057

[whikloj]

Boy do I wish I'd read @joecorall's post before I started this

[mjordan]

Hi everyone, I'll give these options a test this evening.

[mjordan]

@kayakr I merged @whikloj's PR to resolve this issue. OK to close?

[mjordan]

@kayakr ?

[kayakr]

@mjordan Sorry for the delay. Trying it now.

[kayakr]

@mjordan Resolved by #160, thanks.