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

Flysystem Loader does not support v2 #1308

Closed
JoshuaEstes opened this issue Nov 25, 2020 · 4 comments
Closed

Flysystem Loader does not support v2 #1308

JoshuaEstes opened this issue Nov 25, 2020 · 4 comments

Comments

@JoshuaEstes
Copy link

Upgrading to Flysystem v2 causes error

Uncaught PHP Exception TypeError: "Argument 2 passed to Liip\ImagineBundle\Binary\Loader\FlysystemLoader::__construct() must be an instance of League\Flysystem\FilesystemInterface, instance of League\Flysystem\Filesystem given, called in /app/var/cache/prod/ContainerMppLxC7/getLiipImagine_Binary_Loader_DefaultService.php on line 20" at /app/vendor/liip/imagine-bundle/Binary/Loader/FlysystemLoader.php line 33

Preconditions

  1. liip/imagine-bundle ref: d0819fc
  2. league/flysystem ref: 88ab4780a5cc573fb943c8a6987da880b3ec0474
  3. Symfony 5.1.8

Steps to reproduce

  1. Using Symfony, install the flysystem v2 and apply the twig filter

Expected result

  1. Shouldn't get any errors

Actual result

  1. Just crashes

The current workaround I have is a Custom FlysystemLoader.

References:

@dbu
Copy link
Member

dbu commented Nov 27, 2020

i was wondering why this can happen, but found that flysystem is an optional dependency in composer.json. we currently only test with flysystem 1.

can you do a pull request that adjusts the flysystem in require-dev to allow version 1 or 2? if there are any tests, we should see if it works.

the error you get is surpising me though. why would the Filesystem class not implement the FilesystemInterface? glad if you can investigate and provide a fix pull request.

@JoshuaEstes
Copy link
Author

The upgrade to Flysystem v2 doesn't have a "FilesystemInterface". There are also some method changes. I'm not 100% sure what the best solution would be right now. Either create a "v2" class or check if the FilesystemInterface exists. Could be another option.

Here's what I did.

<?php

namespace App\ImagineBundle\Binary\Loader;

use League\Flysystem\Filesystem;
use Liip\ImagineBundle\Exception\Binary\Loader\NotLoadableException;
use Liip\ImagineBundle\Exception\InvalidArgumentException;
use Liip\ImagineBundle\Model\Binary;
use Liip\ImagineBundle\Binary\Loader\LoaderInterface;
use Liip\ImagineBundle\Binary\BinaryInterface;
use Symfony\Component\HttpFoundation\File\MimeType\ExtensionGuesserInterface as DeprecatedExtensionGuesserInterface;
use Symfony\Component\Mime\MimeTypesInterface;

class FlysystemLoader implements LoaderInterface
{
    /**
     * @var Filesystem
     */
    protected $filesystem;

    /**
     * @var MimeTypesInterface|DeprecatedExtensionGuesserInterface
     */
    protected $extensionGuesser;

    /**
     */
    public function __construct(MimeTypesInterface $extensionGuesser, Filesystem $filesystem)
    {
        if (!$extensionGuesser instanceof MimeTypesInterface && !$extensionGuesser instanceof DeprecatedExtensionGuesserInterface) {
            throw new InvalidArgumentException('$extensionGuesser must be an instance of Symfony\Component\Mime\MimeTypesInterface or Symfony\Component\HttpFoundation\File\MimeType\ExtensionGuesserInterface');
        }

        if (interface_exists(MimeTypesInterface::class) && $extensionGuesser instanceof DeprecatedExtensionGuesserInterface) {
            @trigger_error(sprintf('Passing a %s to "%s()" is deprecated since Symfony 4.3, pass a "%s" instead.', DeprecatedExtensionGuesserInterface::class, __METHOD__, MimeTypesInterface::class), E_USER_DEPRECATED);
        }

        $this->extensionGuesser = $extensionGuesser;
        $this->filesystem       = $filesystem;
    }

    /**
     * {@inheritdoc}
     */
    public function find($path)
    {
        if (false === $this->filesystem->fileExists($path)) {
            throw new NotLoadableException(sprintf('Source image "%s" not found.', $path));
        }

        $mimeType = $this->filesystem->mimetype($path);

        $extension = $this->getExtension($mimeType);

        return new Binary(
            $this->filesystem->read($path),
            $mimeType,
            $extension
        );
    }

    private function getExtension(?string $mimeType): ?string
    {
        if ($this->extensionGuesser instanceof DeprecatedExtensionGuesserInterface) {
            return $this->extensionGuesser->guess($mimeType);
        }

        if (null === $mimeType) {
            return null;
        }

        return $this->extensionGuesser->getExtensions($mimeType)[0] ?? null;
    }
}
# config/services.yaml
# ...
    App\ImagineBundle\Binary\Loader\FlysystemLoader:
        arguments: ['@mime_types', '@League\Flysystem\Filesystem']
        tags:
            - { name: 'liip_imagine.binary.loader', loader: flysystemV2 }

@weaverryan
Copy link
Contributor

It looks like supper for v2 has been merged - we just need a tag :)

@dbu
Copy link
Member

dbu commented Oct 6, 2021

the tag happened (a while ago) https://github.com/liip/LiipImagineBundle/releases/tag/2.6.0

@dbu dbu closed this as completed Oct 6, 2021
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

3 participants