Small enhancements to the DefaultRecord class #3279
Conversation
The german search index K10Plus Zentral stores title, title_sub, and title_short as multi-valued. This change to RecordDriver/DefaultRecord modifies the respective getters accordingly.
There was a problem hiding this comment.
Thanks, @dmj, this makes sense to me, but I have some suggested improvements to the approach.
| @@ -1210,7 +1210,14 @@ public function getSeries() | |||
| */ | |||
| public function getShortTitle() | |||
| { | |||
| return $this->fields['title_short'] ?? ''; | |||
| if (array_key_exists('title_short', $this->fields)) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It certainly is shorter but could be harder to understand. I can certainly live with both.
There was a problem hiding this comment.
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.
| @@ -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)) { | |||
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@dmj, it also looks like a |
|
@dmj, since there has been no activity here for a few months, I wanted to check with you on the status of this PR. I originally scheduled this for release 10.0, which should be completed in the next couple of months. Do you think we can finish this work in time for the release, or should we reschedule it for 10.1 or 11.0? |
|
I've merged the latest dev branch into the PR to get things up to date again. (There were no conflicts). |
|
Since this has not gotten completed in time, I am pushing it forward to the 11.0 milestone. |
These are two small enhancements to the DefaultRecord class that we would like to see in VuFind mainline. They make it easier for us to work the K10Plus Zentral search index.