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

[1.0][Config] remove path option. #366

Merged
merged 1 commit into from
Mar 28, 2014

Conversation

makasim
Copy link
Collaborator

@makasim makasim commented Mar 27, 2014

No description provided.

@svscorp
Copy link

svscorp commented Mar 27, 2014

The question is why? Why the path has been removed?

I don't want to have hardcoded filtername in the cached image path:

// file path in WebPathResolver

%web_path% + / + %cache_prefix% + / + %filter_name% + / +  %source_path%

@makasim
Copy link
Collaborator Author

makasim commented Mar 27, 2014

Because dependency between resolvers and filters is a bad design IMO.

I dont think it is so important to have prefixes for generated content.

The filter name that is set to path is very descriptive IMO.

@havvg @lsmith77 @dbu please give your opinion about this PR.

@makasim
Copy link
Collaborator Author

makasim commented Mar 27, 2014

if you still need this feature you can rule it by configuring several resolvers: https://gist.github.com/makasim/9802395

@svscorp
Copy link

svscorp commented Mar 27, 2014

@makasim yeah, I have answered in the email. Let's bring it here.

I had this config in 0.*: http://pastebin.com/y4KGrBhv
,and cached images were stored here: %web_path% + / + %cache_prefix% + / + %filter_path%|%filter_name% + / + %source_path%

Now, using your example, I should do it like this: http://pastebin.com/ZXtwYMPK
, which becomes kind a big. And I can't achieve my goal anyway, because a filter (which is filterName actually) is presented in generated path. And I can not override this with my custom resolver, because I have already explicitly set resolvers in the options. My custom resolver from the services.yml become ignored.

With path it was flexible.

@svscorp
Copy link

svscorp commented Mar 27, 2014

Also, there is a lot of confusion with $filter variable. Somewhere it is a filter indeed. Somewhere it is a filterName (like in file path generation). I can change and PR that, but after current question will be resolved :)

@dbu
Copy link
Member

dbu commented Mar 27, 2014

i don't understand this enough to give a real opinion. i don't know how the bundle works exactly - would it help to allow to embed resolver config into the filter config? or is there something else that could make this more elegant? his example indeed looks problematic...

@svscorp
Copy link

svscorp commented Mar 27, 2014

Basically, I would like bundle users to have full control on path where cached image is being stored.

@makasim
I dont think it is so important to have prefixes for generated content.
The filter name that is set to path is very descriptive IMO.

Well, I think it is important to have flexibility in where do you store your generated content. For someone, who won't configure that, will use default stuff. But someone who want to have control - can't ;)

I think we need to collect more opinions as well.

@makasim
Copy link
Collaborator Author

makasim commented Mar 27, 2014

@ASKozienko wdyt?

@ASKozienko
Copy link

@makasim кому вдуть?
I know currently we are tied to something/what/is/called/path but we should move to something what is called id. And only ResolverInterface is aware how to generate appropriate url from image id.
@makasim 👍

@svscorp
Copy link

svscorp commented Mar 27, 2014

@ASKozienko so basically it's not 1.0 than until you implement something you mentioned with 'id', right?

And only ResolverInterface is aware how to generate appropriate url from image id.

If so, how does the users specify different cached location per filter in one resolver?

@ASKozienko
Copy link

@svscorp just don't care where cache is

@svscorp
Copy link

svscorp commented Mar 27, 2014

@ASKozienko who doesn't? I do care, for an instance :)

@ASKozienko
Copy link

@svscorp "liip/imagine-bundle": "<1.0" :)

@dbu
Copy link
Member

dbu commented Mar 27, 2014

i see why from the implementation we do not want to mix formats and caching. but i think the urls are important, as they face outside. what if there would be a configuration shortcut for the use case of svscorp?

@svscorp
Copy link

svscorp commented Mar 27, 2014

@dbu

what if there would be a configuration shortcut for the use case of svscorp?

Yes! It is the thing I am missing since path configuration parameter was removed from filter's configs. I think it is important to be flexible in where to store the cache. And if you are removing this (helpful) functionality to make a new major tag, I think it should be implemented with another (correct) way according to the code design we wanted.

It is a first time I see feature remove (without providing new implementation) in tag increment, that's why I am protecting it here :)

Thanks for fast responses guys, btw!

@svscorp
Copy link

svscorp commented Mar 27, 2014

@ASKozienko

yeah, or fork the project. But I would like to stay with original project better ;)

@makasim
Copy link
Collaborator Author

makasim commented Mar 27, 2014

@svscorp you can name the filter like preview/160x120. In this case the url will look even better:

http://localhost/media/cache/preview/160x120/foo/bar.jpeg

instead of

http://localhost/media/cache/preview_160/preview/160x120/foo/bar.jpeg

@makasim
Copy link
Collaborator Author

makasim commented Mar 27, 2014

@svscorp

And if you are removing this (helpful) functionality to make a new major tag, I think it should be implemented with another (correct) way according to the code design we wanted.

your statement is try to 1.x and any next releases but for 0.x. Quote from semantic versioning site. which we try to follow.

Major version zero (0.y.z) is for initial development. Anything may change at any time. The public API should not be considered stable.

@svscorp
Copy link

svscorp commented Mar 28, 2014

@svscorp you can name the filter like preview/160x120. In this case the url will look even better:
http://localhost/media/cache/preview/160x120/foo/bar.jpeg
instead of
http://localhost/media/cache/preview_160/preview/160x120/foo/bar.jpeg

Yeah, I know. It's more like a hacky, naming the filter as a partial path :)
But the whole path should be configurable as well as cache_prefix is configurable? Otherwise it doesn't make sense. Why not to hardcode the whole path? :)

Anyway, I understand I shouldn't refer to 0.* in 1.* discussion. But could you implement path configuration in 1.*?

@makasim
Copy link
Collaborator Author

makasim commented Mar 28, 2014

But could you implement path configuration in 1.*?

@svscorp there were several requests on same topic. I will implement when I have time not high priority though.

@svscorp
Copy link

svscorp commented Mar 28, 2014

@makasim I can do that. Just need to pick up the right strategy you guys will accept.

@makasim
Copy link
Collaborator Author

makasim commented Mar 28, 2014

@svscorp solution does not have to couple resolvers and filters, I am thinking of path generation strategy. The strategy maybe configured in several ways. Later inject to resolvers. I see here some problems, for example how to do the strategy which would satisfy all resolvers?

Personally I don`t come with any clean solution yet.

Second do we really need such overhead, for generated content?

@makasim
Copy link
Collaborator Author

makasim commented Mar 28, 2014

@havvg @lsmith77 still waiting for your response.

makasim added a commit that referenced this pull request Mar 28, 2014
@makasim makasim merged commit 93cbf43 into liip:master Mar 28, 2014
@makasim makasim deleted the remove-path-option branch March 28, 2014 19:31
@makasim
Copy link
Collaborator Author

makasim commented Mar 28, 2014

I merge this because path option is not used anywhere right now. To avoid any other confusing.

I see some interest in ability to customize cached paths. So this would be done later

@svscorp
Copy link

svscorp commented Mar 31, 2014

👍

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

Successfully merging this pull request may close these issues.

4 participants