-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
Added get_debug_type as new function #5143
Conversation
Will be changing the scalar types to represent their in-code versions. |
This looks to be just as easily implemented in userland code, what is the rationale for implementing this in the core (the other PR didn't really give me any clue)? The issue mentioned in regards to resources is notable, unless it returned something like: |
Just a request from Beberlei that cropped up in a discussion with Derick. I've had to implement this pattern a few times in the past week, and Beberlei mentioned it appears almost 200 times in Symfony and almost 400 times in Magento. Perhaps more significantly, it's an opportunity to change the return values on this to match what is expected from type hints e.g. int not integer, float not double. It might make more sense for just hitting up zend_get_type_by_const for everything other than IS_OBJECT. |
@KalleZ Basically this is the function you need to generate the |
@nikic that makes sense to me, +1. Side question, do we want to distinguish resource types or just flat out return them as "resource"? Given |
@KalleZ I was basically wondering on the same thing in the other PR. So I'm curious if it would be worth to reserve the EDIT: I understood it in the meanwhile: as |
@kocsismate thinking about it, I think it might be better to just leave the issue open but instead invest time in looking at changing resources into opaque objects (similar to how |
Implementation looks fine. This is going to need a (small) RFC though. Expect some bikeshedding on names... |
Thank you for opening this PR, we really miss this helper IMHO! the polyfilling code could be like that (inspired from what we do in Symfony): function get_debug_type($value): string
{
if (!is_object($value)) {
return gettype($value);
}
$class = get_class($value);
if (0 !== strpos($class, "class@anonymous\0")) {
return $class;
}
return (get_parent_class($class) ?: key(class_implements($class))).'@anonymous';
} |
Cutting Regarding use of the parent class / implemented interface, I'd only do that if we actually change the class name to look like that. |
ONLY if using the parent/interface as proposed! otherwise, we'll be unable to implement using them in userland with this function (and this would void the benefit of introducing it)
That could be nice, but it could have BC consequences (breaking the code above)... |
Why? Your code should still work just fine as long as you drop the match on the |
|
I don't think there's a lot of value in ^-- that though, it doesn't seems like important information. Basically |
Yes, it's true that we can implement get_extended_debug_type(), but we could also totally implement get_debug_type() right now. We didn't because that'd mean yet another package/dependency, with all the associated maintenance overhead. We'd better keep the current code, which is fine...
Actually it's critical information: when using anonymous classes as exception classes, the default error message is totally useless as far as the class name is concerned. Same for anonymous objects found in stack traces. This is the reason why I tend to avoid using them in portable classes, and why I implemented this parent/interface display in the tools I'm using (Symfony Debug/ErrorHandler). Not everyone uses Symfony, so it would be awesome if PHP could borrow this. |
Could it be worth adding the resource type? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice!
RETURN_NEW_STR(zend_strpprintf(0, "resource (%s)", name)); | ||
} else { | ||
RETURN_INTERNED_STR(ZSTR_KNOWN(ZEND_STR_CLOSED_RESOURCE)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't it miss "IS_UNINITIALIZED" used with typed properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They throw upon trying to read them, so could not be used in an expression.
This PR was merged into the 1.15-dev branch. Discussion ---------- [Php80] Add get_debug_type() Waiting for - [ ] php/php-src#5143 - [ ] https://wiki.php.net/rfc/get_debug_type - [x] php/php-src#5153 Commits ------- 61168ea [Php80] Add get_debug_type()
This PR was merged into the 1.15-dev branch. Discussion ---------- [Php80] Add get_debug_type() Waiting for - [ ] php/php-src#5143 - [ ] https://wiki.php.net/rfc/get_debug_type - [x] php/php-src#5153 Commits ------- 61168ea [Php80] Add get_debug_type()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an UPGRADING note and squash when merging.
Rebased and merged as ef0e447. |
Alternative to #5142 exposing a new function get_debug_type which will return the class name if the type is an object.
RFC: https://wiki.php.net/rfc/get_debug_type