Remove no longer used "log_errors_max_len" ini directive

[mvorisek]

directive is no longer used, usage removed in https://github.com/php/php-src/pull/5639/files#diff-2978fe1c2c45b4eca89dc476376ddc7193bc4e5e7fff0c7d1c465f057b35a5e6L1199

[cmb69]

Hm, was that removal intentional? If so, that should be noted in the migration guide.

[mvorisek]

Removed by @nikic, I hope so because a cropped error output (very often a long stacktrace) was very hard to use to identify real error source. Also already in PHP 8.0 releases ;-)

@cmb69 are the CI failures related to this PR?

[cmb69]

Yes, at least partially. You need to adjust the number of expected array elements in parse_ini_file_variation3.phpt to 9.

[nikic]

Hm, was that removal intentional? If so, that should be noted in the migration guide.

Oops, it wasn't actually. So yeah, we should either re-introduce the functionality into PHP-8.0 (with an explicit truncation) or drop it entirely (as done here). Not sure which...

[mvorisek]

Drop, please, I am so happy that stack traces from fatal errors are no longer cropped :)

I hope a BC break is enought to reason this :D

[cmb69]

So yeah, we should either re-introduce the functionality into PHP-8.0 (with an explicit truncation) or drop it entirely (as done here). Not sure which...

Apparently, nobody complained so far, so maybe the removal without prior deprecation is fine.

[nikic]

I started a thread on the mailing list in https://externals.io/message/114185 and there were no objections (by dint of there being no response). As such, I think we can go ahead with this. This will need a rebase as more tests using the setting have been added in the meantime.

[mvorisek]

rebased and all occurrences of log_errors_max_len removed

[cmb69]

ext/oci8/tests/error3.phpt likely needs to be adapted (I don't think we're testing oci8 on CI). Anybody with a setup who could run the test? Maybe @cjbj?

[cjbj]

The change to ext/oci8/tests/error3.phpt is good.

[ramsey]

Labeling this as a bugfix since it was a bug that these vestiges remained (even if the real bug was that it was inadvertently removed in the first place 😉 ).

[ramsey]

Should the PHP-8.0 upgrade guide be updated to mention that this no longer exists (works) in PHP 8?

[mvorisek]

@ramsey I think it deserves a line in PHP 8 upgrade as for ex. https://github.com/PrestaShop/PrestaShop/blob/1.7.8.x/install-dev/index_cli.php#L70 is in the code mainly for this - originally, without the echo statement, error was cropped, but now the complete exception/stacktrace is printed

[cmb69]

This was merged into master. If it should go into PHP-8.0 it would need to be cherry picked from there. Note that we usually do it by applying to the lowest branch and then merge upwards till master (cf. https://wiki.php.net/vcs/gitworkflow).

[ramsey]

@cmb69 I was asking whether it needed a mention in the PHP 8.0 upgrade guide because of this, which is already in PHP-8.0:

https://github.com/php/php-src/pull/5639/files#diff-2978fe1c2c45b4eca89dc476376ddc7193bc4e5e7fff0c7d1c465f057b35a5e6L1199

[ramsey]

That said, maybe this should be cherry-picked into PHP-8.0, too. If so, I can revert it from master and follow the usual process. I could see this going either way, but I initially didn't think of it as a bugfix for PHP 8.0.

[ramsey]

I reverted this in master (b5d5d06) and added it to PHP-8.0 (d2d227e) with entries for NEWS and UPGRADING (c2b5284)

/cc @carusogabriel @sgolemon

[ramsey]

@nikic is the ABI break because of the changes to php_globals.h? Can the rest of this be applied without those changes?

[cmb69]

@ramsey, ah, yes, would be helpful to tell users that the INI setting has no effect anymore.

[ramsey]

@mvorisek Since I messed things up, this has been reverted from both master and PHP-8.0. Please feel free to open a new PR against the PHP-8.0 branch that applies the changes to the tests and ini files and also includes an update to UPGRADING to mention that log_errors_max_len no longer has any effect. It cannot be removed from main.c or php_globals.h since that would introduce breaking changes to the application binary interface.

Sorry for the churn.

[mvorisek]

@ramsey I see... Should we really change tests in PHP 8.0? I think we should merge this PR again (eg. cherrypick) into master and then add the UPGRADING/NEWS line into both versions. I prefer if some maintainer can commit this.

[cmb69]

@mvorisek, this patch is still in master; it only has been reverted for PHP-8.0 (see b5c0e7e, which is an empty merge).

[mvorisek]

@cmb69 really? see master branch file tree and b5d5d06

[ramsey]

I had reverted it in master and applied it to PHP-8.0 so that I could then merge PHP-8.0 up into master, but it was then reverted from PHP-8.0.

[mvorisek]

I created two branches:

• changes for PHP 8.0 - https://github.com/mvorisek/php-src/tree/remove_unused_log_maxlen_80
• merged changes for master - https://github.com/mvorisek/php-src/tree/remove_unused_log_maxlen_81

@cmb69 or @nikic please check and commit directly