Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle] fix BC-breaking property in WebTestAssertionsTrait#31880
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas commentedJun 5, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Why is this a trait BTW, and not plain methods on WebTestCase? |
4860faf to835280bCompareOskarStark commentedJun 5, 2019
I can’t remember why, maybe@Pierstoval can help us |
Pierstoval commentedJun 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. |
835280b to3b50107Compare3b50107 to6253926Compare| publicstaticfunctionassertResponseIsSuccessful(string$message =''):void | ||
| { | ||
| self::assertThat(static::getResponse(),newResponseConstraint\ResponseIsSuccessful(),$message); | ||
| self::assertThat(self::getResponse(),newResponseConstraint\ResponseIsSuccessful(),$message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Traits should be self contained, we shouldn't provide these as extensibility points.
| } | ||
| privatestaticfunctiongetClient():KernelBrowser | ||
| privatestaticfunctiongetClient(KernelBrowser$newClient =null):?KernelBrowser |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why would it returnnull? 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 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
related to my answer to your other comment:getClient(null) returnsnull
| 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()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What is the reason to not useif ($newClient) {?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Because we want to be able to reset the state usinggetClient(null), whilegetClient() shouldn't reset anything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Seems fair
fabpot commentedJun 6, 2019
Thank you@nicolas-grekas. |
…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
No properties should be exposed.