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 file transformers to the file loader #57

Merged
merged 4 commits into from
Mar 19, 2012
Merged

Add file transformers to the file loader #57

merged 4 commits into from
Mar 19, 2012

Conversation

lucasaba
Copy link
Contributor

In this way, it is possible to pass an array of transformer
to the data loader.
Transformers can be used to create images for
virtually any file type.

This PR is a cleanup of previous posts and references #52.

In this way, it is possible to pass an array of transformer
to the data loader.
Transformers can be used to create images for
virtually any file type.
*/
public function __construct(ImagineInterface $imagine, $formats, $rootPath)
public function __construct(ImagineInterface $imagine, $formats, $rootPath, $transofrmers)
Copy link
Contributor

Choose a reason for hiding this comment

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

typo in the variable name

Copy link
Contributor

Choose a reason for hiding this comment

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

btw, you should typehint the array

@lucasaba
Copy link
Contributor Author

poke

@lsmith77
Copy link
Contributor

ok this looks fine to me now.
one thing that we might want to add eventually is to add some by reference parameter to applyTransform() to stop the transform foreach. but i think that can wait.

$info = pathinfo($absolutePath);
if (isset($info['extension']) && strpos(strtolower($info['extension']), 'pdf') !== false) {
//Check if Imagick extension is loaded
if (!extension_loaded('Imagick'))
Copy link
Contributor

Choose a reason for hiding this comment

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

please do not omit curly brackets

Copy link
Contributor

Choose a reason for hiding this comment

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

opps .. i also noticed another thing. i would prefer it if we would inject the Imagick instance .. i don't have experience with its API .. but i assume we could also call setFilename() instead of passing the absolute path to the constructor? could you do a PR to change this?

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm actually it seems Imagine itself is using the same approach .. the Imagick OO API also seems a bit strange:
https://github.com/avalanche123/Imagine/blob/master/lib/Imagine/Imagick/Imagine.php

@avalanche123 do you have a tip for us here what would make the most sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. It would be better. I'm checkin the API and PR the changes!

lsmith77 added a commit that referenced this pull request Mar 19, 2012
Add file transformers to the file loader
@lsmith77 lsmith77 merged commit 8dd784e into liip:master Mar 19, 2012
@lucasaba
Copy link
Contributor Author

Thank you for all the corrections, informations, lessons and for your patience ;)

@lsmith77
Copy link
Contributor

no problem .. thx for working through all of this. looking forward for other patches from you :)

@rvanlaak
Copy link

Great addition to the bundle! My usecase is that I want to create thumbnails of all pages, so any ideas on a PR for that? In theory it sounds simple, since the page number is hard-coded in PdfTransformer.php

@havvg
Copy link
Contributor

havvg commented Jun 18, 2013

The thing is you are creating more than one image in your case.
For itself, there is no problem with it, but well, you can't simply apply it to any PDF, which page do you want to return? You can add options for all of this, but then you would have to ensure the PDF has page X, where X is the page the thumbnail would be returned for, and you need a naming scheme to create the respective thumbnails for.

@rvanlaak
Copy link

In my usecase I already know the exact amount of pages in the PDF, so what I would like to do is create a loop in Twig and do a call to the imagine_filter every time. The PdfTransformer needs to be modified in order to accept a page number, and indeed, else return an IndexOutOfBoundException in case the page does not exists.

The TransformerInterface::apply() function only accepts one parameter, the $absolutePath. Does the interface needs to be modified in order to accept page numbers?

@havvg
Copy link
Contributor

havvg commented Jun 18, 2013

It would indeed require even more. You would need to pass in options into image_filter, to be able to pass them around - and to the transformer. Which in return would require you to have options in the routing to specifically request the correct page, which would require to modify the CacheManager, too.

Unless I'm getting it all wrong right now, I'm against such heavy changes in the 0.x version line. However, if you figure out a sensible solution, send the PR :-)

@rvanlaak
Copy link

Idea to reopen this issue , or should I create another issue for discussing the features?

@havvg
Copy link
Contributor

havvg commented Jun 19, 2013

A new one, please.

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.

5 participants