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

SplFixedArray usage is incompatible with PHP 8.0 #151

Closed yurikuzn closed 3 years ago

yurikuzn commented 3 years ago

Hi,

As of PHP 8.0, SplFixedArray is not an Iterator, so it can't be passed to the constructor of NoRewindIterator.

The place where the problem occurs: https://github.com/zbateson/mail-mime-parser/blob/1.3.0/src/Header/Consumer/AbstractConsumer.php#L126

PHP error produced:

PHP Fatal error:  Uncaught TypeError: NoRewindIterator::__construct():
Argument #1 ($iterator) must be of type Iterator, SplFixedArray given in ...

See: https://github.com/php/php-src/blob/php-8.0.0RC4/UPGRADING#L519

zbateson commented 3 years ago

Thanks for bringing that to my attention @yurikuzn

phpfui commented 3 years ago

I added a bug report to php.net here, but may be a documentation issue as stated above.

phpfui commented 3 years ago

Quick Response from the PHP team: As of PHP 8.0.0, SplFixedArray no longer implements Iterator, but rather IteratorAggregate. Use the ::getIterator() method to get the Iterator: https://3v4l.org/13jav.

This is documented in the migration guide[1], but not in the manual proper.

Would you like a PR for this?

zbateson commented 3 years ago

Sure, that would be great! We'll need to maintain backward compatibility as well... if you have time it would be very much appreciated :+1:

phpfui commented 3 years ago

Will do no later than tonight. Thinking for backward compatibility going with ArrayIterator. I did this in my local copy on my PHP 8.0 server, and it seems to be fine. Do you really need a SplFixedArray here?

Happy to help. Lots of libraries are experiencing PHP 8.0 issues, and not everyone is as responsive as you are.

zbateson commented 3 years ago

I'm pretty sure it's just this one call:

new NoRewindIterator(SplFixedArray::fromArray($tokens))

I just need an iterator from an array basically... it doesn't need to be SplFixedArray.

phpfui commented 3 years ago

ArrayIterator will work then.

I found this problem with my phpfui/phpunit-syntax-coverage package. Should I add that as a PHPUnit test? I have found lots of things with this package. And the beauty of it is, the ReflectionClass stuff is built into PHP, so always updated with the latest, and it does not require code execution to find issues, so you get a much better coverage than the more traditional approaches.

Let me know. Should have something tonight as this package is not used in my work project.

yurikuzn commented 3 years ago

This should work:

new NoRewindIterator(
    (
        new IteratorIterator(
            new ArrayIterator($tokens)
        )
    )->getInnerIterator()
)

Looks gnarly.

phpfui commented 3 years ago

Even simpler:

        return $this->parseTokensIntoParts(new NoRewindIterator(new ArrayIterator($tokens)));

We just need an iterator on an array. SplFixedArray was an iterator on 7.4, but not 8.0 Also SplFixedArray::getIterator() will also work, but only on PHP 8.0, so it is a no go. I can't believe the PHP team did not depreciate this in 7.4, or in 8.0 and remove it in the next release.

phpfui commented 3 years ago

I have the fix done, and unit tests run in 7.1 and higher. The problem is we can't support unit tests for PHP 8.0 and anything below PHP 7.1, as the setUp() method must now return void, which PHP 7.0 and lower don't support.

I'll put just the source change in the PR. I'll do another branch of the 7.1 tests just so you have them.

zbateson commented 3 years ago

Merged #154 to master

zbateson commented 3 years ago

Released in 1.3.1