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

Make Twig dependency optional #12

Merged
merged 13 commits into from
Feb 22, 2024
Merged

Make Twig dependency optional #12

merged 13 commits into from
Feb 22, 2024

Conversation

awurth
Copy link
Contributor

@awurth awurth commented Dec 25, 2023

Fixes #1 - Make TwigBundle dependency optional + some refactoring

  • Add phpstan to CI
  • Fix all phpstan errors
  • Restrict each builder to the actions supported by its associated Gotenberg route
  • Add some validation for required options
  • Allow passing required options to the builder from GotenbergInterface
  • Use more specific exceptions
  • Fix potential duplication of multipart form data fields
  • Make it possible to override any previous option

.github/workflows/ci.yaml Show resolved Hide resolved
src/Builder/TwigTrait.php Outdated Show resolved Hide resolved
src/Builder/TwigTrait.php Outdated Show resolved Hide resolved
@Jean-Beru
Copy link
Contributor

I think you can work with @StevenRenaux to make a unique PR. See #11.

@awurth awurth requested a review from Jean-Beru January 25, 2024 16:20
@Jean-Beru Jean-Beru requested a review from Neirda24 January 26, 2024 08:38
*
* @throws PdfPartRenderingException if the template could not be rendered
*/
public function renderPart(PdfPart $pdfPart, string $template, array $context = []): self
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can find a better name, WDYT ? HTML has htmlContent, what about twigContent, twigHeader and twigFooter ? They can use a protected renderPart method.

This comment was marked as resolved.

Copy link
Contributor

Choose a reason for hiding this comment

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

the twig* prefix will be needed since this trait is used in the HtmlPdfBuilder class which has already a htmlContent method (or just content).

Copy link
Contributor

Choose a reason for hiding this comment

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

Why does HtmlBuilder need Twig ? The whole point of this PR was to not have this depdency. Or am I missing something ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't "need" Twig, the dependency is optional and the method checks if Twig is available.
Are you suggesting to keep the TwigBuilder? As we've discussed it, it wouldn't make sense IMHO since every chromium route could be called with rendered templates.

src/Builder/AbstractPdfBuilder.php Outdated Show resolved Hide resolved
/**
* The HTML file to convert into PDF.
*/
public function htmlContent(string $filePath): self
Copy link
Contributor

Choose a reason for hiding this comment

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

why the prefix html ? We are in a HtmlBuilder context. content should suffice IMO. Same goes for header / footer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed the twig method to content and the HTML file method to contentFile

src/Builder/MarkdownPdfBuilder.php Outdated Show resolved Hide resolved
*
* @throws PdfPartRenderingException if the template could not be rendered
*/
public function renderPart(PdfPart $pdfPart, string $template, array $context = []): self

This comment was marked as resolved.

src/Builder/UrlPdfBuilder.php Show resolved Hide resolved
src/Builder/AbstractChromiumPdfBuilder.php Show resolved Hide resolved
src/Client/GotenbergClientInterface.php Outdated Show resolved Hide resolved
$builder->setConfigurations($this->userConfigurations);

if (null !== $htmlTemplate) {
$builder->htmlTemplate($htmlTemplate);
Copy link
Contributor

Choose a reason for hiding this comment

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

what is htmlTemplate in context of markdown ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the HTML file that wraps the markdown content (https://gotenberg.dev/docs/routes#markdown-files-into-pdf-route). I renamed it to htmlWrapper for rendering with Twig and htmlWrapperFile for HTML files, WDYT?

Copy link
Contributor

@Jean-Beru Jean-Beru left a comment

Choose a reason for hiding this comment

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

Some little comments before approving 🙂

src/Builder/LibreOfficePdfBuilder.php Outdated Show resolved Hide resolved
src/Builder/MarkdownPdfBuilder.php Outdated Show resolved Hide resolved
@StevenRenaux StevenRenaux merged commit c448e19 into sensiolabs:main Feb 22, 2024
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.

Make TwigBundle dependency optional
4 participants