Skip to content
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

Responsive images of a conversion from a collection changed in v10? #2778

Closed
mojowill opened this issue Feb 14, 2022 · 10 comments · Fixed by #2852
Closed

Responsive images of a conversion from a collection changed in v10? #2778

mojowill opened this issue Feb 14, 2022 · 10 comments · Fixed by #2852

Comments

@mojowill
Copy link

I've upgraded an application to L9 and Media Library 10 however the responsive images seem to be behave differently and are giving me an exception:

I have a blade component for a card which I pass in a video:

@php
    $thumbnail = $video->getFirstMedia('thumbnail');
@endphp

<div class="relative group">
    <div class="overflow-hidden w-full bg-gray-200 rounded-md shadow-md aspect-video group-hover:opacity-75">
        <div class="relative">

            <a href="{{ route('video.show', $video->id) }}">
                @if($thumbnail)
                    {{ $thumbnail('app-thumbnail') }}
                @else
                    <img src="{{ asset('images/video.png') }}" alt="Video Thumbnail">
                @endif
            </a>

<snip...>

This gets the first media from a collection called thumbnail and then should display the responsive images for the app-thumbnail conversion, worked fine on Laravel 8 and ML 9 however I now get

Found 1 error while validating the input provided for the GetObject operation: [Key] expected string length to be >= 1, but found string length of 0

I've dumped $thumbnail and can see the conversion available there.
Screenshot 2022-02-14 at 11 21 44

Has something changed with ML10 that I need to be aware of?

@mojowill
Copy link
Author

Adding out put from Ray as well which captures the exception. To add files etc are in place on S3 without issues and I can see the files have been generated and put in the correct place.

Screenshot 2022-02-14 at 11 55 56

@dimifontaine
Copy link

I get the exact same exception when using the getSrcset method on a media. It's working in Laravel 8 but failing in Laravel 9. Images are stored in S3 as well.

In a JsonResource I have:

$featured_media = $this->getFirstMedia('featured-images');
return [
    'featured_image'    => [
        'url' => $featured_media->getUrl('banner'),
            'srcset'    => $featured_media->getSrcSet('banner'),
             'title'     => $featured_media->getCustomProperty('title'),
             'alt'       => $featured_media->getCustomProperty('alt'),
    ],
]

Exception: Found 1 error while validating the input provided for the GetObject operation: [Key] expected string length to be >= 1, but found string length of 0

Here's the Laravel Forge's exception stacktrace if that can help.
Screenshot 2022-02-16 at 08 00 02.

@freekmurze
Copy link
Member

That strange... I'm a bit short on time to investigate this actively, but I would welcome a PR that fixes this.

@mojowill
Copy link
Author

mojowill commented Mar 4, 2022

Looking at this in more detail it seems that getBaseMediaDirectoryUrl() calls down to https://github.com/thephpleague/flysystem/blob/ea4542acf038d2ac37fa36df6a29a25f6dc41a63/src/PathPrefixer.php#L36 (eventually via laravel/framework/illuminate/AwsS3V3Adaptor.php) which strips out the forward slash? If I hardcode

return $this->client->getObjectUrl(
            $this->config['bucket'], $this->prefixer->prefixPath($path)
        );

to

return $this->client->getObjectUrl(
            $this->config['bucket'], $path
        );

It works fine?

@Mahegus
Copy link

Mahegus commented Mar 7, 2022

In the function getResponsiveImagesDirectoryUrl (DefaultUrlGenerator) line 34,

Just change the return statement
from
return Str::finish(url($base.$path), '/');
to
return Str::finish($this->getDisk()->url($path), '/');
and comment out
$base = Str::finish($this->getBaseMediaDirectoryUrl(), '/');

But this will not work after any updates, so just extend the class with your own.

@sebastiaanluca
Copy link

Can confirm @matt127127's solution works.

@freekmurze
Copy link
Member

@matt127127 Could you PR your solution?

@Mahegus
Copy link

Mahegus commented Mar 13, 2022

@matt127127 Could you PR your solution?

#2837

@sebastiaanluca
Copy link

I saw that the PR has been merged into v9. Will it be released as a fix for v10 too?

@freekmurze
Copy link
Member

Ah right, I still need to do this. If you need it fast, send a PR and I'll tag and release quickly 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants