Skip to content

DX: Make AbstractFixerTestCase::getTestFile() final#7116

Merged
Wirone merged 1 commit intoPHP-CS-Fixer:masterfrom
kayw-geek:bugfix/static-call-get-test-file-method
Jul 6, 2023
Merged

DX: Make AbstractFixerTestCase::getTestFile() final#7116
Wirone merged 1 commit intoPHP-CS-Fixer:masterfrom
kayw-geek:bugfix/static-call-get-test-file-method

Conversation

@kayw-geek
Copy link
Copy Markdown
Contributor

@kayw-geek kayw-geek commented Jul 4, 2023

#6860 This PR mark the getTestFile() method to static, but it still be use self:: called in this class.
This will result in the overridden getTestFile() method by the user not being called correctly.

@kayw-geek kayw-geek changed the title Static call the getTestFile() method bug: Static call the getTestFile() method Jul 4, 2023
@Wirone
Copy link
Copy Markdown
Member

Wirone commented Jul 4, 2023

The question is: should getTestFile() be overridden? Looking at the implementation I don't see the point in overriding it. Maybe it should be private or final instead, @kubawerlos?

@kayw-geek anyway, if your fix is correct, we need test case for that 🙂.

@kayw-geek
Copy link
Copy Markdown
Contributor Author

kayw-geek commented Jul 4, 2023 •
edited
Loading

The question is: should getTestFile() be overridden? Looking at the implementation I don't see the point in overriding it. Maybe it should be private or final instead, @kubawerlos?

@kayw-geek anyway, if your fix is correct, we need test case for that 🙂.

I didn't pass the file parameter to the doTest() method before, but instead I overrode the getTestFile() method. Now it seems that only passing the file parameter is indeed sufficient.

My current idea is the same as yours, whether it should be changed to a final method to avoid others and myself from doing the same.

@Wirone I changed the method to final, could you please review the changes again?

@kayw-geek kayw-geek force-pushed the bugfix/static-call-get-test-file-method branch from 193ea12 to 1e35d95 Compare July 6, 2023 09:35
@kayw-geek kayw-geek changed the title bug: Static call the getTestFile() method DX: final the getTestFile() method Jul 6, 2023
@kayw-geek kayw-geek force-pushed the bugfix/static-call-get-test-file-method branch from 1e35d95 to e32a8fe Compare July 6, 2023 09:59
Wirone
Wirone approved these changes Jul 6, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@Wirone Wirone left a comment

Choose a reason for hiding this comment

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

It would be a breaking change if class wasn't @internal 😉. But this makes sense 👍.

@Wirone Wirone changed the title DX: final the getTestFile() method DX: Make AbstractFixerTestCase::getTestFile() final Jul 6, 2023
@Wirone Wirone merged commit 398c2e6 into PHP-CS-Fixer:master Jul 6, 2023
@Wirone
Copy link
Copy Markdown
Member

Wirone commented Jul 6, 2023

Thanks @kayw-geek and welcome into Fixer collaborators team 🍻!

By mistake posted it into squash commit's message 😂 🤦 .

@kayw-geek kayw-geek deleted the bugfix/static-call-get-test-file-method branch July 7, 2023 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Wirone Wirone Wirone approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kayw-geek @Wirone