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

Feature: Add a builder to write pdf metadata #93

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ConstantBqt
Copy link
Contributor

Add WriteMetadataPdfBuilder, inspired by MergePdfBuilder

Partially resolve issue #43

Copy link
Contributor

@Neirda24 Neirda24 left a comment

Choose a reason for hiding this comment

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

I wonder though if it is worth the effort these routes ? Because we already provide a way to set metadata on generate. It requires to put the whole PDF into memory, send it though http to gotenberg and download it again as a stream response potentially. Might not be worth it. @Jean-Beru : WDYT ?

src/Builder/Pdf/WriteMetadataPdfBuilder.php Show resolved Hide resolved
@ConstantBqt
Copy link
Contributor Author

I wonder though if it is worth the effort these routes ? Because we already provide a way to set metadata on generate. It requires to put the whole PDF into memory, send it though http to gotenberg and download it again as a stream response potentially. Might not be worth it. @Jean-Beru : WDYT ?

IMO this is useful to update the metadata an existing PDF file.
I imagine this scenario:

Someone needs to modify thousands of its company's PDFs to change the metadata Copyright or Keywords.

@Neirda24
Copy link
Contributor

Neirda24 commented Jul 8, 2024

Someone needs to modify thousands of its company's PDFs to change the metadata Copyright or Keywords.

But then I would recommend to directly use a binary for this scenario. It requires too much memory + http bandwidth for such small and edge cases...

@ConstantBqt ConstantBqt force-pushed the feat/implement-write-metadata-endpoint branch from 552809f to 1f8ed84 Compare July 9, 2024 07:36
@Jean-Beru
Copy link
Contributor

I wonder though if it is worth the effort these routes ? Because we already provide a way to set metadata on generate. It requires to put the whole PDF into memory, send it though http to gotenberg and download it again as a stream response potentially. Might not be worth it. @Jean-Beru : WDYT ?

Putting whole PDF in memory is not mandatory. HttpClient accepts callback and resources as body: https://symfony.com/doc/current/http_client.html#uploading-data.

But then I would recommend to directly use a binary for this scenario. It requires too much memory + http bandwidth for such small and edge cases...

Of course but some applications may want to delegate this task to the Gotenberg container.

IMHO (here @ConstantBqt :trollface: ), this endpoint can be implemented as a very simple builder. The hardest thing is to know how split our code between file generation (screenshot, pdf, etc.) and data retrieval (read metadata, async, etc.)

@Neirda24
Copy link
Contributor

Neirda24 commented Jul 9, 2024

Our implementation of the http client does not yet covers memory efficient input. even with #90 we will only cover the output part.

@Jean-Beru
Copy link
Contributor

Our implementation of the http client does not yet covers memory efficient input. even with #90 we will only cover the output part.

Sure! It deserve a new issue to improve content and asset files.

docs/pdf/write-metadata-builder.md Show resolved Hide resolved
docs/pdf/write-metadata-builder.md Outdated Show resolved Hide resolved
{
public function yourControllerMethod(GotenbergPdfInterface $gotenberg): Response
{
return $gotenberg->writeMetadata()
Copy link
Contributor

Choose a reason for hiding this comment

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

looking at the example here I wonder. Because there is also the read metadata. We don't "generate" anything. Maybe it should out of scope of builders... ?

@ConstantBqt ConstantBqt force-pushed the feat/implement-write-metadata-endpoint branch from 2ca9878 to 0444157 Compare July 11, 2024 13:22
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.

3 participants