slope-it / breadcrumb-bundle

A bundle for generating dynamic breadcrumbs in Symfony applications
Other
10 stars 4 forks source link

Lazy load twig extensions #16

Closed Jupi007 closed 2 years ago

Jupi007 commented 2 years ago

Hi, I noticed that the twig extension are not lazy loaded. Would you like a PR to implement that?

asprega commented 2 years ago

Hi! Interesting feature, I did not know about it.

It's definitely an interesting concept, but:

As this is conceptually an extension that, if you're using breadcrumbs, would be used in almost all requests, I don't think we should add that complexity. No other bundles I use (nor extensions directly provided by Symfony) are doing this. I don't think making such a change would make any noticeable difference in real world scenario.

If you still think you need to do that, you can always re-define the service as lazy yourself by adding lazy: true to the definition. Alternatively, you could use a compiler pass to do the same.

Would that be okay with you?

Jupi007 commented 2 years ago

About the Twig dependency, this bundle already depends on the ^2.10 version: https://github.com/slope-it/breadcrumb-bundle/blob/aa931b085cbe66c9703942c26e77a4defd764a7d/composer.json#L28

But as you said the performance improvement is negligible . It is only because I am used to do it in my projects that I have proposed it to you :)

Jupi007 commented 2 years ago

I close this, because it could even deteriorate a bit the performance to implement that, as it require a bit more logic.

Anyway, I'm glad I taught you something interesting @asprega :)

asprega commented 2 years ago

Oh yeah, sorry I didn't follow up to your reply. You were right about the version number, for some reason my brain thought that 2.10 was less than 2.4.4 😂

Of course, there's always something to learn. Thanks again for your contributions!