Issue 1367: Loosen Islandora Breadcrumbs applies

[seth-shaw-unlv]

GitHub Ticket: Islandora/documentation#1367

What does this Pull Request do?

Loosens the applies on islandora breadcrumbs so that it no longer requires a value in the configured reference field (member_of by default). This allows Islandora Objects with the Collection model (that won't have a value in member_of unless they are sub-collections) to use this breadcrumb builder instead of defaulting to the system provided breadcrumbs.

What's new?

Simply removed the !empty condition from applies.

How should this be tested?

• Create an Islandora object without completing the member_of field.
• It will use the system breadcrumbs, although it looks like Carapace doesn't show the 'node' parent for these 🤷‍♂️. (If you want to be sure, add a dsm to the breadcrumb builder's build function. It won't fire when the system one does.)
• Apply the PR
• Clear caches
• View the item again. (If you added the dsm function it should fire now.)

Additional Notes:

If you want to be extra sure it is working, you can edit the islandora.breadcrumbs config file's 'includeSelf' setting to 'TRUE' and clear caches again. You should now see the current node's title in the breadcrumbs. (Sorry, we don't have a form for this yet, so you need to use one of the system-supplied config editing methods. We should probably make an issue for that... or maybe I should just do that now...)

This PR is untested since my VM is still tied up testing other stuff... Finally tested and ready for review.

Also, I haven't put it through @qadan's test from the ticket.... but my wife is kicking me off the computer right now.

Interested parties

@qadan and @jordandukart

[seth-shaw-unlv]

I was able to come back and verify that, yes, with this PR the scenario @qadan mentioned in the ticket (copied below for reference) is resolved with this PR. 🎉

• Create a collection repository item; we'll call it B
• Add a member to that collection - I created an image item; we'll call it A
• Create another collection repository item; we'll call it C
• Edit B to have a member relationship to C
• Visit A, note the breadcrumbs display "B / A"
• Clear the cache via drush cr
• Visit A, note the change in breadcrumbs displaying "C / B / A"

Note: this scenario assume that the breadcrumbs include the current page (which is not enabled by default with this module). After the PR is applied visiting 'A' will display the appropriate breadcrumbs without needing the cached cleared.

[jordandukart]

Unrelated to this PR but there is an edge case here where a hanging entity reference will cause a WSOD, in 7 it appears we just killed the recursion and added a placeholder ...: https://github.com/Islandora/islandora_solr_search/blob/7.x/includes/breadcrumbs.inc#L77-L84.

To reproduce chain together A->B->C, delete A and visit B and you'll WSOD as follows:

TypeError: Argument 1 passed to Drupal\islandora_breadcrumbs\IslandoraBreadcrumbBuilder::walkMembership() must implement interface Drupal\Core\Entity\EntityInterface, null given, called in /var/www/html/drupal/web/modules/contrib/islandora/modules/islandora_breadcrumbs/src/IslandoraBreadcrumbBuilder.php on line 113 in Drupal\islandora_breadcrumbs\IslandoraBreadcrumbBuilder->walkMembership() (line 95 of modules/contrib/islandora/modules/islandora_breadcrumbs/src/IslandoraBreadcrumbBuilder.php).

Drupal\islandora_breadcrumbs\IslandoraBreadcrumbBuilder->walkMembership(NULL, Array) (Line: 113)
Drupal\islandora_breadcrumbs\IslandoraBreadcrumbBuilder->walkMembership(Object, Array) (Line: 113)
Drupal\islandora_breadcrumbs\IslandoraBreadcrumbBuilder->walkMembership(Object, Array) (Line: 74)
Drupal\islandora_breadcrumbs\IslandoraBreadcrumbBuilder->build(Object) (Line: 83)
Drupal\Core\Breadcrumb\BreadcrumbManager->build(Object) (Line: 72)

We could throw a check on https://github.com/Islandora/islandora/pull/795/files#diff-cf1229cc1e397e9c51c68ebcc74162b4L112 to ensure there is actually a resolvable parent before recursing again but we'd need to decide on what behavior we'd want rendered in the breadcrumbs when this scenario arises.

Am fine potentially splitting this into another issue @seth-shaw-unlv and moving this one along.

[seth-shaw-unlv]

Might as well look at it now.

[seth-shaw-unlv]

@jordandukart, I'm not able to reproduce the WSOD and error message.

I would think that the existing check for empty and the target empty being an instance of EntityInterface (which should have catched Null) would cover this case:

!$entity->get($this->config->get('referenceField'))->isEmpty() &&
$entity->get($this->config->get('referenceField'))->entity instanceof EntityInterface) {

🤷‍♂️

[jordandukart]

On a fresh box I can't reproduce what I had mentioned in #795 (comment), leaving this open as per @seth-shaw-unlv's Slack message about wanting to roll a test.

[seth-shaw-unlv]

@jordandukart, I updated the test and it seems happy with the change too. Assuming Travis doesn't timeout again we should be 👍.

[jordandukart]

Looks good to me, test passed locally as well just Travis timing out.

[seth-shaw-unlv]

@jordandukart, I updated the travis script to see if it reveals what is stalling the build. 🤞

If any other @Islandora/8-x-committers want to help debug Travis, I'd be happy for the support.

[seth-shaw-unlv]

Whelp, Travis decided it was happy when I added the process listing command and nothing looks wrong... Let's see what happens when I take it back out.

[seth-shaw-unlv]

Hey, @jordandukart! Green means go! 😀

I'm glad Travis is feeling better today.