Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Make string length for getTraceAsString() configurable #5769

Closed

Conversation

TysonAndre
Copy link
Contributor

@TysonAndre TysonAndre commented Jun 25, 2020

Add a zend.exception_string_param_max_len ini setting.
(same suffix as log_errors_max_len)

Allow values between 0 and 1000000 bytes.
For example, with zend.exception_string_param_max_len=0,
"" would represent the empty string, and "..." would represent something
longer than the empty string.
Previously, this was hardcoded as exactly 15 bytes.

RFC: https://wiki.php.net/rfc/throwable_string_param_max_len
Discussion: https://externals.io/message/110717

@TysonAndre TysonAndre force-pushed the throwable_string_length_limit branch from 5fd7888 to e2615b9 Compare June 27, 2020 02:48
@TysonAndre TysonAndre changed the title [Proposal] Make string length for getTraceAsString() configurable [RFC] Make string length for getTraceAsString() configurable Jul 1, 2020
@TysonAndre TysonAndre force-pushed the throwable_string_length_limit branch 2 times, most recently from 55fe472 to b6d7cbb Compare July 3, 2020 02:02
@nikic nikic added this to the PHP 8.0 milestone Jul 22, 2020
Zend/zend_exceptions.c Outdated Show resolved Hide resolved
@@ -18,6 +18,7 @@
#define PHP_GLOBALS_H

#include "zend_globals.h"
#include "php.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems unrelated?

Copy link
Contributor Author

@TysonAndre TysonAndre Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been a while, I think I added this to fix a compilation error I saw when php_globals.h was loaded without loading a header it depended on first.

But that may have been limited to an older version of this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, why is this in php_globals.h at all though? This is Zend functionality, so it should be in zend_globals.h. As is, this causes a layering violation by including PHP headers in Zend.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, now I remember.

The issue was that main/php.h includes the #define PHPAPI, and this file contains extern PHPAPI int core_globals_id; , so if a C file loaded only main/php_globals.h, it would have a compilation error.

That's out of the scope of this RFC, so I'm taking it out.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure this didn't get lost in a race condition, did you see my comment on moving this from php_globals.h into zend_globals.h?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure this didn't get lost in a race condition, did you see my comment on moving this from php_globals.h into zend_globals.h?

I'm not sure if I understood exactly how you wanted me to implement this, and I'd think that would be more appropriate outside of this PR. It might be easier for you to create a PR/commit than to explain it in that much detail.

Also, that predated my response

The issue was that main/php.h includes the #define PHPAPI, and this file contains extern PHPAPI int core_globals_id; , so if a C file loaded only main/php_globals.h, it would have a compilation error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I took out the include "php.h"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for being unclear. What I meant is to move the declaration of zend_long exception_string_param_max_len; from php_core_globals into zend_executor_globals, which is the same place where exception_ignore_args is defined. We should not have dependencies on PHP headers from inside Zend.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for being unclear. What I meant is to move the declaration of zend_long exception_string_param_max_len; from php_core_globals into zend_executor_globals, which is the same place where exception_ignore_args is defined. We should not have dependencies on PHP headers from inside Zend.

That sounds doable and makes sense

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for being unclear. What I meant is to move the declaration of zend_long exception_string_param_max_len; from php_core_globals into zend_executor_globals, which is the same place where exception_ignore_args is defined. We should not have dependencies on PHP headers from inside Zend.

Okay, done in bcc7ade

@nikic
Copy link
Member

nikic commented Jul 22, 2020

Please see 9250abf for the correct format to use in php.ini-production/development. This was messed up for zend.exception_ignore_args a bit.

@TysonAndre TysonAndre force-pushed the throwable_string_length_limit branch 2 times, most recently from a40e22c to dabdeec Compare July 22, 2020 13:58
@TysonAndre TysonAndre force-pushed the throwable_string_length_limit branch from dabdeec to a004e00 Compare July 22, 2020 14:13
Add a `zend.exception_string_param_max_len` ini setting.
(same suffix as `log_errors_max_len`)

Allow values between 0 and 1000000 bytes.
For example, with zend.exception_string_param_max_len=0,
"" would represent the empty string, and "..." would represent something
longer than the empty string.
Previously, this was hardcoded as exactly 15 bytes.

Discussion: https://externals.io/message/110717
@TysonAndre TysonAndre force-pushed the throwable_string_length_limit branch from a004e00 to bcc7ade Compare July 25, 2020 13:08
Zend/zend.c Outdated Show resolved Hide resolved
@php-pulls php-pulls closed this in 07db641 Jul 25, 2020
php-pulls pushed a commit that referenced this pull request Jul 27, 2020
jrfnl added a commit to PHPCompatibility/PHPCompatibility that referenced this pull request Jul 30, 2020
…getTraceAsString()

> - zend.exception_string_param_max_len
>   . New INI directive to set the maximum string length in an argument of a
>   stringified stack strace.

Refs:
* https://wiki.php.net/rfc/throwable_string_param_max_len
* https://github.com/php/php-src/blob/47e4001976f3ff85a6d479df39dc82d4ceb18b19/UPGRADING#L957-L959
* php/php-src#5769
* php/php-src@07db641

Includes unit tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants