Skip to content
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

Fix deprecation messages in AdminResettingController and AdminResettingController #1201

Merged
merged 2 commits into from Jul 28, 2020

Conversation

phansys
Copy link
Member

@phansys phansys commented Jul 27, 2020

Subject

Fix deprecation messages in AdminResettingController and AdminResettingController, since currently they are making references to non existing classes.

I am targeting this branch, because these changes respect BC.

@phansys phansys requested a review from a team July 27, 2020 16:47
@jordisala1991
Copy link
Member

There might me something wrong on tests:

Since symfony/phpunit-bridge 5.1: Using "@expectedDeprecation" annotations in tests is deprecated, use the "ExpectDeprecationTrait::expectDeprecation()" method instead.

seems like we using the old way of expecting deprecations. Deprecated 😅

@phansys phansys force-pushed the fix_controller_deprecations branch from d7dfd24 to d60215e Compare July 27, 2020 17:24
@phansys
Copy link
Member Author

phansys commented Jul 27, 2020

There might me something wrong on tests:

I've replaced those annotations. The test should be fine now.

use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\RequestStack;

class AdminResettingControllerTest extends TestCase
{
use ExpectDeprecationTrait;
Copy link
Member

Choose a reason for hiding this comment

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

I think this is only needed if you using really old versions of phpunit. on 8.5 we should be fine without it

Copy link
Member Author

Choose a reason for hiding this comment

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

I can confirm it is required. We are using phpunit/phpunit:8.5.0:

<env name="SYMFONY_PHPUNIT_VERSION" value="8.5.0" />

Please, see symfony/symfony#37153.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see, needed then, I was confused with the expectException

@phansys phansys marked this pull request as ready for review July 27, 2020 17:46
@phansys phansys requested a review from a team July 27, 2020 17:46
@phansys phansys mentioned this pull request Jul 27, 2020
jordisala1991
jordisala1991 previously approved these changes Jul 27, 2020
@phansys phansys requested a review from a team July 27, 2020 22:20
@VincentLanglet
Copy link
Member

There should be no changelog, for pedantic isn't it ?

And I think two commit would be better.

  • One with the deprecation message fix
  • One removing the deprecated @expectedDeprecation. (Not sure if it was really needed btw, since we'll remove all of them in the next major ; all the other SonataBundle use this annotation, so now we're in a not-consistent situation 😅)

@phansys
Copy link
Member Author

phansys commented Jul 27, 2020

Changelog section removed, since these changes are pedantic. It was:

Changelog

### Fixed
- Fixed QCNs referenced by deprecation messages at `AdminResettingController` and `AdminResettingController`;
- Fixed deprecated usage of `@exptectedDeprecation` through `ExpectDeprecationTrait::expectDeprecation()`.

@phansys
Copy link
Member Author

phansys commented Jul 28, 2020

There should be no changelog, for pedantic isn't it ?

Changelog removed.

And I think two commit would be better.

Commits splitted.

@jordisala1991 jordisala1991 merged commit 409502c into sonata-project:4.x Jul 28, 2020
@jordisala1991
Copy link
Member

Thank you @phansys

@phansys phansys deleted the fix_controller_deprecations branch July 28, 2020 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants