Create a new cover loader for EDS to proxy the image provided from the EDS API by meganschanz · Pull Request #5542 · vufind-org/vufind

meganschanz · 2026-08-20T20:45:40Z

This is following up on the Slack conversation that started on August 18th with the idea to switch how EDS cover images are loaded. To instead use a cover loader instead of putting the image URL returned from the EDS API directly in the template for the client side to request.

The benefits of making a new cover loader for EDS:

Increased privacy for end users since the image requests now come from the VuFind server instead of individual user IPs (which we can’t be sure if those image services track/store)
No need to configure additional content security policy exceptions for any possible image service used by EBSCO for images or have a broad img-src * which would be a security concern

This makes the images available at Cover/Show?recordid={{eds-record-id}}&size={{size}}&source=EDS, for example: Cover/Show?recordid=cat09276a%2Cebc.ebs29904633e&size=medium&source=EDS.

To test:

Update the EDS.ini to include valid EBSCO_Account information
Update the config.ini to enable EDS within coverimages
Perform test searches (starting from EDS/Home) or view some record pages (/EdsRecord/nlebk,401631/)

Note:

We did consider validating the URLs via a head request to make sure it returned a valid response, but decided to remove that to save the extra call. But that does mean if there are invalid URLs returned from EDS then we won't have an image for them (example: EdsRecord/cat09276a,ebc.ebs29904633e)

…e EDS API

demiankatz

Thanks, @meganschanz -- see below for a few thoughts on this.

I'd suggest that we wait to hear more from @cwolfebsco about the policy side of things before investing more time, though -- if it's a no-go due to EBSCO rules, then there's no point in wasting time on polishing... but I thought a review might be helpful for future reference should we get the green light to proceed.

demiankatz · 2026-08-21T13:06:00Z

+        $recordId = $ids['recordid'] ?? '';
+        $url = $this->getCachedData($recordId);
+
+        if (str_starts_with($url, 'http://') || str_starts_with($url, 'https://')) {


Should you ensure that $url is a string before performing these string operations? What happens if the cache is empty?

Good point. I changed how that check is done.

demiankatz · 2026-08-21T13:06:35Z

+        $url = $this->getCachedData($recordId);
+
+        if (str_starts_with($url, 'http://') || str_starts_with($url, 'https://')) {
+            $this->debug('Returning Cover image URL: ' . $url);


Maybe helpful to specify the source, in case there are multiple cover handlers in play.

Suggested change

$this->debug('Returning Cover image URL: ' . $url);

$this->debug('Returning EDS cover image URL: ' . $url);

I applied the suggestion. The only reason I didn't in the first place was I worried the message was too repetitive since the class name is (always?) in the message too:
vufind.DEBUG: VuFind\Content\Covers\EDS: Returning EDS cover image URL: https://some-url

demiankatz · 2026-08-21T13:09:45Z

 class EDS extends DefaultRecord
 {
    use Feature\IlsAwareTrait;
+    use \VuFind\Http\CachingDownloaderAwareTrait;


You're not using the caching downloader, so why are you using this trait?

I am not; good catch. That was leftover from when I was experimenting with caching the images. I removed it.

demiankatz · 2026-08-21T13:10:23Z

+    \VuFind\Http\CachingDownloaderAwareInterface
+{
+    use \VuFind\Log\LoggerAwareTrait;
+    use \VuFind\Http\CachingDownloaderAwareTrait;


This trait seems unnecessary here as well (but sorry if I'm overlooking something!).

Yup, same as above. I removed it.

demiankatz · 2026-08-21T13:12:23Z

        // for certain ebook packages.
-        if ($closestMatch) {
-            return $closestMatch;
+        if ($thumbnail) {


Would there be value in adding an EDS.ini setting to control whether thumbnails are returned directly or through the cache? Some might prefer the old approach, and it doesn't look like adding flexibility would be too difficult.

Good idea. I made a new setting under [Cover] called loadDirectly and set it to true by default to be consistent with existing functionality. The comment indicates that when it is false the coverimages setting in the config.ini also needs to be updated to include EDS. I'm open to other names for the setting or suggestions to improve the comment around it.

demiankatz · 2026-08-21T13:13:25Z

-        if ($closestMatch) {
-            return $closestMatch;
+        if ($thumbnail) {
+            $this->putCachedData($this->getUniqueID(), $thumbnail);


As @cwolfebsco asked on Slack, can we set the lifetime here based on the headers of the API response? (I imagine that may be non-trivial and might require injecting the data to pass it down this far into the code -- but at least worth considering).

I don't think we need to do this now considering we're not going to cache the images, and just the URL, right? The cache already has a lifetime based on the config.ini -> [Cache] -> ttl time.

demiankatz · 2026-08-21T13:19:18Z

+     * creating a service.
+     * @throws ContainerException&\Throwable if any other error occurs
+     */
+    public function __invoke(


I wonder if this factory should be simplified or made to extend one of the other existing driver factories -- but the best approach may depend on the answer to the EPF issue.

It didn't seem like any of the existing driver factories injected the \VuFind\Cache\Manager already. But working with the factories is not my strong-suite, so there certainly may be a way to simplify it that I'm just not seeing. And yes we do need to keep in mind that both EDS and EPF will be using it.

cwolfebsco · 2026-09-01T05:38:36Z

@demiankatz & @meganschanz: Thanks for your patience on this. As you know, I wanted to make sure I had a clear answer from Product Management before responding.

Cover images provided for catalog enrichment or eBooks as part of EDS API are licensed for use within EDS API results only. We're not able to grant permission to cache these images, I am afraid.

Happy to discuss further if it would help to understand the constraints in more detail.

demiankatz · 2026-09-01T20:50:51Z

@demiankatz & @meganschanz: Thanks for your patience on this. As you know, I wanted to make sure I had a clear answer from Product Management before responding.

Cover images provided for catalog enrichment or eBooks as part of EDS API are licensed for use within EDS API results only. We're not able to grant permission to cache these images, I am afraid.

Happy to discuss further if it would help to understand the constraints in more detail.

Thanks for the clarification, @cwolfebsco!

So it sounds like we definitely would have to turn off the property in the cover loader that caches images.

It looks like the fundamental functionality of the cover loader doesn't depend on caching the actual image, though, just on caching the image url. Since the loading is accomplished by record ID, and the EDS record IDs will never be used to load images outside the context of EDS results, it sounds to me like as long as we don't locally cache the image files themselves, the other aspects of the solution may be within bounds -- unless running the images through a proxy server is an inherent violation, in which case this solution is simply unworkable.

meganschanz added 2 commits August 20, 2026 16:11

Create a new cover loader for EDS to proxy the image provided from th…

caf6bef

…e EDS API

Fix lint and test issues from previous commit

1a02004

demiankatz requested changes Aug 21, 2026

View reviewed changes

meganschanz added 5 commits September 2, 2026 09:04

Merge remote-tracking branch 'core/dev' into eds-covers

f8b2f8e

Updates to EDS cover loader based on initial feedback

ae7073e

Case correction to log message

6647e7a

Update default of loadDirectly to true

7dde2dd

Update test to check both scenarios of loadDirect in EDS Cover config

4d1e0c7

	$this->debug('Returning Cover image URL: ' . $url);
	$this->debug('Returning EDS cover image URL: ' . $url);

Uh oh!

Conversation

meganschanz commented Aug 20, 2026

Uh oh!

demiankatz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cwolfebsco commented Sep 1, 2026

Uh oh!

demiankatz commented Sep 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants