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][Controller] Use interface from Contracts to remove deprecation arnings #29398

Merged
merged 1 commit into from Dec 8, 2018

Conversation

@tomtomau
Contributor

tomtomau commented Dec 1, 2018

Q A
Branch? 4.2?
Bug fix? no
New feature? no
BC breaks? I don't think so
Deprecations? no - removing some :)
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

When we installed 4.2, we hit issues in our qa tools with vimeo/psalm reporting that all of our controllers were implementing a deprecated interface (by extending the AbstractController).

This pr simply updates the AbstractController to use the interface provided in symfony/contracts

I'm not sure if there was any other reason for not including this in the 4.2 release (bc?), so apologies if I've overstepped this.

@tomtomau tomtomau force-pushed the tomtomau:fix-controller-dep branch from 8f947cb to f10227d Dec 1, 2018

@tomtomau

This comment has been minimized.

Contributor

tomtomau commented Dec 1, 2018

Whoops, had forked off master.

Rebased onto 4.2 - not sure if this would be the right branch to target? My thought is that would be fine to land in 4.2.x?

@nicolas-grekas nicolas-grekas added this to the 4.2 milestone Dec 1, 2018

@nicolas-grekas

nicolas-grekas approved these changes Dec 1, 2018 edited

LGTM for 4.2
There is a theoretical BC break here when one does instanceof ServiceSubscriberInterface on a controller - but nobody does that.
We might also consider this as a false positive from psalm (it should ignore same-vendor usage of deprecated interfaces - knowing that in the generic case, fixing this is a BC break.)

@lyrixx

lyrixx approved these changes Dec 4, 2018

@Tobion

Tobion approved these changes Dec 6, 2018

@chalasr

chalasr approved these changes Dec 6, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented Dec 8, 2018

Thank you @tomtomau.

@fabpot fabpot merged commit f10227d into symfony:4.2 Dec 8, 2018

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 Dec 8, 2018

minor #29398 [FrameworkBundle][Controller] Use interface from Contrac…
…ts to remove deprecation arnings (tomtomau)

This PR was merged into the 4.2 branch.

Discussion
----------

[FrameworkBundle][Controller] Use interface from Contracts to remove deprecation arnings

| Q             | A
| ------------- | ---
| Branch?       | 4.2?
| Bug fix?      | no
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | I don't think so     <!-- see https://symfony.com/bc -->
| Deprecations? | no - removing some :) <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | n/a   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | n/a

When we installed 4.2, we hit issues in our qa tools with [vimeo/psalm](https://github.com/vimeo/psalm) reporting that all of our controllers were implementing a deprecated interface (by extending the AbstractController).

This pr simply updates the AbstractController to use the interface provided in `symfony/contracts`

I'm not sure if there was any other reason for not including this in the 4.2 release (bc?), so apologies if I've overstepped this.

Commits
-------

f10227d [FrameworkBundle][Controller] Use interface from Contracts to remove deprecation warnings

@tomtomau tomtomau deleted the tomtomau:fix-controller-dep branch Dec 9, 2018

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