sulu / SuluThemeBundle

MIT License
17 stars 15 forks source link

Image formats defined inside of theme directory are ignored #16

Open almare opened 4 years ago

almare commented 4 years ago
Q A
Bug? yes
New Feature? no
Sulu Version 2.0
Browser Version Browser name and version

Actual Behavior

At the moment the image-format.xml inside path/to//config/image-formats.xml is ignored.

Expected Behavior

As written inside the docu I would expect that the path is working.

Steps to Reproduce

Add file to path/to//config/image-formats.xml

Possible Solutions

If have extended the ImageFormatCompilerPass inside theme Bundle to load the theme path. But this is not enough, because the preview inside the admin also not working. I need to add a image-format.xml inside src/Resources/config/image-formats.xml to get the admin preview working.

axzx commented 4 years ago

you have to clear cache

almare commented 4 years ago

Thank you for your reply. I did it x-times and including rm var/cache. It started working afterchanging this code inside theme bundle.

class ImageFormatCompilerPass extends AbstractImageFormatCompilerPass
{
    /**
     * {@inheritdoc}
     */
    protected function getFiles(ContainerBuilder $container)
    {
        $files = [];

        $activeTheme = $container->get('liip_theme.active_theme');
        $bundles = $container->getParameter('kernel.bundles');
        $configPath = 'config/image-formats.xml';
        $rootDir = $container->getParameter('kernel.project_dir') ;
        foreach ($activeTheme->getThemes() as $theme) {

            $themePath =  $rootDir .'/themes/'. $theme .'/'. $configPath;
            if(file_exists($themePath)){
                $files[] = $themePath;
            }

            foreach ($bundles as $bundleName => $bundle) {
                $bundleReflection = new \ReflectionClass($bundle);
                $path = sprintf(
                    '%s/Resources/themes/%s/%s',
                    dirname($bundleReflection->getFileName()),
                    $theme,
                    $configPath
                );

                if (file_exists($path)) {
                    $files[] = $path;
                }
            }
        }
        return $files;
    }
}

So it has nothing to do with the cache. I think.

alexander-schranz commented 4 years ago

Seems like ImageFormatCompilerPass currently doesn't support themes inside projects and only supports bundles.

@sulu/core-team where should we store theme specific configurations in a project. I would think about something like config/themes/<theme_name>/image-formats.xml?

@almare you can still define all your image formats in the default config/image-formats.xml.

almare commented 4 years ago

root/config/image-formats.xml did also not work for me.

danrot commented 4 years ago

@alexander-schranz The path you have suggested makes sense to me :+1:

almare commented 4 years ago

Is this not part of the milestone 2.0?

danrot commented 4 years ago

I am open to accept a PR which fixes this, but since you should still be able to define your image formats in config/image-formats.xml, this has not a very high priority to me. Especially since the end result would be exactly the same, because the system merges all available image-formats anyway.

However, what bugs me more is that you've said that this is also not working for you. Did you add this to the config/image-formats.xml file that should already exist in your application beforehand and cleared the cache afterwards?

And I just realized that this might also be related to sulu/sulu#2639. If we allow to split the image-formats.xml file into multiple, and implement that in a way that the SuluThemeBundle just has to prepend some configuration, then we could even get rid of the CompilerPass in here.

@alexander-schranz And I am right with my assumption, that this issue should actually go into the SuluThemeBundle? If I see this correctly, this issue has to be fixed there.

danrot commented 4 years ago

@almare Oh, and where in the docs did you find some text that explains this should be possible? Just looked it up, and I couldn't find it.

SebTM commented 2 years ago

I came across this issue today and figured it out in the code that it's not intended like I understood it - so I came here to report/discuss about changing the docs or maybe a PR:

https://docs.sulu.io/en/latest/book/image-formats.html

Or when you use the SuluThemeBundle you can define the formats in your theme folder: path/to//config/image-formats.xml

combined with

By default, the bundle seeks for the themes in the %kernel.project_dir%/themes directory and looks for a theme configuration file named composer.json. This can be changed via the yaml configuration:

from the documentation here made me assume that "themes//config/image-formats.xml" would be a valid option as well. I'm open to provide a PR extending the current compiler-pass or adding a second one for this :)

niklasnatter commented 2 years ago

Do I understand this right the path path/to/<theme>/config/image-formats.xml does not work at the moment? If yes, it would be great if you could create a pull request for this!

NeuralClone commented 2 years ago

I recently started working with this bundle and trying to get this particular feature to work as described led me here.

Unless I'm missing something obvious, I can confirm that path/to/<theme>/config/image-formats.xml doesn't work at the moment. That directory is ignored by the compiler pass as it's currently written. It iterates through my themes but it only adds image formats from bundles.

Since themes found in %kernel.project_dir%/themes are correctly showing up as installed themes at this point in the code (along with other themes added to the repository), it may not require much to get this working.

websmurf commented 1 year ago

Had the same issue on my environment, created a pull request that fixes the issue for me: #34