Skip to content

Conversation

@mhagstrand
Copy link
Contributor

Also added error message for do_bind_function()

@mhagstrand mhagstrand changed the title BUG 73585: Logging of "Internal Zend error" missing class name Fix #73585: Logging of "Internal Zend error" missing class name Nov 23, 2016
Also added error message for do_bind_function()
@laruence
Copy link
Member

bug #73583 has been fixed, and this is actually a side-affect of that bug, which means, it also has been gone altogether with #73583.

@laruence laruence closed this Nov 23, 2016
@mhagstrand
Copy link
Contributor Author

mhagstrand commented Nov 23, 2016

Hi @laruence

I don't believe this a side-effect but rather a regression that can be seen starting from the two links below:
88eae43#diff-3a8139128d4026ce0cb0c86beba4e6b9L1026

88eae43#diff-3a8139128d4026ce0cb0c86beba4e6b9L1033

The class name cannot be found in Z_STRVAL_P(rtd_key) but rather at Z_STRVAL_P(lcname).

That can be seen here:
https://github.com/php/php-src/pull/2219/files#diff-3a8139128d4026ce0cb0c86beba4e6b9R1079

I understand this is an error condition that shouldn't occur but it seems like outputting the class as it previous did should continue.

Thanks
Mitch

@nikic
Copy link
Member

nikic commented Nov 23, 2016

Agree with @mhagstrand. RTD keys always start with a NUL byte, so printing them using %s doesn't make sense, it'll always be empty. Using the lcname is the next best thing we have here.

@nikic nikic reopened this Nov 23, 2016
@laruence
Copy link
Member

I see, then only last one zend_error_noreturn need to be fixed, I'll take care of it,

thanks

@laruence laruence closed this Nov 24, 2016
@laruence
Copy link
Member

I can not image how that class entry could not be found(unless there was some bug like #73583), so I simply removed the zend_error_return. and this is why I think this is not a bug at the first the place. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants