Skip to content

Conversation

eusonlito
Copy link

Hi! I have added some new features and updated the main code to make it more readable.

c8c80f0 To allow add custom parameters to proc_open function.
5053bf8 Added setXvfb function to allow enable xvfb-run when you are executing phpwkhtmltopdf into a non GUI enviroment

Regards!
Lito.

@kwisatz
Copy link

kwisatz commented Dec 4, 2013

The xvfb support is much appreciated, it removes the need to create a wrapper shell script that makes use of xvfb-run.

Regarding your changes for readability, you might want to check out the PSR-2 coding style guide if you haven't already.

@eusonlito
Copy link
Author

Yes I know, I think that my unique not compliant PSR-2 code is the space between function name a parentheses. I can change it, no problem :)

Do you see any other PSR-2 problem?

@mikehaertl
Copy link
Owner

@eusonlito Thanks for your contribution. I'd actually prefer smaller chunks in a PR, so that it's easier to review (i.e. put the binary autoloading and xvfb stuff into separate PRs please).

And when it comes to "cleanup" and "improved readability": That means different things to different people. Since my style got closer to PSR-2 since I wrote this class: I can live with extra curly braces - but please remove all these extra spaces.

@mikehaertl
Copy link
Owner

@eusonlito Some more comments before you put too much work into reorganizing this PR:

  • I'm happy to integrate the xvfb support. I'm just not sure yet if your implementation is perfect. You basically fire up an X server for each and every created PDF. There's an alternative approach as described over here. I think a good implementation should support both ways. What do you think?
  • I like your idea to autodected the binary. I'd just implement it a bit different. So if you don't mind I'd add that myself.
  • I'm also fine with the procEnv change.

So maybe start with a fresh PR that only contains the xvfb and procEnv changes, and we continue discussion there?

@eusonlito
Copy link
Author

Yes, I know that "cleanup" and "improved readability" is very subjective :)

I have updated functions rows to be PSR-2

  • My implementantion is not perfect, I know, but I want a way to use xvfb without external custom code.
  • Ok
  • Ok ;)

I'm not a git expert, I will try to do it about new PR :)

@mikehaertl
Copy link
Owner

Cool. Let me know if you have problems with the new PR. git can be confusing in the beginning and I don't give you a too hard time :).

The new PR is mainly to discuss the xvfb feature independent from the other changes.

@eusonlito eusonlito closed this Dec 4, 2013
@eusonlito
Copy link
Author

I can't send you now the clean PR because I'm reciving a github error:

ERROR: Storage server temporarily offline. See http://status.github.com for GitHub system status. fatal: Could not read from remote repository. 

I will do it when github fix the error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants