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

cases where islandora_breadcrumbs caches aren't invalidated #1367

Closed
qadan opened this issue Nov 22, 2019 · 2 comments
Closed

cases where islandora_breadcrumbs caches aren't invalidated #1367

qadan opened this issue Nov 22, 2019 · 2 comments

Comments

@qadan
Copy link

qadan commented Nov 22, 2019

Generally adding member relationships and members seems to generate breadcrumbs okay, but there's some edge cases where they're not showing due to the breadcrumb cache not becoming invalidated when one would expect it to.

STR:

  • 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"

Some fun investigation times

Looking at chunk of islandora_breadcrumbs that determines IslandoraBreadcrumbBuilder::applies() to a repository item, the Islandora-centric breadcrumbs don't apply if whatever configured referenceField is empty. So it falls back to the breadcrumb implementation upstream, which currently has broken cache invalidation.

Would seem that any time you're creating objects that don't have parents (yet), you're going to have an inconsistent experience with breadcrumbing.

Building up an applies() based on multiple parameters would help; checking the existence of field_model and checking the field_media_of relationship would let us build up more complete breadcrumbs. Can probably throw in a proof of concept for this if desired

Also I believe the build() implementation is supposed to implement $breadcrumb->addCacheTags()? To have better control over when they're invalidated as well as for that X-Drupal-Cache-Tags support.

@seth-shaw-unlv
Copy link
Contributor

@qadan, we call addCacheableDependency for each node in the chain (see https://github.com/islandora/islandora/blob/8.x-1.x/modules/islandora_breadcrumbs/src/IslandoraBreadcrumbBuilder.php#L81) which calls addCacheTags for each one. As far as I understand it, that should take care of it.

As for the applies, I think simply removing the && !$node->get($this->config->get('referenceField'))->isEmpty() will let accomplish what we need. Simply having the member_of field should be sufficient.

I should have a PR shortly, but I won't be able to test it until I'm done testing a different PR.

@jordandukart
Copy link
Member

Fixed.

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

3 participants