Skip to content

Object dumping improvements #14429

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Jun 2, 2024

Core: convert bail-out for incorrect __debugInfo return type to TypeError

This commit, with the follow-up to fix the semantics of the object handler; should fix issue #7922.

This commit also changes the API get_debug_info and the get_properties_for (with the ZEND_PROP_PURPOSE_DEBUG purpose) objects handlers to only be allowed to return a NULL pointer when an exception has been thrown, otherwise an empty HashTable must be returned, this can be done by returning (HashTable*)&zend_empty_array; (and set *is_temp = false for the get_debug_info handler).

It is possible for extensions to have cross-version compatible code by returning an empty array instead of NULL already.

This also "fixes" the issue of dumping an object when the handler throws an exception, as can be seen by change in the expectation of the ext/ffi/tests/035.phpt test.

Zend: convert debug handler to take a bool pointer

This commit is "just" to use more appropriate types and can be dropped or pushed to a later release.

Part of the motivation to include this with the above change is to make extension developers aware of the API change, as most compilers will warn/error about incompatible function signatures.

ext/standard/var.c: show unset properties

This commit fixes some of the inconsistencies with the number of properties indicated in var_dump() and how many are actually displayed. This is because currently uninitialized typed properties are not taken into account via the call to zend_array_count() and thus do not "count" but are displayed anyway.
See the change in Zend/tests/ctor_promotion_untyped_default.phpt for an explanation of what I mean.

However, for it to be perfectly consistent we need to show properties that have been unset(), and this what I've done.

This last commit, will likely require discussions on the Internals mailing list, but I would like to get preliminary opinions about the idea itself.

@Girgias Girgias force-pushed the debug-info-throw-type-error branch from b413ec6 to 99d6c28 Compare June 2, 2024 16:40
@Girgias Girgias marked this pull request as ready for review June 2, 2024 23:18
@iluuu1994
Copy link
Member

iluuu1994 commented Jun 3, 2024

Isn't the problem here that this can result in partial output? E.g:

class C {
    public function __debugInfo(): array {
        return 'nope';
    }
}

$c = new C();

try {
    var_dump([1, 2, $c, 4]);
} catch (Error $e) {
    echo $e->getMessage(), "\n";
}

array(4) {
[0]=>
int(1)
[1]=>
int(2)
[2]=>
[3]=>
int(4)
}
__debuginfo() must return an array

I'm not sure this is desirable.

Changing the output of var_dump for unset properties also sounds like RFC territory.

@devnexen
Copy link
Member

devnexen commented Jun 3, 2024

Zend: convert debug handler to take a bool pointer

This commit is "just" to use more appropriate types and can be dropped or pushed to a later release.

Part of the motivation to include this with the above change is to make extension developers aware of the API change, as most compilers will warn/error about incompatible function signatures.

In favor, but would prefer for 9.0 perhaps.

Copy link
Member

@derickr derickr left a comment

Choose a reason for hiding this comment

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

OK from me for the changes to ext/date (just moving int to bool).

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

Successfully merging this pull request may close these issues.

4 participants