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 KnpSnappyWriter #86

Closed
wants to merge 1 commit into from

Conversation

greg0ire
Copy link
Contributor

Changelog

### Added
- Added pdf writer based on [knp/snappy](https://packagist.org/packages/knplabs/knp-snappy)

Subject

This PR is my take on #76 , thanks @chalasr for what you already did

To do

- [ ] consider relying on knp/snappy-bundle to inject the dependency this is not a symfony bundle so we can't

@greg0ire greg0ire force-pushed the chalasr-chalasr-master branch from 7319fe2 to 93df253 Compare May 19, 2016 21:13
@soullivaneuh
Copy link
Member

knplabs/knp-snappy should be also added on conflict section.

$this->wkhtmltopdf = __DIR__.'/../../../../../vendor/h4cc/wkhtmltopdf-amd64/bin/wkhtmltopdf-amd64';

if (!is_executable($this->wkhtmltopdf)) {
$this->markTestSkipped('The wkhtmltopdf binary is not available');
Copy link
Member

Choose a reason for hiding this comment

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

Should not be a failed? AFAIK, the binary is downloaded from composer so it should be always available for tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm… probably a legacy from before doing this with Composer ? @chalasr , can you help ?

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, h4cc binaries don't work on osx at all (this is why it was previously set to /usr/local/bin), and is separated in two 32/64 packages. It was a kind of security for don't break them all.

@soullivaneuh
Copy link
Member

Not sure about Prophecy mixing getMock system. Maybe we should discuss about what we should apply for the future in order to keep consistency?

@greg0ire
Copy link
Contributor Author

Not sure about Prophecy mixing getMock system. Maybe we should discuss about what we should apply for the future in order to keep consistency?

There is not call to getMock in my test, and I think we should always go with Prophecy.

@soullivaneuh
Copy link
Member

There is not call to getMock in my test, and I think we should always go with Prophecy.

  • You think, not all people do. This is why I would like to discuss. :-)
  • I'm not talking about your test but about tests in general.

BTW, I would like to refactor some test later,it would be great to have a plan.

@greg0ire
Copy link
Contributor Author

Discussion is here : sonata-project/dev-kit#89

@OskarStark OskarStark mentioned this pull request May 20, 2016
4 tasks
@rande
Copy link
Member

rande commented May 20, 2016

I don't think this should be part of the project. Rendering PDF is more about reporting that exporting raw data (the idea of this lib). The PDF output will never match client's requirements.

This code should be included on a dedicated reporting bundle, with specific output handler: princexml, wkhtml, etc ...

@greg0ire
Copy link
Contributor Author

Hmm so off-topic… I did not see that because I don't know the library that well… closing then.

@greg0ire greg0ire closed this May 20, 2016
@rande
Copy link
Member

rande commented May 21, 2016

@greg0ire It is actually my bad, see my comment: #42

@greg0ire
Copy link
Contributor Author

Yeah just saw that indeed. We should create sonataReportingBundle ;)

@chalasr
Copy link
Contributor

chalasr commented May 22, 2016

At first a Reporter lib could be sufficient and, in a next time, integrate it in the admin-bundle through a Reporter class like done for this lib.

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.

7 participants