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

chore: solve PHPUnit 10 deprecations #918

Merged
merged 8 commits into from
Nov 27, 2024

Conversation

jordyvanderhaegen
Copy link
Collaborator

No description provided.

@jordyvanderhaegen jordyvanderhaegen merged commit fea226d into mcamara:master Nov 27, 2024
12 checks passed
@jordyvanderhaegen jordyvanderhaegen deleted the shift-134256 branch November 27, 2024 19:24
@iwasherefirst2
Copy link
Collaborator

iwasherefirst2 commented Dec 1, 2024

Hi @jordyvanderhaegen,

Thanks for your contribution! I noticed that you made $testUrl static instead of using it as a class property. I'm curious about the reasoning behind this choice—was it to emphasize that it's an unchanging value? If so, do you think using a constant might be more suitable for that purpose?

Additionally, I was wondering if we could rename testURL and testURL2 to something more descriptive, like LOCALHOST_WITH_DASH and LOCALHOST_WITHOUT_DASH. It might make the intent a bit clearer to others reading the code.

If you agree, I'd be happy to make these changes and push an update to your PR. Let me know what you think! 😊

@jordyvanderhaegen
Copy link
Collaborator Author

@iwasherefirst2 the reason behind changing it to static was quite simple, PHPUnit 10 requires data providers to be static. I didn't really bother improving it to be a constant, since it was a bit out of scope for this PR.

I'm not totally sure why the testUrl2 is even a thing, it doesn't provide any value. I've removed it in #922 😄

@iwasherefirst2
Copy link
Collaborator

Looks great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants