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
[Tests] New iteration of removing $this
occurrences in future static data providers
#48988
base: 5.4
Are you sure you want to change the base?
[Tests] New iteration of removing $this
occurrences in future static data providers
#48988
Conversation
} | ||
|
||
public function provideMissingStrategies() | ||
{ | ||
yield from $this->provideStrategies('non-existent-file.json'); | ||
yield from static::provideStrategies('non-existent-file.json'); | ||
} | ||
|
||
public function provideStrategies(string $manifestPath) |
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.
public function provideStrategies(string $manifestPath) | |
public static function provideStrategies(string $manifestPath) |
?
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.
The other PR will only change this to static
automatically, if it is directly used as a dataProvider somewhere, is that the case?
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.
I would say yes but isn't it a BC break to do so on a public method?
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.
tests are not covered by the BC policy
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.
/Test/
directory is covered by BC, while /Tests/
not
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.
Oh I wasn't aware of this detail, thank you! Updated the PR
4291a41
to
e9b5e37
Compare
Can you merge this one and #49054 please? |
…c data providers
e9b5e37
to
1386ac2
Compare
That's merged |
I think this can be merged @nicolas-grekas |
Follow-up of #48980