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

Fix PHP 8.3 breakage by declaring missing properties #17

Merged
merged 1 commit into from Jan 23, 2024

Conversation

dlangille
Copy link
Contributor

fixes #16

@flack
Copy link
Owner

flack commented Jan 16, 2024

Thanks for the PR!

Do you think there is any way to trigger the errors you're seeing in unit tests? When I run phpunit locally on PHP 8.3.1, I get no notices (granted, we don't really have a lot of test coverage, but maybe it's easy to add something for this)

@flack
Copy link
Owner

flack commented Jan 16, 2024

P.S.: Looking at the source code, both _timeout and truncSize are never read, they are only set. I wonder if we could simply remove them instead (then we also wouldn't need tests for that :-))

@dlangille
Copy link
Contributor Author

P.S.: Looking at the source code, both _timeout and truncSize are never read, they are only set. I wonder if we could simply remove them instead (then we also wouldn't need tests for that :-))

Unless your users have become accustomed to using it. ;) I checked my code, I'm not using it.

@dlangille
Copy link
Contributor Author

Thanks for the PR!

Do you think there is any way to trigger the errors you're seeing in unit tests? When I run phpunit locally on PHP 8.3.1, I get no notices (granted, we don't really have a lot of test coverage, but maybe it's easy to add something for this)

At https://www.freshports.org - search for newsfeed, which takes you to:

https://www.freshports.org/backend/newsfeeds.php

Clicking on https://www.freshports.org/backend/news.php generated the first error.

Then I clicked on https://www.freshports.org/backend/ - At this URL, when invoking opml (I think), it generated the second error.

https://github.com/FreshPorts/freshports/blob/main/www/backend/news.php is the main URL

https://github.com/FreshPorts/freshports/blob/main/classes/newsfeed.php#L60 is my main generation script

@flack
Copy link
Owner

flack commented Jan 18, 2024

Unless your users have become accustomed to using it. ;) I checked my code, I'm not using it.

well, strictly speaking your change is also breaks backwards compatibility, because these variables were implicitly public before. I guess a good compromise solution might be to remove truncSize (which obviously was copy n pasted between classes), anyone who needs the value of 500 can just set that in their own child class. _timeout we can keep as a protected variable like you suggest, because it's kind of part of the method signature, and who knows, maybe this is actually useful for some child classes somewhere

@flack flack merged commit b912ff2 into flack:master Jan 23, 2024
8 checks passed
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.

PHP 8.3
2 participants