iconv_substr behaves differently under PHP8

[Megatherium]

While testing out software with PHP8 (8.0.0~rc1-6+ubuntu18.04.1+deb.sury.org+1) I got a TypeError from mktime during basic usage of Zend_Date.

<?php

require_once 'vendor/autoload.php';

$date = new Zend_Date('2020-01-01');
$date->set(0, Zend_Date::TIMES);
echo $date.PHP_EOL;

Resulted in this with PHP8

PHP Fatal error:  Uncaught TypeError: gmmktime(): Argument #2 ($minute) must be of type ?int, string given in /somepath/vendor/zf1s/zend-date/library/Zend/Date/DateObject.php:156
Stack trace:
#0 /somepath/vendor/zf1s/zend-date/library/Zend/Date/DateObject.php(156): gmmktime()
#1 /somepath/vendor/zf1s/zend-date/library/Zend/Date.php(2277): Zend_Date_DateObject->mktime()
#2 /somepath/vendor/zf1s/zend-date/library/Zend/Date.php(1078): Zend_Date->_calculate()
#3 /somepath/zd.php(7): Zend_Date->set()
#4 {main}
  thrown in /somepath/vendor/zf1s/zend-date/library/Zend/Date/DateObject.php on line 156

mktime isn't the culprit in this case though. It turned out that Zend_Locale_Format was acting differently:

<?php

require_once 'vendor/autoload.php';

var_dump(Zend_Locale_Format::getTime(0));

7.4:

array(5) {
  'date_format' =>
  string(9) "h:mm:ss a"
  'locale' =>
  string(5) "en_US"
  'hour' =>
  string(1) "0"
  'minute' =>
  bool(false)
  'second' =>
  bool(false)
}

8.0:

array(5) {
  'date_format' =>
  string(9) "h:mm:ss a"
  'locale' =>
  string(2) "en"
  'hour' =>
  string(1) "0"
  'minute' =>
  string(0) ""
  'second' =>
  string(0) ""
}

As it turns oyt iconv_substr behaviour has been normalized and will now always return an empty string if the offset is out of bounds.

We have 73 usages of iconv_substr excluding tests? What's the best way to tackle this? Write a wrapper function that generates the output of iconv_substr <8.0 and replace all calls to iconv_substr with the wrapper function?

[falkenhawk]

That's a great catch! Thank you @Megatherium
I wonder, maybe it's not needed to wrap all calls - maybe it some places it will still behave as expected, only there where it expects false explicitly, it would need to be changed. But I know it's going to require quite a lot of effort to go through all of them, so just using a wrapper everywhere would be easier...

[glensc]

I would also prefer if only single use cases are fixed. might bring up other issues in the code.

I quickly skimmed over symfony/polyfill, do they deal with this problem solving, but seems no.

[glensc]

Perhaps it's regression created by this project?

Found this line in changelog:

CHANGELOG.md: - iconv_substr php 7.0.11+ compatibility fixes - borrowed from axot/zf1@4c6400 (thanks!)

[Megatherium]

Perhaps it's regression created by this project?

Depends on how you wanna look at it.

<?php

foreach (['a', 'ab', 'abc'] as $str) {
    var_dump(iconv_substr($str, 2, 2));
}

for i in 56 74 80 ; do echo $i ; docker run -v /tmp:/tmp --rm phpdockerio/php$i-cli php /tmp/iconv_test.php; done
56
bool(false)
bool(false)
string(1) "c"
74
bool(false)
string(0) ""
string(1) "c"
80
string(0) ""
string(0) ""
string(1) "c"

Also I'm encoutering some other stuff on the way that needs fixing for the tests to succeed. Do you want to keep the scope of the PR narrow or should I push basically a full PHP8 compat branch?

[falkenhawk]

a full PHP8 compat branch would be the bomb 💣💥 💚

[Megatherium]

The PHP8 branch is basically ready.
Before I can push it though I need a small fix in phpunit merged.

[Megatherium]

Ok. Everything's ready. Everything passes, except: 5.4 is suddenly failing at a point where nothing was even changed. That behaviour is something you'd expect from PHP7+ (was is why that test is skipped on 7+). Doesn't make any sense to me.

Apart from that: should I hold the PR until Travis has a real 8.0 image and not snapshot/nightly?

[falkenhawk]

Apart from that: should I hold the PR until Travis has a real 8.0 image and not snapshot/nightly?

Nah, I am afraid it might be a long wait 🙈

Please create the PR, I'm looking forward to it! I will try to look into the php 5.4 issue.

[glensc]

I think the date issues were resolved with:

• Make Zend_Validate_Date work as expected under PHP 8.0 #75
• Fix a bug in Zend_Date for PHP 8.0 #62

so this issue can be closed?

[falkenhawk]

It looks like it. Thank you for noticing.