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

[FrameworkBundle] fix BC-breaking property in WebTestAssertionsTrait #31880

Merged
merged 1 commit into from Jun 6, 2019

Conversation

Projects
None yet
5 participants
@nicolas-grekas
Copy link
Member

commented Jun 5, 2019

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

No properties should be exposed.

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

Why is this a trait BTW, and not plain methods on WebTestCase?
Unless I missed it, #30813 doesn't tell about that, does it?

@OskarStark

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

I can’t remember why, maybe @Pierstoval can help us

@Pierstoval

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

The trait was not discussed, I thought it was convenient to have all assertions in one place, especially when needing them statically. A fix to make the client static in the trait is welcomed, indeed.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:fix-webtestassertionstrait branch from 835280b to 3b50107 Jun 6, 2019

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:fix-webtestassertionstrait branch from 3b50107 to 6253926 Jun 6, 2019

@@ -30,12 +30,12 @@ trait WebTestAssertionsTrait
{
public static function assertResponseIsSuccessful(string $message = ''): void
{
self::assertThat(static::getResponse(), new ResponseConstraint\ResponseIsSuccessful(), $message);
self::assertThat(self::getResponse(), new ResponseConstraint\ResponseIsSuccessful(), $message);

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 6, 2019

Author Member

Traits should be self contained, we shouldn't provide these as extensibility points.

}
private static function getClient(): KernelBrowser
private static function getClient(KernelBrowser $newClient = null): ?KernelBrowser

This comment has been minimized.

Copy link
@Pierstoval

Pierstoval Jun 6, 2019

Contributor

Why would it return null? If the client is null, it should alway throw an exception, shouldn't it? If it's null, all the rest of the trait's codebase would be forced to check for null pointer 😕

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 6, 2019

Author Member

related to my answer to your other comment: getClient(null) returns null

static::fail(\sprintf('A client must be set to make assertions on it. Did you forget to call "%s::createClient"?', __CLASS__));
static $client;
if (0 < \func_num_args()) {

This comment has been minimized.

Copy link
@Pierstoval

Pierstoval Jun 6, 2019

Contributor

What is the reason to not use if ($newClient) {?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 6, 2019

Author Member

Because we want to be able to reset the state using getClient(null), while getClient() shouldn't reset anything

This comment has been minimized.

Copy link
@Pierstoval

Pierstoval Jun 6, 2019

Contributor

Seems fair

@fabpot

fabpot approved these changes Jun 6, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 6253926 into symfony:4.3 Jun 6, 2019

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 Jun 6, 2019

bug #31880 [FrameworkBundle] fix BC-breaking property in WebTestAsser…
…tionsTrait (nicolas-grekas)

This PR was merged into the 4.3 branch.

Discussion
----------

[FrameworkBundle] fix BC-breaking property in WebTestAssertionsTrait

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

No properties should be exposed.

Commits
-------

6253926 [FrameworkBundle] fix BC-breaking property in WebTestAssertionsTrait

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:fix-webtestassertionstrait branch Jun 6, 2019

@fabpot fabpot referenced this pull request Jun 6, 2019

Merged

Release v4.3.1 #31901

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