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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FrameworkBundle] Fix bad interface hint in AbstractController #24431

Merged
merged 1 commit into from Oct 5, 2017

Conversation

Projects
None yet
6 participants
@nicolas-grekas
Member

nicolas-grekas commented Oct 5, 2017

Q A
Branch? 3.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

As spotted by @stof

@chalasr

chalasr approved these changes Oct 5, 2017

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Oct 5, 2017

Member

Does it really make a difference? We have aliases for both interfaces pointing to the same service, don't we?

Member

xabbuh commented Oct 5, 2017

Does it really make a difference? We have aliases for both interfaces pointing to the same service, don't we?

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Oct 5, 2017

Member

I think it's sane to have the type that is closer to the injected implementation in the class itself.

Member

chalasr commented Oct 5, 2017

I think it's sane to have the type that is closer to the injected implementation in the class itself.

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Oct 5, 2017

Member

Instead of changing the interface, I would instead change the implementation to respect the current interface.

Member

fabpot commented Oct 5, 2017

Instead of changing the interface, I would instead change the implementation to respect the current interface.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas
Member

nicolas-grekas commented Oct 5, 2017

done

@fabpot

fabpot approved these changes Oct 5, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Oct 5, 2017

Member

But tests are broken.

Member

fabpot commented Oct 5, 2017

But tests are broken.

@xabbuh

xabbuh approved these changes Oct 5, 2017

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Oct 5, 2017

Member

Instead of changing the interface, I would instead change the implementation to respect the current interface.

Esp. since the FrameworkBundle EngineInterface will be deprecated #21035

Member

Tobion commented Oct 5, 2017

Instead of changing the interface, I would instead change the implementation to respect the current interface.

Esp. since the FrameworkBundle EngineInterface will be deprecated #21035

@Tobion

Tobion approved these changes Oct 5, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Oct 5, 2017

Member

Thank you @nicolas-grekas.

Member

fabpot commented Oct 5, 2017

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 5d29dd0 into symfony:3.3 Oct 5, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Oct 5, 2017

bug #24431 [FrameworkBundle] Fix bad interface hint in AbstractContro…
…ller (nicolas-grekas)

This PR was merged into the 3.3 branch.

Discussion
----------

[FrameworkBundle] Fix bad interface hint in AbstractController

| Q             | A
| ------------- | ---
| Branch?       | 3.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

As spotted by @stof

Commits
-------

5d29dd0 [FrameworkBundle] Fix bad interface hint in AbstractController

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:engine-if branch Oct 5, 2017

@fabpot fabpot referenced this pull request Oct 5, 2017

Merged

Release v3.3.10 #24452

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment