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

Adding #[\ReturnTypeWillChange] for PHP8.1 compatibility #184

Merged
merged 1 commit into from
Dec 8, 2021
Merged

Adding #[\ReturnTypeWillChange] for PHP8.1 compatibility #184

merged 1 commit into from
Dec 8, 2021

Conversation

phpfui
Copy link
Contributor

@phpfui phpfui commented Dec 7, 2021

PHP 8.1 expects return types to be correctly specified and match the parent class. Missing and mismatched return types are now a depreciated feature. This PR makes sure PHP 8.1 does not emit warnings on missing or mismatched return type.

#[\ReturnTypeWillChange] is an official attribute in 8.1 and comment in previous versions, so this should not be an issue for all versions.

Unfortunately PHP 8.2 or 8.3 will probably matching return types, so that means supporting anything less than 7.1 will not be possible. At this point, 7.0 and lower have about 2% share according to Packagist.org, so dropping support for 7.0 and lower would probably not have a big impact on users. They can still use the older versions.

@zbateson
Copy link
Owner

zbateson commented Dec 8, 2021

Thanks for this @phpfui --

Is it only needed where you specified it? (or why do other classes/functions not need this? Or is this a WIP?)

@phpfui
Copy link
Contributor Author

phpfui commented Dec 8, 2021

Sorry, I forgot to say. These are the areas I have found issues in production. I support a fairly active forum with attachments and various email clients, that tend to push things. I have been running it for several days and have not seen any additional issues.

But, as I don't really know the code, I can not for certain say these are the only issues. But certainly these are issues. I verified each file / line number from the error messages.

I would issue a patch. Once you support 8.1, you may get other people upgrading to 8.1 and they may find other issues, but easy to fix.

As far as I am concerned, this commit fixes all my issues.

And thanks for the library. Good stuff!

@zbateson
Copy link
Owner

zbateson commented Dec 8, 2021

Hmmm, that's interesting -- I'm wondering what it looks at to determine the return type. Is it looking at docblock @return and complaining? It might be easy enough to fix it that way instead.

Also I'm technically supporting 8.1 already. It's one of the platforms I test on: https://github.com/zbateson/mail-mime-parser/runs/4338280006?check_suite_focus=true

I'm not sure why that doesn't complain. Did github turn off that check maybe?

@zbateson
Copy link
Owner

zbateson commented Dec 8, 2021

And thanks for the library. Good stuff!

My pleasure and thank you for the encouragement :)

@zbateson
Copy link
Owner

zbateson commented Dec 8, 2021

Aah, it only affects subclasses where the base class defines a return type... so the instances you've updated are PHP classes that actually define return types in newer versions of php, versus my classes where no return types are defined at all.

@zbateson zbateson merged commit b969a8a into zbateson:master Dec 8, 2021
@phpfui
Copy link
Contributor Author

phpfui commented Dec 8, 2021

Aah, it only affects subclasses where the base class defines a return type... so the instances you've updated are PHP classes that actually define return types in newer versions of php, versus my classes where no return types are defined at all.

Exactly! I probably did not explain it correctly.

Not sure what github is doing, as I just started switching over to it from Travis, but you want all warnings on. Maybe the have that turned off?

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.

None yet

2 participants