Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[TwigBundle] Add missing dev dependency to console#21246

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
dunglas wants to merge1 commit intosymfony:3.1fromdunglas:fix_twigbundle_tests

Conversation

@dunglas
Copy link
Member

@dunglasdunglas commentedJan 12, 2017
edited
Loading

QA
Branch?master
Bug fix?no
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsn/a
LicenseMIT
Doc PRn/a

Make Travis green again (related to#19440).

@dunglasdunglas changed the base branch frommaster to2.7January 12, 2017 14:09
@dunglasdunglas changed the base branch from2.7 to3.0January 12, 2017 14:12
@dunglasdunglas changed the base branch from3.0 to3.1January 12, 2017 14:15
"twig/twig":"~1.28|~2.0"
},
"require-dev": {
"symfony/console":"~3.3",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

TwigBundle 3.1 requiring Console 3.3 ? this looks wrong to me

Copy link
MemberAuthor

@dunglasdunglasJan 12, 2017
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Only to run tests with the last version of FrameworkBundle (https://travis-ci.org/symfony/symfony/jobs/191309256). Another option is to make FrameworkBundle requiringconsole. (There is probably a better solution).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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)

nicolas-grekas reacted with thumbs up emoji
Copy link
MemberAuthor

@dunglasdunglasJan 12, 2017
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

the issue is that FrameworkBundlealways registers the compiler pass coming from the component. So it means that the componentis required. There is no point making the dependency optional in composer when it is only optional if you never use the bundle

Copy link
MemberAuthor

@dunglasdunglasJan 12, 2017
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 withsymfony/console < 3.3.

As long as you don't skip the registration, it is a mandatory requirement

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ok, your proposal is way better. See#21248. Closing this one.

fabpot added a commit that referenced this pull requestJan 12, 2017
…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/aFinish#19443. Alternative to#21246.Commits-------ab133ca [FrameworkBundle] Prevent an error when the console component isn't installed
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof left review comments

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dunglas@stof@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp