-
Notifications
You must be signed in to change notification settings - Fork 166
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
Major Refactoring #25
Conversation
… prefix is set to be empty
- added support for converting to different formats - added ability to leave off the format (or set it to .orig) - added ability to disable caching - added ability to set a custom loader (fixes #21) - added ability to configure quality
One approach to solve the cache path resolver dilemma, would be to inject the request optionally into the controller as well and then pass it as a parameter in the resolve() method. And like I said on twitter, once you guys agree with the general concept the next step will be to fix up the tests. |
…al image (useful to be able to read the image from a different place than the public web root)
… the CachePathResolver does not need to be requested scoped (allows reverting the changes to the templating classes)
I don't think I like the json format here, maybe it is best for you to fork the bundle and customize it to ur needs. The idea of extracting a common image loader interface and providing a default filesystem one and ways to register more sounds great, but you have submitted a lot more and it takes a lot of effort to even review this |
Sure I can throw out the json stuff, though I think its quite useful for modern apps. For example we use this to drive the captions inside a slideshow. As for the rest, yes once I started working on this I noticed I needed a lot more features than I anticipated. Though several are quite related. Separating these features however would have meant a ton of extra work, since I had to move a lot of logic to make my original goal possible. I guess for now I would just like to hear if you are comfortable with the general direction I took of moving a lot of logic out to existing and new services. But for now I will just use this fork in my project, so no hurry .. |
I will work on custom loaders and see how much of your original pull request I can apply, although as you can imagine it will take a lot of effort to separate that logic. The goal of the bundle is to provide easy way to process images dynamically with sensible defaults and caching. So the goal of supporting multiple image loading and caching mechanisms is well in the scope of the bundle, anything else, like json representation of images or custom filters is not. I'll ping u once I get to implementing what I just mentioned, I'm glad you can use the fork as is for now. Thanks for moving this, |
The code for json handling is quite small, which is the only feature I think could be questionable for this bundle. What do you mean with "custom filters"? |
I mean thumbnail filter that uses only one side for resizing for example, this is exactly the reason bundle comes with flexible filter loading mechanism that should let you define your own filter loaders |
Hmm why wouldn't you want to support this in the thumbnail filter? Anyway, I guess I will wait until you are done and see if I will just maintain a fork or if I can reasonable put all the features I need into separate classes and/or a bundle that extends yours. |
Conflicts: Controller/ImagineController.php Imagine/Filter/Loader/ThumbnailFilterLoader.php
ok .. i did some more refactoring and removed the support for json/xml. i now have a custom controller in my app that adds this using the FOSRestBundle but since the controller is now a lot simpler it took much code that i now have to maintain in my custom controller. |
why did u make caching optional? |
Did another change to the filter loader interface which made it easier to disable upscaling in the thumbnail filter. its really a pitty that we do not yet have an agreement on the general refactoring because this means i cannot really donate any other features i am adding either (unless of course i start reimplementing them again on top of the current code base which i admittedly am not really inclined to do since i think the old approach was just super monolithic). anyway .. i guess my changes will continue to flow in here for anyone to use .. as soon as you start to make any large changes that i also want, i will have to see what i will do then .. |
Added some tests and gmagick
Thx for this upgrade @lsmith77. |
just checked .. the DI extension tests are still passing .. |
Format and quality handling in the configuration is a must. Ommiting one dimension in thumbnail filter is also nice and useful. I think this PR is great quality change. Nice work @lsmith77 ! One thing I don't understand. What's the benefit of passing @avalanche123 will this be merged any time soon? |
@MHeleniak: that change was done as the cache resolver is now optional but required the request service. however i have since found a better solution. as you can see the current version of the code dynamically injects the cache resolver only when needed. |
Here is what is necessary to make this mergeable:
Given that the above points are fixed, I can review the pull and provide more feedback. Otherwise, I don't understand or disagree with the changes proposed. I realize that a more flexible caching and image loading interface is needed, but I'm not considering changing more than that. In the meantime, feel free to use the fork. |
If you're concerned with the way the default provided filter works, then I should emphasize that it is provided as an example of how to create and use your own filter, much like the basic filters that ship with Imagine, the idea here is that you'd make your own filters that include all the necessary transformations. Later we could start Filter contributions repository to list oss filters |
I understand your concerns, but don't think its feasible for me to comply. While I agree that the diff is hard to read, I think if you look at it as is, its actually quite easy to review. So I guess we are at an impasse here. That being said, maybe someone else who hasn't spend many hours on these changes is motivated to comply with your wishes. Otherwise for now I think it will be more feasible to maintain a fork. |
@lsmith77 One point is that some changes are simply changing the order in which the properties of the classes are declared |
yeah. i know. but i feel like the issue is more that Bulat is more concerned with keeping a narrower focus with this Bundle. I get this impression not only from this PR, but also from others. So I feel like I will be forced to jump through hoops to get half of the changes in that I need. So I think I rather formalize the fork to clarify the situation and spend the time I have on new features and unit tests. On 04.10.2011, at 23:44, Christophe Coevoet reply@reply.github.com wrote:
|
Yes, I'm focused on not having feature overload. I do think that flexible image loading and caching are good additions like I said previously, but for everything else, you're better off creating a separate bundle. I would advice on rebranding your fork into something like FOSImagineBundle, keeping author headers would be appreciated in that case. |
Yes, thats what I was intending to do. I will publish it as LiipImagineBundle, though github will continue to list it as a fork from your Bundle and of course I will leave all author headers intact. |
@lsmith77 keeping it marked as a fork on github is not the best way to give it exposure |
I agree, I think you're better off with an entirely new repo |
Ok, I have created https://github.com/liip/LiipImagineBundle |
I did a lot of changes to make the Bundle flexible enough for my use cases:
3) make it possible to omit either the width or the heigth in the ThumbnailFilter
4) by default ThumbnailFilter will no longer upscale
5) added Configuration class for better config validation
Making the caching optional, means that the entire cache resolver is also optional.