search

zbateson / mail-mime-parser

An email parser written in PHP
https://mail-mime-parser.org/
BSD 2-Clause "Simplified" License
458 stars 58 forks source link

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

Closed phpfui closed 2 years ago

phpfui commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

And thanks for the library. Good stuff!

My pleasure and thank you for the encouragement :)

zbateson commented 2 years ago

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.

phpfui commented 2 years ago

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?