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 test fixtures with deprecated method signatures #33484

Merged
merged 2 commits into from Sep 6, 2019

Conversation

@derrabus
Copy link
Contributor

commented Sep 5, 2019

Q A
Branch? 4.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #33483 (partly)
License MIT
Doc PR N/A

This PR upgrades two fixtures that implemented deprecated method signatures. As far as I can tell, they are used in tests that do not specifically test legacy behavior, so the fixtures should be up to date. Currently, these fixtures cause failing tests on the 4.4 branch.

@nicolas-grekas nicolas-grekas added this to the 4.3 milestone Sep 6, 2019

@nicolas-grekas nicolas-grekas force-pushed the derrabus:bugfix/update-fixtures branch 3 times, most recently from 270a42b to 7f638b2 Sep 6, 2019

@@ -159,17 +104,6 @@ public function testGetResponseNull()
$this->assertNull($client->getResponse());
}
public function testGetInternalResponse()

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Sep 6, 2019

Member

from #30602, this test is wrong: it relies on a class that extends Response while the class is @final
from #31674, it has already been removed from master, despite the fact it was not marked as legacy.
That's two mistakes in a row, fortunately, the recent improvements of DebugClassLoader will allow catching those before they happen :)

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

PR updated. All reported but the ones about the Debug component are legit failures that need action.

@nicolas-grekas nicolas-grekas force-pushed the derrabus:bugfix/update-fixtures branch 3 times, most recently from 77167df to 3ecb1ea Sep 6, 2019

@nicolas-grekas nicolas-grekas force-pushed the derrabus:bugfix/update-fixtures branch from 3ecb1ea to cc3e3d5 Sep 6, 2019

@derrabus

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

TIL: I cannot approve my own pull request. 😅

One question though: We’ve already had checks like this one in place:

if (\func_num_args() < 1 && __CLASS__ !== \get_class($this) && __CLASS__ !== (new \ReflectionMethod($this, __FUNCTION__))->getDeclaringClass()->getName() && !$this instanceof \PHPUnit\Framework\MockObject\MockObject && !$this instanceof \Prophecy\Prophecy\ProphecySubjectInterface) {
@trigger_error(sprintf('The "%s()" method will have a new "Request $request = null" argument in version 5.0, not defining it is deprecated since Symfony 4.2.', __METHOD__), E_USER_DEPRECATED);
}

Are those obsolete now or do we intend to keep them?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

Are those obsolete now or do we intend to keep them?

we intend to keep them, because there is no silver bullet (DebugClassLoader is not mandatory)

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

Thank you @derrabus.

nicolas-grekas added a commit that referenced this pull request Sep 6, 2019
minor #33484 Fix test fixtures with deprecated method signatures (der…
…rabus, nicolas-grekas)

This PR was merged into the 4.3 branch.

Discussion
----------

Fix test fixtures with deprecated method signatures

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #33483 (partly)
| License       | MIT
| Doc PR        | N/A

This PR upgrades two fixtures that implemented deprecated method signatures. As far as I can tell, they are used in tests that do not specifically test legacy behavior, so the fixtures should be up to date. Currently, these fixtures cause failing tests on the 4.4 branch.

Commits
-------

cc3e3d5 Fix more bad tests
592aacf Fix test fixtures with deprecated method signatures.

@nicolas-grekas nicolas-grekas merged commit cc3e3d5 into symfony:4.3 Sep 6, 2019

2 of 3 checks passed

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

@derrabus derrabus deleted the derrabus:bugfix/update-fixtures branch Sep 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.