-
Notifications
You must be signed in to change notification settings - Fork 184
fix: remove twig extension if TwigBundle is not installed #974
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
base: master
Are you sure you want to change the base?
Conversation
|
Hi @Nyholm! Thanks for opening the PR, it looks good to me! The pipelines are a bit broken right now because of security advisories. We will most likely fix this by Monday and then we can get this PR merged |
| } | ||
|
|
||
| if (!$isConfigEnabled) { | ||
| $container->removeDefinition(TwigSentryExtension::class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: TwigSentryExtension incorrectly removed when tracing disabled
TwigSentryExtension is being removed when Twig tracing is disabled, preventing users from using Twig functions like sentry_trace_meta() even when Twig is installed. TwigSentryExtension provides general Sentry Twig functions and should only be removed when Twig itself isn't installed (already handled at line 75), not when tracing is disabled. Only TwigTracingExtension should be removed when tracing is disabled.
| // Remove Twig extension service if Twig is not installed to avoid autoloading failures on Symfony 8 | ||
| if (!class_exists(\Twig\Extension\AbstractExtension::class)) { | ||
| $container->removeDefinition(\Sentry\SentryBundle\Twig\SentryExtension::class); | ||
| $container->removeDefinition(TwigSentryExtension::class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: TwigTracingExtension not removed when Twig unavailable
When AbstractExtension doesn't exist (Twig not installed), only TwigSentryExtension is removed. However, TwigTracingExtension also extends AbstractExtension and will cause autoloading failures if Twig is unavailable. Both extensions should be removed when checking for Twig availability to prevent compilation errors on Symfony 8.
|
@Nyholm This might have become obsolete because of a guard introduced in the Symfony 8 PR that I merged today: Do you mind checking against |
Description
When running this without twig I get build errors because we are missing Twigs AbstractExtension
Issues
Reminders
feat:,fix:,ref:,meta:)