symfony / webpack-encore-bundle

Symfony integration with Webpack Encore!
https://symfony.com/webpack-encore
MIT License
933 stars 83 forks source link

Unknown "encore_entry_link_tags" function #203

Closed n-valverde closed 1 year ago

n-valverde commented 1 year ago

Hello,

I am having a weird issue about encore_entry_link_tags function being unavailable under some special circumstances. First of all, I confirm this function exists and works in every context in my app, except one.

I managed to reproduce on a fresh Symfony app, here is the repo: https://github.com/n-valverde/encore-priority-bug

Here's what's happening:

I tried playing with the compiler passes of these bundles, but without luck.

I then figured out that raising the service tag priority of webpack_encore.twig_entry_files_extension makes the issue disappear, while lowering it reproduces the issue. But the thing is, no priority is defined on our end, everything is at default, so zero, and so is the webpack_encore.twig_entry_files_extension priority

When I first tried to reproduce with a fresh Symfony, it wasn't reproducing, then hacking a priority of -1 into vendor/symfony/webpack-encore-bundle/src/Resources/config/services.xml do reproduce, and when going back to zero, it still reproduce :thinking: , then only disappear with a priority of +1

Reminder to reproduce: throw yourself an exception from LocalFilesystemAdapter::__construct()

I know that I can make our twig extension lazy, and also tweak the Flysytem adapter config to make it lazy, but still I feel like this issue should not happen, and I'm not sure exactly why it's happening. I am thinking it's closely related to how twig must instantiate all non-lazy extensions, because just having the FilesystemOperator injected into a twig extension makes the Flysystem directory creation actually happen during bin/console cache:clear, but it's unclear how the priority is affecting the issue

Thanks for your help!

weaverryan commented 1 year ago

Hi there!

Thanks for the detailed report! I haven't checked out the reproducer yet, but I have a guess at what's going on.

In the compiled container, if you dig far enough, you'll find the generate method that instantiates the Twig Environment object. Near the bottom, you'll see all the extensions being added. It looks something like this (from the compiled container for ux.symfony.com)

$instance->addExtension(new \App\Service\CustomTwigExtension());
$instance->addExtension(new \Twig\Extra\Markdown\MarkdownExtension());
$instance->addExtension(new \Symfony\WebpackEncoreBundle\Twig\EntryFilesTwigExtension(new \Symfony\Component\DependencyInjection\Argument\ServiceLocator($this->getService, [
// ...

My guess that YOUR custom extension is throwing an exception when that class is being instantiated. That causes the entire instantiation of Twig itself to be "aborted" right on that line. Any extensions (or other Twig setup) that comes after will not be complete.

This could actually happen with any service. I think the key is to avoid exceptions from being thrown from the constructor of any service. In your case, if I understand it correctly, your Twig extension service requires the Flysystem service... and THAT explodes when it is instantiated. In that case, I would probably inject a service locator into your Twig extension instead or use a Twig runtime.

Cheers!

n-valverde commented 1 year ago

Hey Ryan, thanks for the nice answer.

The reproducer is very dummy anyway, just a symfony skeleton with a dummy twig extension injecting FilesystemOperator. (Could be any service that throws an exception from constructor indeed)

My guess that YOUR custom extension is throwing an exception when that class is being instantiated. if I understand it correctly, your Twig extension service requires the Flysystem service... and THAT explodes when it is instantiated

Your guess is correct, and you did understand correctly that the exception comes from the Flysystem service.

I would probably inject a service locator into your Twig extension instead or use a Twig runtime.

Indeed that's the way I chose to go, but I was wondering if there was something that could be fixed "deeper", like maybe just defining a higher priority for webpack-encore twig extension out of the box (not sure of what would be the drawbacks?). Because at the end, one can very easily hit the issue, just injecting a vendor service into a twig extension, with the vendor service throwing an exception from the constructor. Unfortunately, when this vendor exception is environment specific and/or random, this issue first appears in production logs and is really not easy to debug

Going with a twig runtime, or configuring Flysystem for laziness sure fixes the issue, but it's not obvious that not doing so will lead to such issue with webpack-encore (maybe just a note in the docs could help?)

Wdyt? Thanks again!

weaverryan commented 1 year ago

Hi!

Thanks for confirming. I don't think we should do anything on our end, however.

If we set the priority higher, we would probably fix this. But if you have a Twig extension that is throwing an exception in its constructor (or during construction), there would still be any other number of Twig extensions (with the default priority of 0) that would fail to load. So you might have the Encore functions, but you'd be missing other functions. You simply can't let your extensions explode during construction.

Cheers!

n-valverde commented 1 year ago

Got it, my point was more about mitigating the issue from encore side, not preventing twig to explode of course, just because the encore function can legitimately be used in custom twig error pages, so I think it could try its best to make itself available as soon as possible in the twig boot process, if that doesn't hurt anything.

Also I totally agree that our extensions should not explode during construction, I'm just concerned that someone hit this issue again and spend days to debug (took me 3 days starting from the production log message)

Thanks for the explanation anyway! Cheers!