Only display CollectionList if record is a collection

[LuomaJuha]

Adds an isActive function for CollectionList.php tab. Uses getHierarchyTrees to check if should be active. Tried with isCollection, but isCollection might be too limited for this case.

[demiankatz]

@LuomaJuha, could you tell me a little more about the motivation behind this change? Collections and hierarchy trees are independently configurable settings, and while they are most commonly used together, I think there could be some situations where collections are turned on while hierarchies are turned off, and this change might cause problems. Normally, the CollectionList tab is only turned on for the collection view, so it's sort of implicitly appropriate.

(It's been literally years since I have reviewed all of this logic, so I may be misremembering what is possible... I just know there's a lot of flexibility, and I want to be careful about making the wrong assumption).

In any case, I assume there's an underlying problem that this is meant to solve, I just want to be sure this is the most appropriate solution!

[LuomaJuha]

@demiankatz sure! The description was pretty narrow but:

in situations where collectionlist is assigned to be shown for a record which has or has not any association with a hierarchy, the tab is always shown. If user then clicks the recordtab collectionlist, it will throw an error indicating that collectionField is not set for records without any assigned hierarchy fields. Idea was to create the function to check that if the record is really a part of a hierarchy.

Edit:
HierarchyTrees might not be the best for testing this but there is other functions like: getHierarchyTopID

[demiankatz]

On further reflection, what do you think of this?

[demiankatz]

Seems like isCollection might be the safest bet of all:

Suggested change

	return parent::isActive() && $this->getRecordDriver()->tryMethod('getHierarchyTrees');
	return parent::isActive() && $this->getRecordDriver()->tryMethod('isCollection');

Is there any reason not to do it this way?

[LuomaJuha]

CollectionList can be active on subrecords and seems like not all of them implements is_hierarchy_id which is used in isCollection function check. That is why i was thinking about if using getHierarchyTopID would be better for this occasion 🤔

[demiankatz]

Is it possible this is just a configuration issue? The hierarchy configuration controls which records are considered to be collections and which are not, based on the Solr index. If you have records that act like collections but do not have is_hierarchy_id set, that sounds like something non-standard, and I wonder if it would make more sense to customize record drivers than to introduce non-standard behavior to the record tab.

Bottom line: my feeling is that, at least for the core code, this tab is meant to operate on collections, so the best check we can apply is confirming that the record under consideration is a collection. Anything else feels like introducing different rules here than everywhere else, which doesn't make sense to me. But as I say, it's been a while since I've reviewed all this, so maybe I'm overlooking something -- my feeling, though, is that if something is wrong, the problem runs deeper than just this tab, and maybe needs to be addressed somewhere else.

[LuomaJuha]

Hmm, actually this comment clears up my thoughts. I think the isCollection check makes alot more sense now in this context 🤔

[demiankatz]

Great, thanks! I do think adding that check is an improvement, so if it works for you, I'll be happy to approve and merge this once the change is in place.

[demiankatz]

Thanks, @LuomaJuha!