symphonycms/symphonycms

Error reporting is set too late

michael-e opened this issue · 16 comments

michael-e commented

Affected Symphony version(s) : 2.7.x
PHP version(s) : 7.2
OS(es) : all

Officially Symphony 2.7 is not supported on PHP 7.2. But, given the fact that it's an LTS release, I nevertheless suggest to keep it working if possible.

PHP 7.2 has an interesting bug. Accoording to the docs, the variant parameter's default value for the idn_to_utf8 function is INTL_IDNA_VARIANT_2003 — but this value has been deprecated. (No, I am not kidding.) So calling the function without the parameter will raise a deprecation warning. The official bug report can be seen here: https://bugs.php.net/bug.php?id=75609.

If PHP's default error reporting is set to E_ALL, this PHP bug will cause the deprecation warning to be thrown on every page:

Deprecated: idn_to_utf8(): INTL_IDNA_VARIANT_2003 is deprecated in /var/www/foo/symphony/lib/boot/defines.php on line 218

Why? Symphony sets its own (more forgiving/suppressing) error reporting too late. If you look at index.php, you see:

// Include autoloader:
require_once DOCROOT . '/vendor/autoload.php';

// Include the boot script:
require_once DOCROOT . '/symphony/lib/boot/bundle.php';

Currently Symphony sets the error reporting in bundle.php. But the autoloader will load defines.php before! In this file (line 218) you will see:

define_safe('HTTP_HOST', function_exists('idn_to_utf8') ? idn_to_utf8($http_host) : $http_host);

There are two possible (simple) fixes for this:

  1. Move the line that defines Symphony's default error reporting from bundle.php to index.php (before the autoloader). This would mean that Symphony's error handling is applied to everything that "happens" in the autoloaded files.
  2. Fix only the line that causes the issue, by calling the function like so: idn_to_utf8($http_host, IDNA_DEFAULT, INTL_IDNA_VARIANT_UTS46).

@nitriques: I am open to both solutions, and I can send a PR if you tell me which route to take.

nitriques commented

@michael-e Moving the default error reporting up seems like a breaking change, so for LTS, it leaves us with only solution 2. Are we sure that INTL_IDNA_VARIANT_UTS46 exists in php 5.3 (it looks like it) ? Also, do they produce the same result for the same input ?

As for 3.0.x, I am still hesitant. So please send a PR for 2.7.x with solution 2. (I have a 5.3 setup still available so I can test it)

michael-e commented

Unortunately it is a breaking change no matter which route we take. PHP has screwed up here.

Using INTL_IDNA_VARIANT_UTS46, I do not expect the result to be the same. A further explanation is here: https://wiki.php.net/rfc/deprecate-and-remove-intl_idna_variant_2003. I wonder how this could ever happen. The author explicitly mentions that "code relying on the default variant might break". Nevertheless everyone agreed with this change.

Even worse for us: INTL_IDNA_VARIANT_UTS46 was not introduced before PHP 5.4, so we need to do s.th. like the following:

$http_host = server_safe('HTTP_HOST');
if (function_exists('idn_to_utf8')) {
    // In PHP 7.2, `idn_to_utf8` can not be called with default parameters,
    // because the default for `variant` has been deprecated. However, the
    // alternative variant `INTL_IDNA_VARIANT_UTS46` was not introduced until
    // PHP 5.4, so we must be careful.
    // https://wiki.php.net/rfc/deprecate-and-remove-intl_idna_variant_2003
    // https://bugs.php.net/bug.php?id=75609
    if (defined('INTL_IDNA_VARIANT_UTS46')) {
        $http_host = idn_to_utf8($http_host, 0, INTL_IDNA_VARIANT_UTS46);
    } else {
        $http_host = idn_to_utf8($http_host);
    }
}
define_safe('HTTP_HOST', $http_host);
unset($http_host);

What do you think?

froschdesign commented

@michael-e

