[TwigBundle] Add missing dev dependency to console #21246
Conversation
e72544e to
33f30b7
Compare
33f30b7 to
66d5b57
Compare
| @@ -24,6 +24,7 @@ | ||
| "twig/twig": "~1.28|~2.0" | ||
| }, | ||
| "require-dev": { | ||
| "symfony/console": "~3.3", |
There was a problem hiding this comment.
TwigBundle 3.1 requiring Console 3.3 ? this looks wrong to me
There was a problem hiding this comment.
Only to run tests with the last version of FrameworkBundle (https://travis-ci.org/symfony/symfony/jobs/191309256). Another option is to make FrameworkBundle requiring console. (There is probably a better solution).
There was a problem hiding this comment.
well, either FraneworkBundle should require it, or it should not be broken when the component is not installed. Otherwise it is a BC break in FrameworkBundle, and TwigBundle is the wrong place to fix it (all projects depending on FrameworkBundle would have to do the same, which is wrong)
There was a problem hiding this comment.
It was already the case for the templating component in 3.3. AFAIK we allow this kind of change (requiring to install a new dependency) and it's valid according to semver.
I'm not against adding Console as a hard dependency of FrameworkBundle, but I guess a goal of 3.3/Flex is to have less components installed by default when requiring framework-bundle. This change will go in the opposite direction.
There was a problem hiding this comment.
the issue is that FrameworkBundle always registers the compiler pass coming from the component. So it means that the component is required. There is no point making the dependency optional in composer when it is only optional if you never use the bundle
There was a problem hiding this comment.
We can also duplicate the code of this helper in the component and in the bundle, but I'm not sure it's worth it.
There was a problem hiding this comment.
The discussion in the PR talked about skipping the compiler pass when the component compiler pass class does not exist.
Registering the compiler pass for console commands is useless if you don't have the console component anyway. The only case needing to take care is people using console without the pass in it, but this is solved easily by making FrameworkBundle conflict with symfony/console < 3.3.
As long as you don't skip the registration, it is a mandatory requirement
There was a problem hiding this comment.
Ok, your proposal is way better. See #21248. Closing this one.
…ent isn't installed (dunglas) This PR was merged into the 3.3-dev branch. Discussion ---------- [FrameworkBundle] Prevent an error when the console component isn't installed | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #21246 | License | MIT | Doc PR |n/a Finish #19443. Alternative to #21246. Commits ------- ab133ca [FrameworkBundle] Prevent an error when the console component isn't installed
Make Travis green again (related to #19440).