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

Add GotenbergAssetRuntime to avoid passing Builder in context #128

Merged
merged 3 commits into from
Jan 8, 2025

Conversation

smnandre
Copy link
Contributor

Passing the builder in the Twig context is a bit fragile.

It won't work when template uses any of Twig layout features (includes, embed, macros, ...) without passing the full context.

Also, starting with Twig 4.0, the include function won't pass the full context per default, with no "boolean" to do so automatically, encouraging template isolation and composition.

This PR introduce a Twig Runtime, lowering the impact of the Extension on the global scope, and allowing to create a time-limited "scope", during which the gotenberg_asset will work as expected.

Passing builder in context is fragile, as it won't work if user
uses any of Twig layout features without passing the full context.

Starting with Twig 4.0, the `include` function won't pass the
full context, to encourage template isolation and composition.

This PR introduce a Twig Runtime, lowering the impact of this extension
on the global scope, and allowing to create a time-limited "scope",
during which the `gotenberg_asset` will work as expected.
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.

Love it thanks @smnandre . Although it means that gotenberg_asset is clearly not existing within twig "normal" runtime. Which eventually we wanted to be able to enable that function to fallback to native "asset" if not in builder context. allowing the same twig template to be used for both web AND pdf. WDYT ?

@Jean-Beru
Copy link
Contributor

Love it thanks @smnandre . Although it means that gotenberg_asset is clearly not existing within twig "normal" runtime. Which eventually we wanted to be able to enable that function to fallback to native "asset" if not in builder context. allowing the same twig template to be used for both web AND pdf. WDYT ?

When gotenberg_asset is used for private assets (that's its main purpose), asset will not be able to be used as a fallback.

A dedicated controller may render these files but it will make them public. An other solution could be to render them as a base64 string.

@smnandre
Copy link
Contributor Author

smnandre commented Jan 6, 2025

I'm not immersed enough in this project to have many use cases in mind... but instinctively i would have said the need to have asset behave differently (only during Gotenberg render) is not that common.

Maybe best to have a distinct methods and user can extends their templates, blocks if they do need to render all but one image the same way.

🤷

@Neirda24 Neirda24 merged commit 044847c into sensiolabs:main Jan 8, 2025
8 checks passed
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