Liipimaginebundle: [BUG] 1.7.2 One FileSystemLocator for several FileSystemLoader

[sebastianblum]

| Q | A
| --- | ---
| Bug Report? | yes
| Feature Request? |no
| BC Break Report? | no
| RFC? | no
| Imagine Bundle Version | 1.7.2

Hello,
with release 1.7.2 our implementation is broken.

We use several FileSystemLoaders to hide the directories we store our images and to shorten the image urls. We have these config:

liip_imagine:
  resolvers:
    default:
      web_path:
        web_root: "%kernel.root_dir%/../web"
        cache_prefix: media

  loaders:
    assets:
      filesystem:
        data_root: "%kernel.root_dir%/../web/assets/"
    default:
      filesystem:
        data_root: "%kernel.root_dir%/../web"
    guide:
      filesystem:
        data_root: "%kernel.root_dir%/../web/assets/guide"
    news:
      filesystem:
        data_root: "%kernel.root_dir%/../web/assets/news"
    press:
      filesystem:
        data_root: "%kernel.root_dir%/../web/assets/press"

With version 1.7.2, you introduced one FileSystemLoader service for all FileSystemLoader.

https://github.com/liip/LiipImagineBundle/blob/1.0/Binary/Locator/FileSystemLocator.php#L28
will be executed in our case 5 times with
"%kernel.root_dir%/../web/assets/",
"%kernel.root_dir%/../web",
"%kernel.root_dir%/../web/assets/guide",
"%kernel.root_dir%/../web/assets/news",
"%kernel.root_dir%/../web/assets/press"

and only the last configuration ("%kernel.root_dir%/../web/assets/press") is stores in $this->roots attribute.

In my opinion, we need one FileSystemLocator each FileSystemLoader.

[robfrawley]

@sebastianblum New in 7.x you can pass an array of root paths to a single loader; you can skip all the complication of calling separate loaders simply to shorten your paths.

liip_imagine:
  loaders:
    all:
      filesystem:
        data_root: 
          - "%kernel.root_dir%/../web"
          - "%kernel.root_dir%/../web/assets/"
          - "%kernel.root_dir%/../web/assets/guide"
          - "%kernel.root_dir%/../web/assets/news"
          - "%kernel.root_dir%/../web/assets/press"

That said, this is definetly a bug that requires attention.

[sebastianblum]

hello @robfrawley thank you very much for your answer.

We used another workaround at the moment because we have sometimes the same image name in different folders.

Can you discuss the solution here in the issue, then I can solve the issue with a pull request.

at the moment, I see two possibilities:

1. One global FileLocator instance and we inject the loaderName, so
   $this->roots = ["path1", "path2"] will become
   $this->roots = [ "default" => ["path1", "path2"], "other" => ["path3"] ]

or

1. Single FileLocator instance (with same implementation) for every FileLoader

What would you prefer?

[robfrawley]

@sebastianblum Originally, I was playing with discrete *Locator instances for each FileSystemLoader. It's a bit complicated to support Symfony <2.8 and >=2.8 due to the incompatible changes between the two to allow "non shared" services (instantiated each time they are requested). I was playing with a compiler pass like the following:

<?php

/*
 * This file is part of the `liip/LiipImagineBundle` project.
 *
 * (c) https://github.com/liip/LiipImagineBundle/graphs/contributors
 *
 * For the full copyright and license information, please view the LICENSE.md
 * file that was distributed with this source code.
 */

namespace Liip\ImagineBundle\DependencyInjection\Compiler;

use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Definition;

class LocatorsCompilerPass extends AbstractCompilerPass
{
    /**
     * {@inheritdoc}
     */
    public function process(ContainerBuilder $container)
    {
        foreach ($container->findTaggedServiceIds('liip_imagine.binary.locator') as $id => $tags) {
            $this->setDefinitionNotShared($container->getDefinition($id));
        }
    }

    /**
     * @param Definition $definition
     */
    private function setDefinitionNotShared(Definition $definition)
    {
        if (method_exists($definition, 'setShared')) {
            // symfony >=2.8
            $definition->setShared(false);
        } else {
            // symfony <2.8
            $definition->setScope('prototype');
        }
    }
}

But I'm open to discussion and ideas, and not settled on any specific solution.

[robfrawley]

@sebastianblum Can you test #875 and report whether it resolves your issue? You patch your vender copy of this bundle by doing something like the following.

# install the `master` from source
cd vendor/liip; rm -fr imagine-bundle
git clone https://github.com/liip/LiipImagineBundle.git imagine-bundle
cd imagine-bundle

# apply this PR as a patch
curl -L https://github.com/liip/LiipImagineBundle/pull/875.diff | patch -p1

[sebastianblum]

Hello @robfrawley,

Yes, great it works.
Your code looks very good. The SymfonyFramework class is brilliant!

I created one comment direct in your pull request.

Thank you very much for your help and this pull request.
sebastian

[robfrawley]

Solved via #875 which has been merged into 1.0. The next release to include this bugfix will be 1.7.3, scheduled for release in the first half of this week.