Unortunately it is a breaking change no matter which route we take. PHP has screwed up here.

Why is a deprecation warning a breaking change? These warnings are good and important! And these are only hints that your code needs an update for the next versions of PHP. But it still works.
Btw. version 5.3, 5.4, 5.5 are dead. By the end of the year version 5.6 and 7.0 are also dead.

Symphony sets its own (more forgiving/suppressing) error reporting too late.

Symphony should not set anything here! Setting the error reporting to something lower hides only the problems in your code, but do not solve them.

If someone uses a dead or old PHP version, they can hide those deprecation warnings themselves.

michael-e commented

I agree with most of your comment. However, we simply don't have the manpower to fix every warning in Symphony 2.7 LTS. (I have started to work on it, but still I have not come very far. I would be glad if some more experienced programmer would take care of it. We even invented a configuration setting called error_reporting_all which simplifies finding warnings that are suppressed otherwise.)

If someone uses a dead or old PHP version, they can hide those deprecation warnings themselves.

But: The deprecation warning happens in PHP 7.2.

Why is a deprecation warning a breaking change?

It is not. Our attempt to fix it is a breaking change, because we must use INTL_IDNA_VARIANT_UTS46 if possible.

froschdesign commented

@michael-e

However, we simply don't have the manpower to fix every warning in Symphony 2.7 LTS.

Right, a lot of work for the entire codebase.

I have started to work on it…

Great! 👍

But: The deprecation warning happens in PHP 7.2.

Sorry, I forgot to add a comment for solution / workaround: Your solution is a typical way to solve this type of problem.

(Notice: If a user-defined function is allowed to fill the gap, then you should use function_exists('idn_to_utf8') otherwise extension_loaded('intl') – but in this case use the last one.)

michael-e commented

Thanks!

So the suggested fix is:

$http_host = server_safe('HTTP_HOST');
if (extension_loaded('intl')) {
    // In PHP 7.2, `idn_to_utf8` can not be called with default parameters,
    // because the default for `variant` has been deprecated. However, the
    // alternative variant `INTL_IDNA_VARIANT_UTS46` was not introduced until
    // PHP 5.4, so we must be careful.
    // https://wiki.php.net/rfc/deprecate-and-remove-intl_idna_variant_2003
    // https://bugs.php.net/bug.php?id=75609
    if (defined('INTL_IDNA_VARIANT_UTS46')) {
        $http_host = idn_to_utf8($http_host, 0, INTL_IDNA_VARIANT_UTS46);
    } else {
        $http_host = idn_to_utf8($http_host);
    }
}
define_safe('HTTP_HOST', $http_host);
unset($http_host);
  • 👍1
froschdesign commented

Sorry, I missed one return value: false. This case should also be considered!

https://secure.php.net/manual/en/function.idn-to-utf8.php#refsect1-function.idn-to-utf8-returnvalues

michael-e commented

So what do you suggest? Somehting like this?

$http_host = server_safe('HTTP_HOST');
if (extension_loaded('intl')) {
    // In PHP 7.2, `idn_to_utf8` can not be called with default parameters,
    // because the default for `variant` has been deprecated. However, the
    // alternative variant `INTL_IDNA_VARIANT_UTS46` was not introduced until
    // PHP 5.4, so we must be careful.
    // https://wiki.php.net/rfc/deprecate-and-remove-intl_idna_variant_2003
    // https://bugs.php.net/bug.php?id=75609
    if (defined('INTL_IDNA_VARIANT_UTS46')) {
        $host_utf8 = idn_to_utf8($http_host, 0, INTL_IDNA_VARIANT_UTS46);
    } else {
        $host_utf8 = idn_to_utf8($http_host);
    }
    $http_host = ($host_utf8 !== false) ? $host_utf8 : $http_host;
}
define_safe('HTTP_HOST', $http_host);
unset($http_host);
  • 👍1
michael-e commented

Or, maybe clearer:

