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

Add ancestor helper and condition. #865

Merged
merged 4 commits into from
Apr 5, 2022

Conversation

jordandukart
Copy link
Member

GitHub Issue: Islandora/documentation#2071

What does this Pull Request do?

Adds a new condition to allow for multiple ancestors to be looked for use in context condition.
Re-works the breadcrumb builder to use the new helper to DRY things up.

What's new?

New condition.

How should this be tested?

Regression test the breadcrumb builder.

Use the condition in whatever context you want and see that it applies correctly.

Interested parties

@seth-shaw-unlv

@jordandukart jordandukart marked this pull request as ready for review March 30, 2022 17:34
@seth-shaw-unlv
Copy link

So, this is a bit odd, I'm not surprised that the refactor resulted in this, but we have a quirk to talk about.

At first glance, the helper works as expected. Below are sub-collection "C" before and after the PR are applied:
Original:
Original No loop
PR:
Helper No loop
They look the same, which means it works. 👍

But what if we make collection "A" also a child of "C", creating a loop?
Original:
Original loop
PR:
Helper loop
Note that the existing ancestor walking code excludes itself from the ancestor tree, whereas the PR ancestor walker includes it as the root. That may not necessarily be a bad thing, but it does break from the current implementation.

Copy link

@seth-shaw-unlv seth-shaw-unlv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@seth-shaw-unlv seth-shaw-unlv merged commit a7e4c16 into Islandora:2.x Apr 5, 2022
@simonhm
Copy link

simonhm commented Apr 18, 2022

@jordandukart @seth-shaw-unlv
This condition shouldn't be required, since not all of blocks need to have this condition. You will get errors and not able to save block when not filling this condition.
It's ok with this condition left empty in Drupal 9.3.x, but not in Drupal 9.4.x-dev.

Screen Shot 2022-04-18 at 6 01 48 PM

Screen Shot 2022-04-18 at 5 59 48 PM

Simon.

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

Successfully merging this pull request may close these issues.

3 participants