Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose parent collection as link header #680

Closed
dannylamb opened this issue Jul 12, 2017 · 17 comments
Closed

Expose parent collection as link header #680

dannylamb opened this issue Jul 12, 2017 · 17 comments
Assignees

Comments

@dannylamb
Copy link
Contributor

Every time a node is viewed, check to see if it has field_memberof, and if it does, add the url to referenced collection as a link header.

You have access to the node and can add link headers in hook_node_view_alter like so:

function islandora_node_view_alter(&$build, EntityInterface $node, EntityViewDisplay $display) {
$build['#attached']['http_header'] = [
['Link', 'http://islandora.ca; rel="awesome"'],
];
}

Use the existing rel of "collection" to describe the link.

@whikloj
Copy link
Member

whikloj commented Jul 20, 2017

hook_node_view_alter is gone in Drupal 8. (https://api.drupal.org/api/drupal/core%21modules%21node%21node.api.php/8.3.x)

So it looks like we need to find a new way to add this header.

@whikloj
Copy link
Member

whikloj commented Jul 20, 2017

Oops sorry, forget that. It has been made part of core.
(https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21entity.api.php/function/hook_ENTITY_TYPE_view_alter/8.3.x)

So hook_node_view_alter is still valid.

@Natkeeran
Copy link
Contributor

Natkeeran commented Jul 20, 2017

@whikloj
The following function adds the collection link (i.e Link →/node/8; rel='collection', </node/16>; rel="canonical", </node/16>; rel="shortlink", </node/16>; rel="revision"). Note that Islandora currently only supports one parent collection (in the islandora image). We would need to support multiple collections, right?

/**
 * Implements hook_node_view_alter() to add collection links to headers.
 */
function islandora_node_view_alter(&$build, EntityInterface $entity) {
  // Only proceed if memberof field exists and has value
  if($entity->hasField('field_memberof') == true && count($entity->get('field_memberof')->getValue()) == 0) {
    return;
  }

  $arrMemberOf = $entity->get('field_memberof')->getValue();

  $collection_id = $arrMemberOf[0]['target_id'];
  $collection_path = \Drupal::service('path.alias_manager')->getAliasByPath('/node/'.$collection_id);

  $build['#attached']['http_header'] = [
    ['Link', "$collection_path; rel='collection'"],
  ];
}

@whikloj
Copy link
Member

whikloj commented Jul 21, 2017

@Natkeeran Looking good, you should probably add a check to ensure the thing you are linking to is of the bundle islandora_collection. Eventually the field_memberof could also be used to link a page to a book.

Last thing, is in your snippet it appears that the collection URI doesn't have < and > around it and instead has an arrow ->?? Is that just Github messing with your comment?

@dannylamb
Copy link
Contributor Author

@Natkeeran I wouldn't restrict based on the number of entities in field_hasmember and just iterate over all of them, adding a link header for each. Then when we open up to having multiple collection memberships we shouldn't have to touch up this code.

@Natkeeran
Copy link
Contributor

Natkeeran commented Jul 24, 2017

@whikloj
I've added the collection check. Note that this is going to cost us a little in performance due load and check of each member entity info. Added < > to the URIs as well.

@dannylamb
Added the loop to check against the list.

/**
 * Implements hook_node_view_alter() to add collection links to headers.
 */
function islandora_node_view_alter(&$build, EntityInterface $entity) {
  // Only proceed if memberof field exists and has value.
  if ($entity->hasField('field_memberof') == false || count($entity->get('field_memberof')->getValue()) == 0) {
    return;
  }

  // Loop through each member and add to the collection_links.
  $arrMemberOf = $entity->get('field_memberof')->getValue();
  $collection_links =  [];
  foreach ($arrMemberOf as &$memberInfo) {
    $collection_id = $memberInfo['target_id'];
    $collection_entity = $entity->load($collection_id);

    // If collection entity does not exist, skip.
    if ($collection_entity == NULL) {
      continue;
    }

    // If entity bundle type is not Collection, skip.
    $collection_entity_bundle = $collection_entity->bundle();
    if ($collection_entity_bundle != "islandora_collection") {
      continue;
    }

    $collection_entity_url = $collection_entity->url('canonical', ['absolute' => TRUE]);
    array_push($collection_links, "<" . $collection_entity_url . ">");
  }

  if (count($collection_links) > 0) {
    $collection_links_str = implode(";", $collection_links);
    $build['#attached']['http_header'] = [
      ['Link', "$collection_links_str; rel='collection'"],
    ];
  }
}

One issue I did run into is that while my FF debug provides the added links as below, I could not that get the added links with POSTMAN. I was able to get that before. Not sure what changed!

Link: <http://localhost:8000/node/1>;<http://localhost:8000/node/3>; rel='collection', </node/2>; rel="canonical", </node/2>; rel="shortlink", </node/2/delete>; rel="delete-form", </node/2/edit>; rel="edit-form", </node/2/revisions>; rel="version-history", </node/2>; rel="revision", </devel/node/2>; rel="devel-load", </devel/node/2/render>; rel="devel-render", </devel/node/2/definition>; rel="devel-definition"

@whikloj
Copy link
Member

whikloj commented Jul 25, 2017

Yes this will cost us, but that is just how it has to be. Also we can expand this loop to account for additional relationships if we so desire.
Only thing I am not sure about is whether you can do

Link: <http://localhost:8000/node/1>;<http://localhost:8000/node/3>; rel='collection'

or if you should be doing

Link: <http://localhost:8000/node/1>; rel='collection', <http://localhost:8000/node/3>; rel='collection'

I'm not good with these syntax diagrams, https://tools.ietf.org/html/rfc5988#section-5

@dannylamb
Copy link
Contributor Author

@whikloj The second. It should be separate header values per link.

@dannylamb
Copy link
Contributor Author

And yeah... all these checks are going to cost us. But I think it's worth doing and then seeing if it actually becomes an issue. Loading two extra entities? That's probably fine. Ten entities? We'll see. A hundred entities? That'll probably be a problem.

@dannylamb
Copy link
Contributor Author

@Natkeeran Feel free to make a PR at any time. And maybe Postman is caching? If you can get it with cURL, then you know it's working. You can make a HEAD request with curl -I.

@DiegoPino
Copy link
Contributor

DiegoPino commented Jul 25, 2017

@Natkeeran should be in @whikloj's later form:

<http://localhost:8000/node/1>; rel='collection', <http://localhost:8000/node/3>; rel='collection'

The operation to get this can be a bit expensive and not sure if in tune with how the rest of drupal 8 environment does the render array stuff, kinda feel we need to look at
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21BubbleableMetadata.php/8.2.x

There are over 62 implementations so plenty of examples, but mostly it's a way of adding your own #attachment , etc stuff, in a cacheable and mergeable way.

What i found so far in case someone could find this uselful:

https://www.drupal.org/docs/8/api/render-api/bubbleable-metadata

Filters can also make use of bubbleable-metadata
https://api.drupal.org/api/drupal/core%21modules%21filter%21src%21FilterProcessResult.php/class/uses/FilterProcessResult/8.2.x

So maybe, doing the following mix?
A) use the hook_node_view_alter hook to force every node to have an extra Islandora defined Filter
B) create that filter and use there BubbleableMetadata to add the headers to the attachment? See https://api.drupal.org/api/drupal/core%21modules%21filter%21src%21FilterProcessResult.php/class/FilterProcessResult/8.2.x AND as example and
https://www.lullabot.com/articles/creating-a-custom-filter-in-drupal-8

