contao / manager-plugin

Contao Manager Plugin
GNU Lesser General Public License v3.0
4 stars 9 forks source link

Require symfony/symfony #4

Closed ghost closed 6 years ago

ghost commented 7 years ago

Issue by @m-vo September 12th, 2017, 16:38 GMT

Shouldn't it be correct to require(-dev?) symfony/symfony in (all) contao bundles? (Not that anyone uses the core-bundle standalone right now...)


Background: I stumbled across this when I wrote a manager plugin for a reusable bundle. During development I use composer with the bundle's composer.json to get code checking and autocompletion for the required components. For the manager plugin the bundle classes are needed - so e.g. for TwigBundle there is the need to require symfony/symfony.

see Manager Plugin references SecurityBundle, TwigBundle, MonologBundle... and ContaoManagerBundle btw.

ghost commented 7 years ago

Comment by @leofeyer September 14th, 2017, 13:06 GMT

I don't think so. First, requiring symfony/symfony has been implicitly deprecated with Symfony flex. Second, you should only require the components that you are actually using, so maybe the requirements of the manager plugin are not sufficient?

Does this issue affect only the manager plugin?

ghost commented 7 years ago

Comment by @m-vo September 14th, 2017, 13:18 GMT

Likely that it only affects the manager plugin.

In the plugin in the core we're e.g. using the following:

use Symfony\Bundle\TwigBundle\TwigBundle;
// ...
TwigBundle::class

It's arguabable if a TwigBundle::class really is consuming the class as this string might work without the class beeing present. But code checking complains about it so that's why I thought it might be useful to have it available as a dev-requirement.

ghost commented 7 years ago

Comment by @leofeyer September 14th, 2017, 13:21 GMT

Well use Symfony\Bundle\TwigBundle\TwigBundle does indicate that the Twig bundle is a required dependency that should be listed in the composer.json file.

leofeyer commented 6 years ago

The issue should not have been moved. Please see https://github.com/contao/core-bundle/issues/1070.