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

Major Refactoring #25

Closed
wants to merge 35 commits into from

Conversation

Projects
None yet
4 participants
@lsmith77
Copy link

lsmith77 commented Jul 21, 2011

I did a lot of changes to make the Bundle flexible enough for my use cases:

  1. some minor cleanups
  2. refactored the controller/filter
  • added support for converting to different formats
  • added ability to leave off the format
  • added ability to disable caching
  • added ability to set a custom loader (fixes #21)
  • added ability to configure quality
    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.

@lsmith77

This comment has been minimized.

Copy link
Author

lsmith77 commented Jul 21, 2011

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.

lsmith77 added some commits Jul 22, 2011

add support for defining a filter without a type to return the origin…
…al image (useful to be able to read the image from a different place than the public web root)
make the Request instance a parameter to the resolve() method so that…
… the CachePathResolver does not need to be requested scoped (allows reverting the changes to the templating classes)
@avalanche123

This comment has been minimized.

Copy link
Owner

avalanche123 commented Jul 22, 2011

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

@lsmith77

This comment has been minimized.

Copy link
Author

lsmith77 commented Jul 23, 2011

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 ..

@avalanche123

This comment has been minimized.

Copy link
Owner

avalanche123 commented Jul 23, 2011

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,

@lsmith77

This comment has been minimized.

Copy link
Author

lsmith77 commented Jul 24, 2011

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"?

@avalanche123

This comment has been minimized.

Copy link
Owner

avalanche123 commented Jul 24, 2011

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

@lsmith77

This comment has been minimized.

Copy link
Author

lsmith77 commented Jul 24, 2011

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.

@lsmith77

This comment has been minimized.

Copy link
Author

lsmith77 commented Jul 31, 2011

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.

@avalanche123

This comment has been minimized.

Copy link
Owner

avalanche123 commented Jul 31, 2011

why did u make caching optional?

@lsmith77

This comment has been minimized.

Copy link
Author

lsmith77 commented Aug 12, 2011

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 ..

@Abhoryo

This comment has been minimized.

Copy link

Abhoryo commented Sep 21, 2011

Thx for this upgrade @lsmith77.

@lsmith77

This comment has been minimized.

Copy link
Author

lsmith77 commented Sep 27, 2011

just checked .. the DI extension tests are still passing ..

@ghost

This comment has been minimized.

Copy link

ghost commented Oct 4, 2011

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 service_container instead of imagine.cache.path.resolver only in 9ad85ed ?

@avalanche123 will this be merged any time soon?

@lsmith77

This comment has been minimized.

Copy link
Author

lsmith77 commented Oct 4, 2011

@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.

@avalanche123

This comment has been minimized.

Copy link
Owner

avalanche123 commented Oct 4, 2011

Here is what is necessary to make this mergeable:

  • minimize the diffs to make them understandable - consider Controller/ImagineController.php file in the diff of the current pull request. Just by looking at the header of the class where the attributes are defined, I see that not much has changed, we still have cachePathResolver for example, but because the pull request includes code rearrangements, the diff tells us that it had been removed and readded.
  • get rid of code rearrangements and not important changes - I have a filter method in the controller, which was renamed to filterAction. Things like that make it hard to understand the real modifications and benefits of the pull by cluttering the diff view.
  • respect and mimic the initial code formatting and conventions - same thing, I don't like using optional parameters with if conditions as a result of that or Action suffix on controller actions

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.

@avalanche123

This comment has been minimized.

Copy link
Owner

avalanche123 commented Oct 4, 2011

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

@lsmith77

This comment has been minimized.

Copy link
Author

lsmith77 commented Oct 4, 2011

I understand your concerns, but don't think its feasible for me to comply.
There really isn't much code in the current version of the Bundle and its all quite monolithic. Undoing the filterAction change would of course be trivial, but not really get us closer. Trying to split this up into smaller chunks is essentially me trying to guess in what order you would prefer the changes with a high likelihood that in the middle you decide to do everything differently meaning that in the end I will likely have to do the work 2-3 times over.

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 lsmith77 closed this Oct 4, 2011

@stof

This comment has been minimized.

Copy link
Contributor

stof commented Oct 4, 2011

@lsmith77 One point is that some changes are simply changing the order in which the properties of the classes are declared

@lsmith77

This comment has been minimized.

Copy link
Author

lsmith77 commented Oct 4, 2011

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:

@lsmith77 One point is that some changes are simply changing the order in which the properties of the classes are declared

Reply to this email directly or view it on GitHub:
#25 (comment)

@avalanche123

This comment has been minimized.

Copy link
Owner

avalanche123 commented Oct 4, 2011

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.

@lsmith77

This comment has been minimized.

Copy link
Author

lsmith77 commented Oct 4, 2011

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.

@stof

This comment has been minimized.

Copy link
Contributor

stof commented Oct 4, 2011

@lsmith77 keeping it marked as a fork on github is not the best way to give it exposure

@avalanche123

This comment has been minimized.

Copy link
Owner

avalanche123 commented Oct 4, 2011

I agree, I think you're better off with an entirely new repo

@lsmith77

This comment has been minimized.

Copy link
Author

lsmith77 commented Oct 4, 2011

@lmcd lmcd referenced this pull request Nov 7, 2012

Closed

Added PasteFilterLoader #95

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.