Lastly, question about that implementation, is

$build['#attached']['http_header'] = ...

Not overwriting existing, possibly added by other modules or core, headers? Not sure.

@dannylamb
Copy link
Contributor Author

@DiegoPino I took your post as an opportunity to investigate the interplay between adding link headers and D8 render array caching, and it looks like we're in the clear. The alter won't run unless the cache is invalidated, and D8 core has already set that up appropriately. Any time the underlying node is edited, the render array (along with our headers) is regenerated. This operation will not run on every page load, and it will also not serve a stale link header in case someone edits field_memberof.

And yes, I find the $build['#attached']['http_header'] = ... syntax confusing as well. When playing with it, I discovered it indeed appends instead of overwriting, which is not how it appears on the surface.

@Natkeeran Please feel free to issue a PR with what you have. You shouldn't have to do anything extra w/r/t caching.

@Natkeeran
Copy link
Contributor

@dannylamb

I'll create a PR.

There is a minor difference with respect to how the Links get displayed in CURL vs FF.
CURL:

$ curl -I http://localhost:8000/node/2
HTTP/1.1 200 OK
Date: Tue, 25 Jul 2017 18:53:03 GMT
Server: Apache/2.4.18 (Ubuntu)
Cache-Control: must-revalidate, no-cache, private
X-Drupal-Dynamic-Cache: MISS
Link: <http://localhost:8000/node/1> rel='collection', <http://localhost:8000/node/3> rel='collection'
Link: </node/2>; rel="canonical"
Link: </node/2>; rel="shortlink"
Link: </node/2>; rel="revision"
X-UA-Compatible: IE=edge
Content-language: en
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
Expires: Sun, 19 Nov 1978 05:00:00 GMT
X-Generator: Drupal 8 (https://www.drupal.org)
X-Drupal-Cache: MISS
Content-Type: text/html; charset=UTF-8

FF

Date: Tue, 25 Jul 2017 18:53:22 GMT
Server: Apache/2.4.18 (Ubuntu)
Cache-Control: must-revalidate, no-cache, private
X-Drupal-Dynamic-Cache: UNCACHEABLE
Link: <http://localhost:8000/node/1> rel='collection', <http://localhost:8000/node/3> rel='collection', </node/2>; rel="canonical", </node/2>; rel="shortlink", </node/2/delete>; rel="delete-form", </node/2/edit>; rel="edit-form", </node/2/revisions>; rel="version-history", </node/2>; rel="revision", </devel/node/2>; rel="devel-load", </devel/node/2/render>; rel="devel-render", </devel/node/2/definition>; rel="devel-definition"
X-UA-Compatible: IE=edge
Content-Language: en
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
Expires: Sun, 19 Nov 1978 05:00:00 GMT
X-Generator: Drupal 8 (https://www.drupal.org)
Vary: Accept-Encoding
Content-Encoding: gzip
Content-Length: 8131
Keep-Alive: timeout=5, max=98
Connection: Keep-Alive
Content-Type: text/html; charset=UTF-8

Method

/**
 * Implements hook_node_view_alter() to add collection links to headers.
 */
function islandora_node_view_alter(&$build, EntityInterface $entity) {
  // Only proceed if memberof field exists and has value.
  if ($entity->hasField('field_memberof') == false || count($entity->get('field_memberof')->getValue()) == 0) {
    return;
  }

  // Loop through each member and add to the collection_links.
  $collection_members = $entity->get('field_memberof')->getValue();
  $collection_links =  [];
  foreach ($collection_members as &$member_info) {
    $collection_id = $member_info['target_id'];
    $collection_entity = $entity->load($collection_id);

    // If collection entity does not exist, skip.
    if ($collection_entity == NULL) {
      continue;
    }

    // If entity bundle type is not Collection, skip.
    $collection_entity_bundle = $collection_entity->bundle();
    if ($collection_entity_bundle != "islandora_collection") {
      continue;
    }

    $collection_entity_url = $collection_entity->url('canonical', ['absolute' => TRUE]);
    array_push($collection_links, "<" . $collection_entity_url . "> rel='collection'");
  }

  if (count($collection_links) > 0) {
    $collection_links_str = implode(", ", $collection_links);
    $build['#attached']['http_header'] = [
      ['Link', $collection_links_str],
    ];
  }
}

@DiegoPino
Copy link
Contributor

@Natkeeran could you accumulate this

count($entity->get('field_memberof')->getValue())

into a temporary var and reuse it? Instead of

if ($entity->hasField('field_memberof') == false || count($entity->get('field_memberof')->getValue()) == 0) {
    return;
  }

and then later again $collection_members = $entity->get('field_memberof')->getValue(); ?

maybe it is my environment, which is pretty much on debug mode, but i get twice the same entity field query in your code which makes the alter hook a bit more expensive. By calling $collection_members = $entity->get('field_memberof')->getValue() only once, and reusing the value in the conditional (for counting) and then later for iterating (at least here) i get one SQL query (which joins a few tables!) less. Just if possible of course, if you don´t see that behavior just discard my message. Good work, Thanks!

@dannylamb
Copy link
Contributor Author

@Natkeeran Not sure what the difference between cURL and FF is other than FF aggregates all the link headers. That's nothing to worry about.

And heads up, looks like you're missing the ';' between the url and the 'rel' extension.

@Natkeeran
Copy link
Contributor

@DiegoPino Yes, your suggestion makes sense. More efficient.

@dannylamb
Copy link
Contributor Author

Resolved via 615b62b

dannylamb pushed a commit to dannylamb/CLAW that referenced this issue Feb 8, 2018
* DSID not datastream.

* Catch the typed exception and return FALSE for a deprecation cycle.

* Move the exception for easy removal later.

* Update as per code review feedback.

* Update README.

* May.

* English.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants