Skip to content
This repository has been archived by the owner on Jul 15, 2021. It is now read-only.

Only load GuzzleExtension when profiler is available #261

Open
rvanlaak opened this issue Jan 29, 2020 · 4 comments
Open

Only load GuzzleExtension when profiler is available #261

rvanlaak opened this issue Jan 29, 2020 · 4 comments

Comments

@rvanlaak
Copy link
Contributor

rvanlaak commented Jan 29, 2020

The hard dependency in composer.json on Twig can get removed, and the Twig related service definitions only have to get loaded when profiler is available and enabled.

Why?

Right now Twig is a hard requirement, although Twig does not need to be a dependency in this bundle.

  • As a result Twig will get loaded in production for API applications that don't use Twig
  • Twig solely is needed when the profiler is available and enabled in userland.

The minimum usage gets visible when searching the codebase for Twig, it all is related to the profiler.

If userland did require symfony/web-profiler-bundle, that will take care of requiring twig/twig (see related composer.json).

Proposed solution

  • remove twig dependency from composer.json
  • let CsaGuzzleExtension load service definitions when profiler class exists (instead of removing them when disabled)
@csarrazi
Copy link
Owner

Hi @rvanlaak. I agree with your proposition. We could move out the Twig dependency, however it is important to note that the reason why it is here is that templates and extensions depend on specific versions of Twig. Hence the dependency is here in order to ensure that only compatible versions of Twig are pulled, or that only a bundle compatible with the version of Twig used by the project is pulled.

I would be delighted if we could either move the Twig dependency to suggests, but that won't help restrict the actual version of Twig (which is used to ensure backward compatibility).

However, I definitely agree with the extension loading service definitions when the profiler class exists.

If you wish to open a PR, I would be glad to review it and merge it.

@rvanlaak
Copy link
Contributor Author

The conflicts section is there to solve that: https://getcomposer.org/doc/04-schema.md#conflict

What minimum version of Twig would be needed?

@csarrazi
Copy link
Owner

As mentioned in the composer.json: "^2.10 || ^3.0" are the only versions supported :)

@rvanlaak
Copy link
Contributor Author

rvanlaak commented Feb 21, 2020

Can you then move them to the conflicts section, as we agree that it is not a hard dependency.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants