search

csarrazi / CsaGuzzleBundle

A bundle integrating Guzzle >=4.0 in Symfony
250 stars 76 forks source link

Only load GuzzleExtension when profiler is available #261

Open rvanlaak opened 4 years ago

rvanlaak commented 4 years ago

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.

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

csarrazi commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

rvanlaak commented 4 years ago

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