$http_host = server_safe('HTTP_HOST');
if (extension_loaded('intl')) {
    // In PHP 7.2, `idn_to_utf8` can not be called with default parameters,
    // because the default for `variant` has been deprecated. However, the
    // alternative variant `INTL_IDNA_VARIANT_UTS46` was not introduced until
    // PHP 5.4, so we must be careful.
    // https://wiki.php.net/rfc/deprecate-and-remove-intl_idna_variant_2003
    // https://bugs.php.net/bug.php?id=75609
    if (defined('INTL_IDNA_VARIANT_UTS46')) {
        $host_utf8 = idn_to_utf8($http_host, 0, INTL_IDNA_VARIANT_UTS46);
    } else {
        $host_utf8 = idn_to_utf8($http_host);
    }

    if ($host_utf8 !== false) {
        $http_host = $host_utf8;
    }
}
define_safe('HTTP_HOST', $http_host);
unset($http_host);
nitriques commented

Boy does this change sucks. But I think it is the only viable solution. Here would be my amended version:

$http_host = server_safe('HTTP_HOST');
if (function_exists('idn_to_utf8')) {
    // In PHP 7.2, `idn_to_utf8` can not be called with default parameters,
    // because the default for `variant` has been deprecated. However, the
    // alternative variant `INTL_IDNA_VARIANT_UTS46` was not introduced until
    // PHP 5.4, so we must be careful.
    // https://wiki.php.net/rfc/deprecate-and-remove-intl_idna_variant_2003
    // https://bugs.php.net/bug.php?id=75609
    // @deprecated: This 'hack' can be removed when dropping php < 7.2
    if (defined('INTL_IDNA_VARIANT_UTS46')) {
        $host_utf8 = idn_to_utf8($http_host, 0, INTL_IDNA_VARIANT_UTS46);
    } else {
        $host_utf8 = idn_to_utf8($http_host);
    }

    if ($host_utf8 !== false) {
        $http_host = $host_utf8;
    }
    unset($host_utf8);
}
define_safe('HTTP_HOST', $http_host);
unset($http_host);
  1. I would keep function_exists('idn_to_utf8') because I believe in user supplied versions!
  2. I would also unset $host_utf8
  3. I would add a deprecation notice in the comment
  4. Remember that LTS does not cover php 7.2. 3.0.0 will.
  5. Wait: there's secure.php.net ? WTF !!

So @michael-e, wanna send a PR ? I might be easier to comment on it if a PR is open. Thanks guys.

froschdesign commented

@nitriques

I would keep function_exists('idn_to_utf8') because I believe in user supplied versions!

Mmmh, but your composer.json doesn't include anything! 😉
Or do you know a user-defined function for this case?

michael-e commented

I didn't expect this to get so complicated… well, I will send a PR for further discussion.

@nitriques: Actually the hack could be removed when dropping PHP 5.3. For 5.4 and later we could use idn_to_utf8($http_host, 0, INTL_IDNA_VARIANT_UTS46). Calling the function without with parameters is required in 7.2 and 7.3, since the old default (deprecated in 7.2) will not be changed before 7.4. PHP has really shot itself in the foot here.

michael-e commented

I fixed a typo in the above comment. Sorry for that.

nitriques commented

@froschdesign

Mmmh, but your composer.json doesn't include anything!

Can you elaborate more ? I do not see the relationship... sorry!

@michael-e Thanks!

froschdesign commented

@nitriques

Can you elaborate more ? I do not see the relationship... sorry!

Nothing special: you want allow a user-defined function to fill the gap, but the entire CMS does not use any other external packages or libraries. That is a contradiction.

nitriques commented

@froschdesign Ah, in that sense, valid point. This is changing in 3.0.x, as we will depend on composer and let users install other things with it.

But a well-advised (or ill-advised, it depends!) user like myself will not hesitate to custom patch index.php to provide things I need to get things done.