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
Fix CoverController::getImageParams isbn array check #2800
Fix CoverController::getImageParams isbn array check #2800
Conversation
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, @LuomaJuha! I'm copying @xmorave2 into this thread in case he has any insights. I thought I had done end-to-end testing of this, but maybe I did something incorrectly or overlooked a detail. It certainly seems like you've found a gap in the code. I also have a couple of questions about implementation details (see below).
continue; | ||
} | ||
if (is_array($isbn)) { | ||
$isbn = reset($isbn); |
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.
Why is this reset necessary?
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.
Why do you reduce the ISBNs to first one only? We probably want to pass all available ISBNs to cover loader.
So we want something like this instead:
$isbns = $params()->fromQuery($identification);
if (empty($isbns)) {
continue;
}
if (!is_array($isbns)) {
$isbns = [$isbns];
}
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.
Now it should convert them into an array as default. And keep all the array values.
return [ | ||
// Legacy support for "isn" param which has been superseded by isbn: | ||
'isbn' => $params()->fromQuery('isbn') ?: $params()->fromQuery('isn'), | ||
'isbn' => $isbn, |
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 think the VuFind\Cover\Loader is now expecting an 'isbns' key instead of singular 'isbn' in order to take advantage of multiple values. I wonder if 'isbns' => (array)$isbn
would be the best way to approach this.
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.
Yes we need isbns
param instead of isbn
.
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.
isbns it is now.
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 @LuomaJuha for catching this! It's definitely a bug and my oversight from #2633.
continue; | ||
} | ||
if (is_array($isbn)) { | ||
$isbn = reset($isbn); |
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.
Why do you reduce the ISBNs to first one only? We probably want to pass all available ISBNs to cover loader.
So we want something like this instead:
$isbns = $params()->fromQuery($identification);
if (empty($isbns)) {
continue;
}
if (!is_array($isbns)) {
$isbns = [$isbns];
}
return [ | ||
// Legacy support for "isn" param which has been superseded by isbn: | ||
'isbn' => $params()->fromQuery('isbn') ?: $params()->fromQuery('isn'), | ||
'isbn' => $isbn, |
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.
Yes we need isbns
param instead of isbn
.
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 quick update @LuomaJuha. I do some hand on testing using ObalkyKnih driver with ajax covers disabled and it is working fine.
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.
Looks good! Thanks, @LuomaJuha and @xmorave2!
I think this was forgotten in #2633 but the parameters do not work properly when trying to fetch an image to show.