Skip to content

Fix test fixtures with deprecated method signatures #33484

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

Merged
merged 2 commits into from
Sep 6, 2019

Conversation

derrabus
Copy link
Member

@derrabus derrabus 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 bugfix/update-fixtures branch 3 times, most recently from 270a42b to 7f638b2 Compare September 6, 2019 09:12
@@ -159,17 +104,6 @@ public function testGetResponseNull()
$this->assertNull($client->getResponse());
}

public function testGetInternalResponse()
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

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

@nicolas-grekas nicolas-grekas force-pushed the bugfix/update-fixtures branch 3 times, most recently from 77167df to 3ecb1ea Compare September 6, 2019 09:32
@derrabus
Copy link
Member Author

derrabus 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
Copy link
Member

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
Copy link
Member

Thank you @derrabus.

nicolas-grekas added a commit that referenced this pull request Sep 6, 2019
…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
@derrabus derrabus deleted the bugfix/update-fixtures branch September 6, 2019 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants