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

Huge delay with imageMeta when running tests #25

Open
HassanZahirnia opened this issue Dec 11, 2022 · 3 comments
Open

Huge delay with imageMeta when running tests #25

HassanZahirnia opened this issue Dec 11, 2022 · 3 comments

Comments

@HassanZahirnia
Copy link

Hey 👋

I use spatie/laravel-medialibrary for managing files including images. And I noticed when I'm running tests there is a huge stall (around 5-6seconds) on the imageMeta creation. I pass the getFirstMediaUrl to the SEOData and it works fine in browser since it returns a full url to the image (like: http://www....)

view()->share('SEOData', new SEOData(
     image: $category->getFirstMediaUrl(Disk::CategoryPictures),
));

But during tests since the storage is faked, the getFirstMediaUrl returns a local path like /storage/1/.... and this seem to confuse this package's imageMeta() inside SEOData class and the test would get delayed for many seconds until it finally passes.

I wonder if this is something that can be fixed or taken into account for such scenario ?
Or is there a different approach I should take for passing the image path. I'm asking this since spatie/laravel-medialibrary is a popular package and people probably might end up with same pitfall.

Thanks in advance and thank you for your great package 😊🙌

@ralphjsmit
Copy link
Owner

Hey Hassan! Thanks a lot for your detailed and thoughtful issue. I totally agree testsuites should be fast and this should not take 5-6 seconds. 🙂

I think that it has to do with the function getimagesize() in the ImageMeta class. Could you try both the following:

  1. Update your code, so that it passes the path and not the url. Should be something like this:
$category->getFirstMedia(Disk::CategoryPictures)->getPath();
  1. Go to the ImageMeta class in your vendor files, add the following code and share with me the results:
<?php

namespace RalphJSmit\Laravel\SEO\Support;

use Exception;

class ImageMeta
{
    public ?int $width = null;

    public ?int $height = null;

    public function __construct(string $path)
    {
        $publicPath = public_path($path);

        if ( ! is_file($publicPath) ) {
            dump("Not a file");
            report(new Exception("Path {$publicPath} is not a file."));

            return;
        }

        [$width, $height] = getimagesize($publicPath);
        dump($width, $height);

        $this->width = $width;
        $this->height = $height;
    }
}

Thanks!

@HassanZahirnia
Copy link
Author

HassanZahirnia commented Dec 12, 2022

Hey! Thanks for your quick reply. 😄
I applied your instructions and I got the "Not a file" message.

I want to point out that spatie/laravel-medialibrary returns a full disk path to it's media. It does not return or store path relative to the public directory, which I know is what your package is designed to use. So there is that. And I looked into laravel-medialibrary docs and I found no way to return a relative path.

As for now, I found a workaround for this problem. I did some investigations and found out laravel-medialibrary uses the disk url config to generate the media url. And as you know Laravel's Storage::fake() creates a dummy disk with only one config property and that is the root path so it's missing lots of properties that a normal disk in config/filesystems.php would do.

So I added a second parameter to the Storage::fake() and adjusted it like below:

Storage::fake(Disk::CategoryPictures, [
    'url' => config('app.url') . '/storage',
]);

Now the test will run fine with normal speeds ✅
Although I'm not sure if this is considered a "hack" or it's actually how a proper storage fake should look like.

@ralphjsmit
Copy link
Owner

ralphjsmit commented Dec 13, 2022

Hey @HassanZahirnia! Hmm, I guess a good solution, but indeed slightly hacky. But it is a good fake. For now I guess that it would be nice, but I could add a method that would allow you to generate URLs based on paths (and vice versa). I don't have time to do that this year, so you might want to use the current solution for now. Thanks! 🙂

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

No branches or pull requests

2 participants