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

Allow to asynchronously generate PDFs #65

Merged
merged 8 commits into from
May 7, 2024

Conversation

yhabteab
Copy link
Member

The current implementation of HeadlessChrome::toPdf() always assumes that it controls the event loop instance, i.e. HeadlessChrome creates and starts the event loop manually. This may work for most use cases as they are mostly triggered via Icinga Web, but not if you want to generate PDFs using a daemon. Since our scheduler uses the same global event instance, it is unfavourable to call Factory::create() over again occasionally.

refs Icinga/icingaweb2-module-reporting#229

@cla-bot cla-bot bot added the cla/signed label Mar 13, 2024
@yhabteab yhabteab requested a review from nilmerg March 13, 2024 10:16
@yhabteab yhabteab force-pushed the do-not-create-own-event-loop branch 7 times, most recently from e676eae to a1e9bb0 Compare March 13, 2024 15:56
@yhabteab yhabteab requested review from nilmerg and removed request for nilmerg March 13, 2024 15:58
Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

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

I'm not really convinced that the usage of Promise\Deferred is correct. I don't know what's correct nor have I ever used one myself (as mentioned offline) but it looks wrong. Will look into this tomorrow to see if I'm right.

library/Pdfexport/HeadlessChrome.php Outdated Show resolved Hide resolved
library/Pdfexport/HeadlessChrome.php Outdated Show resolved Hide resolved
library/Pdfexport/HeadlessChrome.php Outdated Show resolved Hide resolved
library/Pdfexport/HeadlessChrome.php Outdated Show resolved Hide resolved
vendor/iio/libmergepdf/tcpdi/fpdf_tpl.php Outdated Show resolved Hide resolved
@yhabteab yhabteab force-pushed the do-not-create-own-event-loop branch 2 times, most recently from 5f08ecb to f2c8ca5 Compare March 15, 2024 13:41
library/Pdfexport/HeadlessChrome.php Outdated Show resolved Hide resolved
library/Pdfexport/HeadlessChrome.php Outdated Show resolved Hide resolved
library/Pdfexport/HeadlessChrome.php Outdated Show resolved Hide resolved
library/Pdfexport/HeadlessChrome.php Outdated Show resolved Hide resolved
library/Pdfexport/HeadlessChrome.php Show resolved Hide resolved
library/Pdfexport/HeadlessChrome.php Outdated Show resolved Hide resolved
library/Pdfexport/HeadlessChrome.php Outdated Show resolved Hide resolved
library/Pdfexport/HeadlessChrome.php Outdated Show resolved Hide resolved
library/Pdfexport/HeadlessChrome.php Show resolved Hide resolved
@yhabteab yhabteab force-pushed the do-not-create-own-event-loop branch 3 times, most recently from 5e63f0c to 3be4f74 Compare March 18, 2024 13:23
@yhabteab yhabteab requested a review from nilmerg March 18, 2024 13:25
library/Pdfexport/HeadlessChrome.php Outdated Show resolved Hide resolved
@yhabteab yhabteab requested a review from nilmerg March 20, 2024 13:12
nilmerg
nilmerg previously approved these changes Mar 22, 2024
Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

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

Didn't test the asynchronous variant. I guess you did that thoroughly yourself. 😅

@nilmerg nilmerg added this to the 0.11.0 milestone Apr 9, 2024
Copy link
Contributor

@sukhwinder33445 sukhwinder33445 left a comment

Choose a reason for hiding this comment

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

Please rebase and remove this commit, it has already merged as a Composer patch.

@raviks789 raviks789 merged commit 43c4232 into main May 7, 2024
15 checks passed
@raviks789 raviks789 deleted the do-not-create-own-event-loop branch May 7, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants