-
Notifications
You must be signed in to change notification settings - Fork 7.8k
"::class resolution as scalar" Feature #187
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
"::class resolution as scalar" Feature #187
Conversation
* Allows for Name::class, self::class, static::class, parent::class resolution to a scalar based on current use rules and current namespace. * Reuses existing keyword "class" * Alters zend_compile.c\zend_resolve_class_name() to facilitate compile time class name resolution (and runtime by way of FCALL)
* Added class scope checking for self,parent+static * Fixed tab/spaces in phpt file
just a quick test with your patch, got a memleak: <?php
class b {
public function dummy() {
echo self::class;
}
} lead to a memleak:
|
I've located the leak and will work the fix out, thanks- (I forgot to compile with --enable-debug). |
* Fixed memory leak by adding zval_dtor() on class_name in zend_compile.c's zend_resolve_class_name()
@@ -0,0 +1,56 @@ | |||
--TEST-- | |||
class name as scaler from ::class keyword |
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.
Nitpicking: scaler
-> scalar
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.
Indeed. Thanks for pointing this out.
I am aware that this implementation needs more work (there are additional edge cases). But I wanted to bring it to a vote to ensure that additional work on this feature would not ultimately be all-for-naught, as outside of core/master inclusion, this feature is effectively useless.
I am working on a few more tests and some remaining issues ( |
@ralphschindler would love to get this merged, as the RFC ended positively. Could you drop me a message to discuss the remaining work? |
I need to rework the patch as per the conversations that were had on the internals list. My plan is to do it during the holidays this week (American Thanksgiving), so I should be able to get this updated by end of week. In its current state, it's not good for merge. |
@ralphschindler, cool, thanks! If you need help with anything, drop me a note. |
@ralphschindler any news? |
@ralphschindler ping |
@ralphschindler ping again. |
@ralphschindler By the way, if you want this in 5.5 you need to hurry. We're close to feature freeze ;) |
@nikic I have it mostly working, in two different forms, any chance you could help me bring it to completion? |
@ralphschindler Depends on how much help you need, but in principle yes :) What are you still missing / what issues are you having with the implementation? |
@nikic I will push what I have to my github account, and give you a summary of the decisions I need to make. Mostly, I need to decide how to handle situations like this: class Foo {
public function bar(static::class $foo, parent::class $bar, self::class $baz) {}
} I believe I had self::class working in that scenario, but the other two were not with the current implementation of the FETCH_CLASS_CONST. |
@ralphschindler Assuming you mean Parameter initializers (and all other initializers) in PHP only accept compile-time scalars. As |
I agree with @nikic here. If parent and static throw an error, that’s more than fine. |
Since I decided to go in a different direction with the implementation and wanted to keep a clean commit history, I re-branched from master and applied my fixes there and will open up a new PR. see #256 |
@ralphschindler: You don't need a new pull request, just force-push to this branch, the history will update here. |
FOR RFC: https://wiki.php.net/rfc/class_name_scalars
Patch addresses: