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

add support to Imagine Basic filters #59

Closed
wants to merge 3 commits into from
Closed

add support to Imagine Basic filters #59

wants to merge 3 commits into from

Conversation

inmarelibero
Copy link

This PR aims to give support to almost all the Imagine basic filters, not only thumbnail.

The list of the new filters supported is:

crop
resize
rotate
filpHorizontally
flipVertically
thumbnail

As described in the documentation, these are the basic filters that could be interesting for image manipulation in twig. Filters like copy or paste are not implemented.

A user would be now able to use all these options in config.yml:

avalanche_imagine:
    filters:
        crop:
            type:    crop
            options: { start: [0, 0], size: [120, 120] }
        resize:
            type:    resize
            options: { size: [120, 120] }
        rotate:
            type:    rotate
            options: { angle: 145, background: '#CCC' }
        flipHorizontally:
            type:    flipHorizontally
        flipVertically:
            type:    flipVertically
        outset:
            type:    thumbnail
            options: { size: [120, 120], mode: outset }
        inset:
            type:    thumbnail
            options: { size: [120, 120], mode: inset }

if (!isset($options['options'])) {
throw new InvalidArgumentException(sprintf(
'Options for filter type "%s" must be specified', $filter
));
}
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should remove old code instead of commenting it. Versionning is handled by git, not by comments

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right, I forgot them. fixed

@avalanche123
Copy link
Owner

This is a great idea, thanks for the work. The way you changed filter manager won't allow for extension with custom filter loaders as it will always use the basic filter loader. The idea was to make it easy for users to write loaders for their own filters. I would prefer you left filter manager as is but split the code into individual filter loaders for each filter or registered basic filter loader in DIC for each filter it can load using appropriate tag.

@inmarelibero
Copy link
Author

Yes I considered making one individual filter loaders, but then I saw the amount of code was not big, and also it's intuitive for me that all basic filters would stay in some BasicFilter.php class.
I made a comment on a file, it could resolve custom filter loaders

@avalanche123
Copy link
Owner

I think we should treat all filters the same and configure them using dependency injection container and tags. By hard-coding a special case for basic filters we're not doing mixing configuration with code and not taking advantage of DIC. You should be able to just tag basic filter loader with:

<tag name="imagine.filter.loader" filter="thumbnail" />
<tag name="imagine.filter.loader" filter="crop" />
<tag name="imagine.filter.loader" filter="resize" />
<tag name="imagine.filter.loader" filter="rotate" />
<tag name="imagine.filter.loader" filter="flipHorizontally" />
<tag name="imagine.filter.loader" filter="flipVertically" />

and leave FilterManager untouched

@stof, is this correct?

@stof
Copy link
Contributor

stof commented May 6, 2012

@avalanche123 yes, except that your compiler pass only reads the first tag and ignore others currently. But it can be fixed easily

@avalanche123
Copy link
Owner

@inmarelibero I merged @stof's pull request, so now you should be able to register one filter loader to load multiple filters like I showed in my comment above. Please update your pull request so I can merge it

@avalanche123
Copy link
Owner

this has been stale for a while, are you still interested in landing this in bundle?

@avalanche123
Copy link
Owner

closing for the lack of activity

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.

None yet

3 participants