zbateson / mail-mime-parser

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

Changed return type in php 8.2 #207

Closed tanthammar closed 1 year ago

tanthammar commented 1 year ago

php 8.2 methods DateTime::createFromImmutable and DateTimeImmutable::createFromMutable has a tentative return type of static.

src/Header/DateHeader.php method getDateTimeImmutable() (row 51) needs a changed return type.

    /**
     * Returns a DateTimeImmutable for the part's DateTime object, or null if
     * the date could not be parsed.
     *
     * @return DateTimeImmutable|null The parsed DateTimeImmutable object.
     */
    public function getDateTimeImmutable()
    {
        $dateTime = $this->getDateTime();
        if ($dateTime !== null) {
            return DateTimeImmutable::createFromMutable($dateTime);
        }
        return null;
    }

I don't dare to make a PR because I don't have any knowledge of this pkg other then that it is required by spatie/laravel-ray.

zbateson commented 1 year ago

Hi @tanthammar --

Just looking into this change. What should the return type be changed to? DateTimeImmutable::createFromMutable will always return a DateTimeImmutable object, it would only change if I called SomeSubclassOfDateTimeImmutable::createFromMutable, and even then @return DateTimeImmutable would still be valid, no?

Had a look around but couldn't figure out anything specific that this would change on my end -- sorry if I'm being dense.

phpfui commented 1 year ago

I believe the problem is the method has two return types (null and DateTimeImmutable). Since you are still supporting PHP 5.4 (probably should be 7.1 at a minimum as this point in time), you can't specify a return type, much less a nullable type (?DateTimeImmutable), PHP 8.0 and higher will complain.

The way to fix this for older code is to put in an attribute comment like so:

#[\ReturnTypeWillChange]

This is a backward compatible change, since it is a comment, but PHP 8.0 and higher will stop complaining. When you drop PHP 7.0 and lower support, you can change it to return ?DateTimeImmutable and remove the attribute.

Hope this helps.

zbateson commented 1 year ago

I don't specify a return type, I documented one. I think it's crazy that PHP8 actually looks at that.

Since you are still supporting PHP 5.4 (probably should be 7.1 at a minimum as this point in time)

I don't do it to piss people off, it's my own use case. I haven't had a chance to look at this in a while tho, I may be able to bump up to 5.6 or 7-something but I need to selfishly look at where I'm using it. RHEL7 still supports php5.4, which has been why I've needed it.

Since it's just documentation though, couldn't I change the docs to return ?DateTimeImmutable?

phpfui commented 1 year ago

Yes, but not sure it that will resolve the warning. Worth trying, as that is what it would be PHP 7.1 compatible.

If you bump up the package to 7.1, people using 7 or less just would no longer receive updates. They could still use the older version. Worth considering, as parameter and return types are very helpful.

Also looks like there are issues with depreciations in 8.1 and 8.2 8.2 is mostly not declaring the $stream property. I tried doing that, but unit tests still failed with trying to access a method on null. Not sure what the problem is as the code is fairly complex, but looks like it should not work under the older versions as well. It was very confusing for me to sort out. Something to look into as this will cause this package to not work sometime in the near future.

phpfui commented 1 year ago

Also looks like RHEL7 supports 7.3, which would be a good reason to bump the package to 7.1. 7.2 and 7.3 don't really add anything in terms of code changes.

zbateson commented 1 year ago

If you bump up the package to 7.1, people using 7 or less just would no longer receive updates. They could still use the older version. Worth considering, as parameter and return types are very helpful.

Yeah was thinking that -- I'm back to work next week and will take a look at my usage.

Also looks like there are issues with depreciations in 8.1 and 8.2 8.2 is mostly not declaring the $stream property. I tried doing that, but unit tests still failed with trying to access a method on null.

Okay, thanks for that and I'll take a look at that as well. Might be hints in the guzzlehttp/psr7 library I'm using, they probably support php8.1 and 2.

phpfui commented 1 year ago

Happy to help with anything. Let me know what you need.

Two packages for dev I can recommend are:

Happy to add either or both. Really helps with the code.

zbateson commented 1 year ago

Amazing, thank you :grin: -- yeah happy to have those added.

If you have time to look at the stream issue feel free (or this one either changing to ?DateTimeImmutable or adding ReturnTypeWillChange). If you're lost or need any pointers or have questions feel free to reach out here, in discussions, or my email (my github username at gmail.com).

Thanks again

phpfui commented 1 year ago

OK, will do a PHP 8.1 branch to upgrade depreciations first. Then add PHPStan and PHPCSFixer. You may have to do the PHP 8.2 port, but I might understand the code better after the last two upgrades!