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.

Sign up for GitHub

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

Small enhancements to the DefaultRecord class #3279

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

Small enhancements to the DefaultRecord class #3279

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6f57724
Handle multi-valued title fields
Jan 2, 2024
de1b3a8
Map electronic article to OpenURL article
Jan 2, 2024
220c76d
Check if title{_sub,_short} exists before accessing it
Jan 2, 2024
5dae291
Use more concise code returning short and sub title
Jan 4, 2024
cc81f9f
Merge branch 'dev' into t/enhance-default-record
demiankatz Apr 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 26 additions & 5 deletions module/VuFind/src/VuFind/RecordDriver/DefaultRecord.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -734,9 +734,9 @@ protected function getOpenUrlFormat()
$formats = $this->getFormats();
if (in_array('Book', $formats) || in_array('eBook', $formats)) {
return 'Book';
} elseif (in_array('Article', $formats)) {
} elseif (in_array('Article', $formats) || in_array('electronic Article', $formats)) {
Copy link
Member

Choose a reason for hiding this comment

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

Rather than adding more hard-coded values here, what if you add new class properties for $journalFormats and $articleFormats and then do an array_intersect to see if they are present? This way, we can leave the default values at the current ['Journal'] and ['Article'], but it's very easy to adjust in a local subclass for other situations (or we might even be able to make it configurable somewhere).

I would prefer not to add new format strings here that are not found in the language files or anywhere else, as I think that could lead to confusion... though if you think that's the only way forward, I'm certainly willing to discuss further!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. Hardcoded values are a quick'n'dirty solution.

It'll think about it, there might be a longer development arc: It should be possible to separate the serialization of a record (OpenURL, Schema.org, XML, Citations) from the RecordDriver.

Copy link
Member

Choose a reason for hiding this comment

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

I'll leave this open until we can find a generic solution -- though if defining properties to hold the lists is a workable short-term solution for you, we could do that now and still work toward a more robust longer-term solution.

While I support separation of concerns where practical, I think the main challenge to figure out is how to deal with the various format-specific special cases and limitations if we have an external serializer. I'd rather have that logic in the record drivers than have a single monolithic class riddled with special cases, as I think that's much less extensible and flexible. But maybe there's a middle ground where we build a serialization mechanism that can have driver-specific plugins, so we retain the current degree of flexibility but also reduce the size of individual classes. Or maybe you have something entirely different in mind! But, just as one very specific example, I think the current system where each record driver knows which formats of XML it can return through getXML is better than a system where we have a single XmlGenerator class that needs to somehow figure out when it can do Dublin Core and when it can do MARC-XML, etc., etc.

return 'Article';
} elseif (in_array('Journal', $formats)) {
} elseif (in_array('Journal', $formats) || in_array('eJournal', $formats)) {
return 'Journal';
} elseif (strlen($this->getCleanISSN()) > 0) {
// If the record has an ISSN and we have not already
Expand Down Expand Up @@ -1210,7 +1210,14 @@ public function getSeries()
*/
public function getShortTitle()
{
return $this->fields['title_short'] ?? '';
if (array_key_exists('title_short', $this->fields)) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd consider simplifying this method (and the equivalent below) to something like:

$titles = (array)($this->fields['title_short'] ?? []);
return $titles[0] ?? '';

Pretty sure this has the same outcome in less code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It certainly is shorter but could be harder to understand. I can certainly live with both.

Copy link
Member

Choose a reason for hiding this comment

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

If you think it's too hard to understand, we could introduce a support method like protected getFirstValueFromField($fieldName) and put the logic in there... and then the other methods could call it. That would make the individual methods even shorter, and the behavior of the two-line logic would be self-documenting based on the method name.

if (is_array($this->fields['title_short'])) {
$title = $this->fields['title_short'][0];
} else {
$title = $this->fields['title_short'];
}
}
return $title ?? '';
}

/**
Expand All @@ -1231,7 +1238,14 @@ public function getSource()
*/
public function getSubtitle()
{
return $this->fields['title_sub'] ?? '';
if (array_key_exists('title_sub', $this->fields)) {
if (is_array($this->fields['title_sub'])) {
$subtitle = $this->fields['title_sub'][0];
} else {
$subtitle = $this->fields['title_sub'];
}
}
return $subtitle ?? '';
}

/**
Expand Down Expand Up @@ -1336,7 +1350,14 @@ public function getThumbnail($size = 'small')
*/
public function getTitle()
{
return $this->fields['title'] ?? '';
if (array_key_exists('title', $this->fields)) {
if (is_array($this->fields['title'])) {
$title = $this->fields['title'][0];
} else {
$title = $this->fields['title'];
}
}
return $title ?? '';
}

/**
Expand Down