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

Enhancement: use internal exception #462

Open
garak opened this issue Jan 6, 2023 · 3 comments
Open

Enhancement: use internal exception #462

garak opened this issue Jan 6, 2023 · 3 comments
Assignees

Comments

@garak
Copy link
Contributor

garak commented Jan 6, 2023

This library is currently throwing a generic RuntimeException when the pdf conversion fails, see the source code at https://github.com/KnpLabs/snappy/blob/master/src/Knp/Snappy/AbstractGenerator.php#L431

This makes it hard for the users to properly react to such exceptions since they are forced to rely on the exception text message (which it's not even guaranteed to stay the same).
So, a possible nice enhancement could be adding an internal exception (extending RuntimeException, so it would be back-compatible)
By the way, this library already uses this approach with Knp\Snappy\Exception\FileAlreadyExistsException
I can provide a PR.

@AntoineLelaisant
Copy link

Hello! Thanks for your feedback! I agree with your point, having generic exception is not handy at all to let users handle things properly. But I do not think this is something we will try implement in the current version of snappy as we are currently working on a v2 (see #467). For sure we will take your feedback into consideration for it.

Nevertheless if you have time to propose a PR to enhance the current version I would be glad to review and merge it.

@stale
Copy link

stale bot commented Jun 18, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 18, 2023
@MarkTatterTierney
Copy link

MarkTatterTierney commented Jan 5, 2024

Also sometimes a pdf is generated when a RuntimeException is thrown during the checkProcessStatus method. There is no way to ignore the error and carry on with the process.

Putting the checkOutput step before the checkProcessStatus and then optionally ignoring the checkProcessStatus if there is no output error would be beneficial.

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

No branches or pull requests

7 participants