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

Setting default configuration (resolver) from another bundle #1042

Closed
silverbackdan opened this issue Jan 29, 2018 · 5 comments
Closed

Setting default configuration (resolver) from another bundle #1042

silverbackdan opened this issue Jan 29, 2018 · 5 comments

Comments

@silverbackdan
Copy link
Contributor

Q A
Bug Report? yes
Feature Request? no
BC Break Report? yes
RFC? yes
Imagine Bundle Version 2.0.x-dev

I have a question about this line:
https://github.com/liip/LiipImagineBundle/blob/2.0/DependencyInjection/Configuration.php#L55

It seems it causes an issue in my implementation which would require a workaround of defining my own custom resolver extending WebPathResolver to define the web_root and cache_prefix options.

I'm creating a bundle where the LiipImagineBundle is optional. If it exists I prepend the configuration which includes

// FooBundle/DependencyInjection/FooExtension::prepend
...
                'resolvers' => [
                    'default' => [
                        'web_path' => [
                            'web_root' => '%kernel.project_dir%/public',
                            'cache_prefix' => 'images/cache'
                        ]
                    ]
                ]
...

With the option performNoDeepMerging() on the resolver prototype, I cannot set the web_root or cache_prefix in this way as it reverts back to the default.

Is performNoDeepMerging required for another use case to prevent unwanted behaviour or could it be removed?

I think the same could be said for the loaders configuration although I'm not sure just yet.

@silverbackdan
Copy link
Contributor Author

silverbackdan commented Jan 29, 2018

After a little more time I can see it won't be straight forward to just extend the web path resolver.

I think the simplest quickest solution will be to add some configuration using Flex into the main app's configs.

I think the only real issue I have with the default config for SF4 is the web_root path and I imagine this is already on a to do somewhere to detect SF4 file structure and adjust the default web_root path.

I think the loaders config is working though when setting from another bundle.
^^ same for the filesystem loader

I think the recommended actions are to override this in a user's config right now. Perhaps it'd be better to test directories exist and adjust the default automatically?

@robfrawley
Copy link
Collaborator

The disabled deep merging was added by @makasim in f8f10ba, perhaps @makasim can elaborate on the reason for why it is required?

As for the web_root value, we are working on bringing the 2.x branch in-line with Symfony 4.x requirements, including auto-detection of public/versus web/ (see #1039 for this specific discussion).

@silverbackdan
Copy link
Contributor Author

Thanks @robfrawley - the PR looks as though it's progressing. I hope @makasim wouldn't mind elaborating or linking to a relevant discussion, I'll happily wait. Cheers.

@makasim
Copy link
Collaborator

makasim commented Jan 30, 2018

There must've been a reason. Unfortunately, I don't remember the detail.

@silverbackdan
Copy link
Contributor Author

silverbackdan commented Jan 30, 2018

OK no worries. I'm happy with the PR #1044 as the default behaviour which negates my needs to change this from another bundle.

I've had a think and I suppose in a re-usable bundle it's probably better for the final application to make all these configurations anyway for transparency, and the Imagine bundle being on Flex means they get a config file and I can put post-install instructions on my bundle if needed.

Thank you for the reply though @makasim and the support @robfrawley

Closing the issue now.

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