8000 Fix `$this` calls to static ones when relevant by alexandre-daubois · Pull Request #58373 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Fix $this calls to static ones when relevant #58373

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 25, 2024
Merged

Fix $this calls to static ones when relevant #58373

merged 1 commit into from
Sep 25, 2024

Conversation

alexandre-daubois
Copy link
Member
Q A
Branch? 7.2
Bug fix? no
New feature? no
Deprecations? no
Issues -
License MIT

@carsonbot carsonbot added this to the 7.2 milestone Sep 24, 2024
GromNaN
GromNaN reviewed Sep 25, 2024
View reviewed changes
Copy link
Member
@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

All the PHPUnit assertions are static methods called on $this. Unless this calls are nested in static methods (data provider, class events), I don't see the point of changing this.

@alexandre-daubois
Copy link
Member Author

All the PHPUnit assertions are static methods called on $this

Unfortunately, this is not exactly true. There are more than 1200 occurrences of self::assert in the codebase. I know there's a will to make this consistent, once there's a way to automate the change in the CI (with fabbot for instance). I can't find the discussion again though.

But this is why I didn't make the change on tests. There are just too many and the decision hasn't been fully made. I think however that it's always good thing to be fully accurate on (non) static calls elsewhere in sources, especially since there are only 8 occurrences of "wrong" calls.

nicolas-grekas
nicolas-grekas approved these changes Sep 25, 2024
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Works for me, even-though this provides nothing in practice.

I'm 👎 to do this for test cases. That'd be just noise in the git history and overhead for maintainers.

@nicolas-grekas
Copy link
Member

Thank you @alexandre-daubois.

@nicolas-grekas nicolas-grekas merged commit db47683 into symfony:7.2 Sep 25, 2024
9 of 10 checks passed
@alexandre-daubois alexandre-daubois deleted the static-calls branch September 25, 2024 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GromNaN GromNaN GromNaN left review comments

@javiereguiluz javiereguiluz javiereguiluz approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@xabbuh xabbuh xabbuh approved these changes

@lyrixx lyrixx Awaiting requested review from lyrixx lyrixx is a code owner

@chalasr chalasr Awaiting requested review from chalasr chalasr is a code owner

Assignees
No one assigned
Labels
Status: Reviewed
Projects
None yet
Milestone
7.2
Development

Successfully merging this pull request may close these issues.
6 participants
@alexandre-daubois @nicolas-grekas @javiereguiluz @GromNaN @xabbuh @carsonbot
0