-
Notifications
You must be signed in to change notification settings - Fork 374
Fix browzine coverimage load for primo backend. #4720
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
base: dev
Are you sure you want to change the base?
Conversation
Primo backend only returns isbn but not issn, browzine coverimage loader needs issn to get the cover image.
demiankatz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sharing this! In order to fix the error in GitHub Actions, please run composer fix and commit the results; that should adjust the minor style issues that are causing the problem.
I'm also requesting a review from @EreMaijala, since he is more familiar with Primo than me and may have additional feedback.
|
Also, Booksite needs isbn to be used for cover image loading. Not sure if we need to change isn here to isbn or add another isbn. vufind/module/VuFind/src/VuFind/Content/Covers/Booksite.php Lines 70 to 91 in 260d485
vufind/module/VuFind/src/VuFind/Content/Covers/BrowZine.php Lines 59 to 83 in 260d485
|
| return ['size' => $size, 'contenttype' => 'JournalArticle']; | ||
|
|
||
| if (empty($params['isbn']) && empty($params['issn'])) { | ||
| $params['contenttype'] = 'JournalArticle'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't contenttype still be always set? As far as I can see it's only used by cover loader's fetchFromContentType method, but Summon driver sets it from format. Perhaps the same could be done here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree: it might be preferable to set the content type by mapping format values if we want to be more consistent.
I'm not sure if the Booksite service/API even still exists; that integration was contributed many, many years ago and I don't think anyone has been able to test it since then. It's certainly possible that code fell out of date during that time. If you want to try to fix it in a separate PR, you're welcome to, but I'm not sure how to confirm that the fix makes a difference. |
It's right that this booksite service is inaccessible this year, and the last time seen on wayback machine is 2024 DEC. Maybe this backend could be removed someday in the future. |
I have opened #4736 to clean this up. Thanks for doing the investigation! (And on this present PR, I am waiting for @EreMaijala to re-review before taking further action; he is out of office today but should return next week). |
Primo backend only returns isbn but not issn, browzine coverimage loader needs issn to get the cover image.