Add qualified Dublin Core record driver.#5053
Add qualified Dublin Core record driver.#5053EreMaijala wants to merge 7 commits intovufind-org:devfrom
Conversation
EreMaijala
changed the title
EreMaijala
changed the title
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @EreMaijala! See below for a few suggestions.
| $userLocale = $this->localeSettings->getUserLocale(); | ||
| [$userLanguage] = explode('-', $userLocale); | ||
| if (null !== ($results = $localeResults[$userLocale] ?? $localeResults[$userLanguage] ?? null)) { | ||
| return $results; | ||
| } | ||
| // Check for matching language in locale-specific results: | ||
| foreach ($localeResults as $locale => $results) { | ||
| [$lang] = explode('-', $locale); | ||
| if ($lang === $userLanguage) { | ||
| return $results; | ||
| } | ||
| } |
There was a problem hiding this comment.
Would it make sense to extract this logic to a support function (like getBestLocaleMatch($locale, $localeResults) so that you can reuse it inside the foreach loop on lines 145-150? Otherwise, the default/fallback logic is less flexible than the other logic, since it doesn't have the fuzzy language matching piece.
There was a problem hiding this comment.
Yes. done. Also decided to move them to a trait for reusability.
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @EreMaijala, just one more thing!
| foreach ($localeResults as $locale => $results) { | ||
| [$lang] = explode('-', $locale); | ||
| if ($lang === $userLanguage) { | ||
| return $results; | ||
| } | ||
| } |
There was a problem hiding this comment.
In my earlier comment, I was suggesting including this logic as part of the getBestLocaleMatch method, so it could be used in both scenarios. Is there a reason not to do that? Even if we only want to do it some of the time, maybe moving it there and adding a $fuzzyLanguageFallback switch would make sense.
There was a problem hiding this comment.
Should be done, If I now understood properly. :)
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @EreMaijala, see below for a couple more minor things...
| } | ||
|
|
||
| // Check for matching language in locale-specific results: | ||
| [$language] = explode('-', $locale); |
There was a problem hiding this comment.
We don't need to calculate this twice.
| [$language] = explode('-', $locale); |
| */ | ||
| protected function getLocaleSpecificResults(array $localeResults, array|string $allResults): array|string | ||
| { | ||
| if (null === $this->localeSettings) { |
There was a problem hiding this comment.
Since this property is not defined by this trait, should we at least include a note in the header comment indicating that the trait expects the property (and recommending using it in combination with LocaleSettingsAwareTrait)?
If we're not expecting this scenario to ever happen, might it even be better to throw an exception if the dependency is missing? Otherwise, it may be hard to troubleshoot why things aren't working as expected.
2 participants
This pull request adds support for "qdc" record format with the SolrQdc driver. As it is it doesn't do a whole lot, just allows one to see an abstract on the Description tab. But it introduces a couple of new things I'd like feedback on before any further progress: