-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
[WIP] Native reflection pass-through #82
[WIP] Native reflection pass-through #82
Conversation
squashed from: Work in progress: Passthrough cd98cc6 Work In Progress: Computer B0RKed! :-( 9d53b25 Finished majority of passthrough code. 3a8d144 Minor test generalization (to include builtin classes) d5966b9 Finished breaking out getClassesToAnalyze() into a separate d89bcd0 method. BROKEN: work in progress... added builtin classes to test. a778110 BROKEN: good progress... fbe9a31 Some rough edges to find and fix. Changes before laptop died ae9cb03 Merge branch 'master' into losborn_passThrough 6c98c4d minor fix before bed. d91682c Passes all tests again. 58d897a Still have some loose ends to tie up. Work in Progress: adding builtin functions to test set 5cc5e66 BROKEN: Work in Progress: adding builtin functions to tests 46078b2 All tests pass again: added passthrough for Functions. cee9d42 Need to: * Add ReflectionExtension code to emit proper classes. * Refactor BROKEN: Started adding ReflectionExtension tests. 8320983 Broken: Minimal progress 3a18b93 Broken: minor fixes... (from ancient spare computer) ab79fda Merge branch 'master' into losborn_passThrough 673dd9a Tests pass. d7e98cd Still need to implement ReflectionExtension specialization. Complete proxy implementation. 586c528 Still need to do some refactoring. Try to placate Scrutinizer. 6f974ef more scrutinizer fixes 6b75c57 more scrutinizer ca58eb1
47c5db8
to
0274744
Compare
(caught one *REAL* bug... yeah!)
@Bob4ever, if we go in this direction, this will probably obsolete #77. |
My schedule is still pretty unpredictable for the time being, but @lisachenko, can you give me a sense of how close this is to what you had envisioned for an API passthrough? Do you see anything particular you might want me to change? I know this is a pretty big patch, and I certainly don't expect a code review on something still in progress. I'm simply wanting your overall impressions. Thanks |
Wow! It's a huge job! Thanks! From my first quick view, it's what I planned to do previously, need to look deeper into PR. |
src/IReflection.php
Outdated
*/ | ||
namespace Go\ParserReflection; | ||
|
||
interface IReflection |
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, use PSR-2 naming: interfaces should have Interface
suffix in their names.
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.
The purpose of this interface is intended to be more than it currently appears to be. Because we don't have multiple inheritance the idea was that each Reflection class would have an IReflection...
interface, and that each of these would extend the parent of the original Reflection class. The purpose being that you could easily type-hint any parser-reflection class, with all compatible subtypes by using, for instance IReflectionFunctionAbstract
, which would be implemented by all subtypes that are parser-reflection classes. I thought a full "Interface" prefix for this purpose would become too cumbersome. Also, native Reflections already have a Reflector
interface, so, if I didn't want to go in this direction, I probably would have named this Reflector
instead. I also didn't want the compatibility headaches of renaming the interface later.
Interested to hear your feedback.
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.
...or perhaps you want me to rename this Reflector
, and add an empty derived interface (IReflection
or ReflectionInterface
or Interfaces\Reflection
or whatever you think is best) when I add all its brethren? (Kicking the van down the road so to speak.)
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.
My POV is to name it as ParserReflection\ReflectionInterface
and leave all stuff in this interface
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.
👍
Still tempted to add Go\ParserReflection\Reflector
in the future for consistency, but I see no need for that in this PR.
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.
@lisachenko, I'm finally digging into making these changes (as time is available), and a concern came to mind: It looks like some of the ReflectionType
values in PHP 7.0 are being broken out into subclasses in PHP 7.2. If this trend continues, we may see ReflectionTrait
and ReflectionInterface
as derived classes of ReflectionClass
in the not too distant future. If we wish to mirror this class heirarchy, this would cause a name collision for us. Perhaps we should change IReflection
to Reflector
which is already in PHP as an interface?
(Just a thought. In the meantime, I've already made the change you suggested. [Not pushed yet.] )
src/IReflection.php
Outdated
|
||
interface IReflection | ||
{ | ||
public function wasIncluded(); |
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.
why do we need such a method/interface?
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.
The Interface I explained above. The method was something I felt was intrinsic to the feature that parser-reflection classes offer: The ability to Reflect a construct before it's included (any built-in construct is already "included"). I believe I do use this in part of my of my passthrough implementation, but I thought knowing if a construct was already defined was intrinsic to what a parser-reflection class is.
This is also why I think adding a factory mechanism, which this patch is the first step towards, (and you indicated that you are sitting on the fence about) will be such a powerful feature. It allows seemlessly layering these application specific derived classes into the existing ecosystem of reflection classes.
I look forward to your thoughts.
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.
I like your explanation, so let's go with wasIncluded
method in the interface.
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.
👍
src/ReflectionFunction.php
Outdated
} | ||
|
||
/** | ||
* Has class been loaded by PHP. |
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.
Incorrect doc-block
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.
Can you give me an example of the proper format? What did I miss? -Thanks.
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.
This method relates to the function, so it should be related to the function and not to the class :) So, naming should be "Has function/method been loaded by PHP"
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.
That looks like a typo/copy-paste error... I've been using the name "construct" to refer to functions/classes/constants/variables, but I'm open to a better name.
src/ReflectionFunction.php
Outdated
$isUserDefined = true; | ||
if ($this->wasIncluded()) { | ||
$nativeRef = new BaseReflectionFunction($functionName); | ||
$isUserDefined = $nativeRef->isUserDefined(); |
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.
Assignment alignment missed, same applied to many places.
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.
Wasn't sure if you had a preference there. I'm happy to fix. Not always sure when applicable, but I'll see what I can do.
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.
Maybe it's my personal taste but when I'm looking at aligned code it reads more natural and simpler.
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.
I prefer it too, but we might want a rule of thumb to ad consistency. (i.e. Min number of consecutive assignments to align. How to mix with broken lines. Should they cascade into nested blocks. etc.)
For now I will only align 2 or more consecutive assignments with no intervening broken lines unless you have a more specific preference.
@@ -341,4 +399,18 @@ private function getClassMethodNode() | |||
{ | |||
return $this->functionLikeNode; | |||
} | |||
|
|||
/** | |||
* Has class been loaded by PHP. |
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.
Incorrect doc-block
} else { | ||
$this->initializeInternalReflection(); | ||
// We need the native implementation of getInterfaceNames() here. | ||
$ifaceNames = parent::getInterfaceNames(); |
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, do not cut variable names, because readability will be lower in that case, so ifaceNames
=> parentInterfaceNames
$this->initializeInternalReflection(); | ||
$methodRefs = parent::getMethods(); | ||
$this->methods = []; | ||
foreach ($methodRefs as $eachMethod) { |
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.
I'd suggest eachMethod
=> reflectionMethod
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.
👍 Sounds good. I wanted to avoid using $methodRefs
with $methodRef
, as that would be rather hard to distinguish.
if ($orig->isDefaultValueAvailable() || $orig->isOptional()) { | ||
if ($orig->isDefaultValueAvailable()) { | ||
if (method_exists($orig, 'isDefaultValueConstant') && $orig->isDefaultValueConstant()) { | ||
$constNameParts = explode('::', $orig->getDefaultValueConstantName(), 2); |
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.
This block looks very suspicious - why do you need to parse constant name by hands? It's a task for NodeExpressionResolver
to resolve such values.
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.
Instead of doing this by hand, just ask parser reflection for this file+class+method+param, it will look at existing AST and will search for definition.
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.
I'm fine with moving part of this to NodeExpressionResolver
, but it seems built to process nodes rather than strings. How would you recommend I delegate this to the parser? My gut reaction would be to give it something like:
sprintf('<?php function foo($bar = %s) {}', $orig->getDefaultValueConstantName())
then pluck out the resulting AST nodes for the default value. Is there a cleaner way?
tests/AbstractClassTestCaseBase.php
Outdated
$argKeys = array_keys($filenameArgList); | ||
$fileName = $filenameArgList[$argKeys[0]]; | ||
$resolvedFileName = stream_resolve_include_path($fileName); | ||
$fileNode = ReflectionEngine::parseFile($resolvedFileName); |
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, enable assignment alignment to improve readability
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.
I'll look for a Sublime plugin that does that... what IDE(s)/editor(s) do you use?
return array_pop($nameParts); | ||
} | ||
|
||
protected function getStringificationOf($value, $maxLen = 128) |
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.
Wow! Why do we need such a complex function? I'd vote to drop it and replace with something more general, without dark magic inside...
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.
This method is most of why this PR is still [WIP]
right now... it is there exclusively for test failure readability, so I think it adds minimal risk, but needs its own unit tests, and should be moved into a class in tests/Support
, or somewhere similar.
Opinions?
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.
Let me amend that.... I intend to use this function for future tests, not just test failure reporting, but it is currently only used for test error reporting.
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.
@lisachenko, I did some exploring and found PHPUnit
uses SebastianBergmann\Exporter\Exporter
to avoid this very mess. I see no reason I can't do similarly. I'm (slowly) working on extracting a PR from this one that refactors the test suite and will address this there.
Updated: The code I had originally based this on (at least conceptually) was from Codeception here: https://github.com/Codeception/Codeception/blob/baef1675a1f06e07d1dd63b46147616235020fdb/src/Codeception/Step.php#L94
Hi, @loren-osborn! Sorry for the delay, was on vacation. Could we proceed with this PR? I also want to ask you and @aik099 if you want to become library developer with rights to push changes into master branch. |
src/ReflectionClass.php
Outdated
$nativeRef = new BaseReflectionClass($fullClassName); | ||
$isUserDefined = $nativeRef->isUserDefined(); | ||
} | ||
if ($isUserDefined) { |
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.
Previously there wasn't any special handling for internal classes here and likely attempt to resolve them would fail. Do we have a test to cover this behavior?
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.
Looks like this code is much less clear than I realized. I will wrap this logic into a new private method isParsedNodeMissing()
. I believe the tests I added cover this new case, but I will verify this. 📝 To understand why this new logic is necessary, let me contrast the old and new behavior:
The Old Behavior
- If the parsed
Node
wasn't supplied:- Have the
ReflectionEngine
find it using the locator- If the locator can't find the class source code, throw a (hopefully fatal) exception.
- Have the
Now we need to support classes that may not have PHP source code. Keep in mind "user defined" simply means PHP source code exists. So I changed this to:
The New Behavior
- If no parsed
Node
is supplied:- The class is considered user defined unless the class is already defined in PHP.
- If the class is already defined in PHP:
- The class is user defined if native
ReflectionClass
says it is.
- The class is user defined if native
- If the class is user defined:
- Ask
ReflectionEngine
to load the PHP using the locator as before.
- Ask
This way we aren't searching for PHP source files for built in classes. Does this clarify the new logic at all?
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.
It does. I agree, that logic for detecting missing $classLikeNode
should be extracted to separate method.
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.
@aik099, I wanted to share the results of verifying all the logical cases were being covered: There was, unfortunately one case that I'm not currently testing. I need to add tests for files that haven't been included by PHP when the test is performed. (I had already been thinking how to do this properly, but I will come up with a stop-gap measure for now.) Currently we include all the Stub files, so we can compare the results to the behavior of the builtin reflection classes.
- Add tests for code not
include
d orrequire
d by PHP at test time.
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.
Added cases to the test suite for namespace Go\ParserReflection\Stub\NeverIncluded
which mirrors Go\ParserReflection\Stub
, but is never included or required by PHP.
: new static($implementName); | ||
$interfaces[$implementName] = $interface; | ||
$implementName = $implementNode->toString(); | ||
$interfaces[$implementName] = new static($implementName); |
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.
I see the parent/static
thing was removed.
- What was it for and why it's not needed anymore?
- Any tests covering changed piece?
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.
- It used to be needed, because
Go\ParserReflection\ReflectionClass
didn't previously support built in classes (or interfaces in this case), but now it does. - This new situation should already be under test, but I will verify: 📝
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.
OK.
? new parent($traitName) | ||
: new static($traitName); | ||
$traits[$traitName] = $trait; | ||
$traits[$traitName] = new static($traitName); |
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.
I see the parent/static
thing was removed.
- What was it for and why it's not needed anymore?
- Any tests covering changed piece?
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.
(See last answer)
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.
OK.
* @return ReflectionClass | ||
* The apropriate reflection object. | ||
*/ | ||
protected function createReflectionForClass($className) |
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.
Why this method was removed? If I'm not mistaken, then it's used in \Go\ParserReflection\Traits\ReflectionClassLikeTrait::getParentClass
method.
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.
Now that we can reflect builtin classes, this is no longer needed.
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.
OK.
// to behave exactly like \ReflectionExtension, but return | ||
// Go\ParserReflection\ReflectionFunction and | ||
// Go\ParserReflection\ReflectionClass where apropriate. | ||
return new ReflectionExtension($extName); |
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.
Is it tested?
To actually test it you need to know what extension are loaded into PHP and try to reflect class provided by them.
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.
I included tests for two seemingly ubiquitous extensions. I believe I setup the tests to skip if not loaded... not sure I have coverage on this particular method, but I will verify. 👍 📝
@@ -98,7 +124,7 @@ public function getTypeNode() | |||
public function ___debugInfo() | |||
{ | |||
return array( | |||
'name' => isset($this->propertyNode) ? $this->propertyNode->name : 'unknown', | |||
'name' => isset($this->propertyName) ? $this->propertyName : 'unknown', |
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.
The isset
won't get you desired result, because $this->propertyName
is now always set (thanks to your changes) from mandatory constructor argument.
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.
I have to investigate further, but you're probably right. If I can simplify this, I will. 👍
@@ -53,7 +60,7 @@ public function getClosureScopeClass() | |||
{ | |||
$this->initializeInternalReflection(); | |||
|
|||
return forward_static_call('parent::getClosureScopeClass'); | |||
return parent::getClosureScopeClass(); |
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.
Why change this?
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.
This is just blatantly wrong. This is an instance method, not a static method.
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.
Why this was working before and needs to be changed now? Maybe this was workaround for older PHP versions?
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.
Now these four calls to forward_static_call()
are a bug fix, and distinct from the rest of this PR, so I'd be happy to move this to a separate PR.
- Basically any call to
forward_static_call()
within an instance method is a bug. - Two of these dealt with static properties of a
ReflectionClass
's class, so that confusion is very understandable. - After some searching I found the commits that added these:
- @lisachenko, any recollection from these commits you can share?
- Not sure why these weren't caught before. I suspect they are not being properly tested?
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.
👍 for moving this into separate PR.
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.
One other thing, for this PR: the native implementation returns ?ReflectionClass
, so the result needs to be reinstantiated as a Go\ParserReflection\ReflectionClass
before it's returned, unless it's null
.
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.
No big deal. Good catch.
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.
Ok... doesn't appear that the old code has any bad behavior, but I moved this into PR #86.
@@ -63,33 +70,59 @@ public function getClosureThis() | |||
{ | |||
$this->initializeInternalReflection(); | |||
|
|||
return forward_static_call('parent::getClosureThis'); | |||
return parent::getClosureThis(); |
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.
Why change this?
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.
As above, it's an instance method.
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.
As above, why this was needed before?
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.
(See above... Move to new PR?)
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.
👍 for moving this into separate PR.
} | ||
|
||
public function getDocComment() | ||
{ | ||
if (!$this->functionLikeNode) { |
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.
I see lots of similar checks for internal reflection initialization. Maybe we can create a proxy classes that would use __call
method where optional initialization of internal reflection would be made.
That would greatly reduce copy/paste of such code, but I'm not sure if excessive usage of __call
method would result in performance decrease.
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.
I'm thinking similarly. I believe __call()
is pretty expensive, but I believe this can be simplified. I'll look into it further. 👍
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.
I have some thoughts on this I want to explore. No promises, but we'll see.
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.
OK.
} else { | ||
return null; | ||
if (PHP_VERSION_ID < 70000) { |
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.
This check won't help you, because we can use PHP5 to parse PHP7 files and vice-versa.
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.
If we're in this else block there is no PHP source code to analyze, and we're only querying the PHP binary and any loaded extensions.
Ok... just dashing off a quick note... I'd fallen off the radar a bit myself for a bit, and hobbling by with a Windows XP laptop currently, so somewhat underequipped to test any PHP 7 code at the the moment, except on my iMac at home. I do have a bunch more PRs I want to contribute, but had to focus on other areas of my life recently. @lisachenko, I greatly appreciate the offer to be a library contributor, and am happy to help any way I can. Prudence dictates that I should still rely on your very helpful peer reviews, if that isn't inconvienient. @aik099, thanks for the second round of review comments. I will respond as soon as I am able. Give me a day or two to get back up to speed on this PR. |
@loren-osborn , sure, take your time. Whenever you're ready just add a comment so that I can know that push/commit round was made a ready to be reviewed. |
@aik099, based on your review comments, I wanted to give some overall context for this PR that may be unclear: The goal of this PR is to bring feature pairity to Of course, we don't get this new flexibility for free, so we have lots of new special cases for most As far as testing all of this new functionality, I did add tests for these cases, in general, but I need to go back and check each of the conditions you pointed out individually, which I fully intend to do. I will use a 📝 to remind myself of tests I need to go back and verify, as well as pointing out any tests that I am already aware are missing/insufficient. These are simply overall responses to the issues I saw you raise. I'll go back now and respond to your individual points. These are the outstanding issues I'm already aware of, but I appreciate any feedback you have on these as well: Known Outstanding Issues:
|
I thought it was solving #16. Does it as well?
I understand now. Thanks. |
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.
Next time, when you plan to answer several comments please click on Start a review
button instead of Add single comment
and at the end of the process submit your review as Comment
type. This nice feature of GitHub allows to send single notification email for all comments instead of separate email for each.
src/ReflectionClass.php
Outdated
$nativeRef = new BaseReflectionClass($fullClassName); | ||
$isUserDefined = $nativeRef->isUserDefined(); | ||
} | ||
if ($isUserDefined) { |
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.
It does. I agree, that logic for detecting missing $classLikeNode
should be extracted to separate method.
: new static($implementName); | ||
$interfaces[$implementName] = $interface; | ||
$implementName = $implementNode->toString(); | ||
$interfaces[$implementName] = new static($implementName); |
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.
OK.
? new parent($traitName) | ||
: new static($traitName); | ||
$traits[$traitName] = $trait; | ||
$traits[$traitName] = new static($traitName); |
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.
OK.
* | ||
* @var string|\Closure | ||
*/ | ||
private $functionName; |
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.
OK. Make sure you've add tests for cases, where it's not initialized then. I guess tests, when it's initialized are already there.
* | ||
* @var string | ||
*/ | ||
private $methodName; |
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.
(As above:) OK. Make sure you've add tests for cases, where it's not initialized then. I guess tests, when it's initialized are already there.
@@ -345,7 +354,7 @@ public function isDefaultValueConstant() | |||
*/ | |||
public function isOptional() | |||
{ | |||
return $this->isVariadic() || ($this->isDefaultValueAvailable() && $this->haveSiblingsDefalutValues()); | |||
return $this->isVariadic() || ($this->isDefaultValueSet() && $this->haveSiblingsDefalutValues()); |
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.
Better to move this into separate PR for optional parameter without default value handling (part 1).
* | ||
* @return bool | ||
*/ | ||
protected function isDefaultValueSet() |
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.
Better to move this into separate PR for optional parameter without default value handling (part 2).
@@ -53,7 +60,7 @@ public function getClosureScopeClass() | |||
{ | |||
$this->initializeInternalReflection(); | |||
|
|||
return forward_static_call('parent::getClosureScopeClass'); | |||
return parent::getClosureScopeClass(); |
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.
Why this was working before and needs to be changed now? Maybe this was workaround for older PHP versions?
@@ -63,33 +70,59 @@ public function getClosureThis() | |||
{ | |||
$this->initializeInternalReflection(); | |||
|
|||
return forward_static_call('parent::getClosureThis'); | |||
return parent::getClosureThis(); |
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.
As above, why this was needed before?
* @return ReflectionClass | ||
* The apropriate reflection object. | ||
*/ | ||
protected function createReflectionForClass($className) |
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.
OK.
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.
I apologize for the abundance of notifications last time around. "Start a new review" sounded a bit misleading to me.
@@ -224,6 +250,10 @@ public function invokeArgs($object, array $args) | |||
*/ | |||
public function isAbstract() | |||
{ | |||
if (!$this->functionLikeNode) { |
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.
I'm somewhat concerned about this. There is already a method that does this: !$this->isUserDefined()
. We don't actually want to call this, since as I stated elsewhere, it calls into the parent class to be sure, but that is probably the method we should use. I could remove the extra (safeguard) logic from isUserDefined()
, but I'm not sure how I feel about this. Unfortunately I'm not sure if this is clear given your previous confusion. I could really use any suggestion you have to keep this repeated logic simple, clear and specific.
@@ -89,7 +89,7 @@ public function __construct( | |||
$this->parameterIndex = $parameterIndex; | |||
$this->declaringFunction = $declaringFunction; | |||
|
|||
if ($this->isDefaultValueAvailable()) { | |||
if ($this->isDefaultValueSet()) { |
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.
Here I do specifically disagree with you. First off, the ?string
is a "required nullable type". This is almost the opposite of the "optional parameter with no default value", which only appears to exist in builtin functions and methods. Secondly, we're doing the same proxying we do throughout this PR; We just accomplish it differently. Everywhere else in this PR, if we don't have source code for a function/method/class/etc. we call the method on the parent class. It would be rather dIfficult to initialize the ReflectionParameter
objects properly for methods with the API as it is, so instead, we query all the user visible properties, and build a parse node that will give have the correctly reflected properties, and build a Go\ParserReflection\ReflectionParameter
from the synthetic ParamNode
we synthesized. This works great except for optional parameters with no default value, which there is no syntax for. What we do is add an attribute to indicate this behavior, and check for it in ReflectionParameter
; which are the changes you're commenting on. Look at the synthetic ParamNode
generation code at ReflectionFunctionLikeTrait::getRefParameter()
.
@@ -165,6 +165,11 @@ public function __toString() | |||
*/ | |||
public function allowsNull() | |||
{ | |||
// Allow builtin types to override | |||
if ($this->parameterNode->getAttribute('prohibit_null', false)) { |
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.
(Disagree... see above.)
@@ -329,7 +336,9 @@ public function isCallable() | |||
*/ | |||
public function isDefaultValueAvailable() | |||
{ | |||
return isset($this->parameterNode->default); | |||
return | |||
isset($this->parameterNode->default) && |
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.
(Disagree... see above.)
@@ -345,7 +354,7 @@ public function isDefaultValueConstant() | |||
*/ | |||
public function isOptional() | |||
{ | |||
return $this->isVariadic() || ($this->isDefaultValueAvailable() && $this->haveSiblingsDefalutValues()); | |||
return $this->isVariadic() || ($this->isDefaultValueSet() && $this->haveSiblingsDefalutValues()); |
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.
(Disagree... see above.)
@@ -53,7 +60,7 @@ public function getClosureScopeClass() | |||
{ | |||
$this->initializeInternalReflection(); | |||
|
|||
return forward_static_call('parent::getClosureScopeClass'); | |||
return parent::getClosureScopeClass(); |
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.
Now these four calls to forward_static_call()
are a bug fix, and distinct from the rest of this PR, so I'd be happy to move this to a separate PR.
- Basically any call to
forward_static_call()
within an instance method is a bug. - Two of these dealt with static properties of a
ReflectionClass
's class, so that confusion is very understandable. - After some searching I found the commits that added these:
- @lisachenko, any recollection from these commits you can share?
- Not sure why these weren't caught before. I suspect they are not being properly tested?
@@ -63,33 +70,59 @@ public function getClosureThis() | |||
{ | |||
$this->initializeInternalReflection(); | |||
|
|||
return forward_static_call('parent::getClosureThis'); | |||
return parent::getClosureThis(); |
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.
(See above... Move to new PR?)
* | ||
* @var string|\Closure | ||
*/ | ||
private $functionName; |
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.
👍 📝 (Note to self:) Verify test cases!!!
@@ -89,7 +89,7 @@ public function __construct( | |||
$this->parameterIndex = $parameterIndex; | |||
$this->declaringFunction = $declaringFunction; | |||
|
|||
if ($this->isDefaultValueAvailable()) { | |||
if ($this->isDefaultValueSet()) { |
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.
I did some thinking on this. I think I'm able to do the same thing with a more minor change to ReflectionParameter
. Let me take another crack at this.
Everything I said above is still valid, but I think I have a more subtle solution with the same behavior.
} | ||
|
||
public function getDocComment() | ||
{ | ||
if (!$this->functionLikeNode) { |
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.
I have some thoughts on this I want to explore. No promises, but we'll see.
@aik099, continuing the discussion from #86: Part of the reason I'm not quite so scared of it, is this is my second pass at this change, and it gave me no problems previously. I agree it's a bigger delta than I remember. One thing that I probably would have included in this PR, that I think will be it's own PR now, is explicit closure support. (I may add protection against using On the subject of a closure support PR, I think it would be useful to add two methods to
What do you think? This is only a half-formed idea at this point. |
In real reflection (e.g. created from object) that would probably point back to object to which reflected method belongs. But when you're doing it statistically you have no object to return from that method. The only thing you can return is another reflection of what you expect $this to be, but then return type won't be compatible with what built-in reflection method returns. |
No, these would be discovery methods like
The intent, is being able to inspect closures that might be returned to (or otherwise interact with) calling code. The static analysis to determine which closures may be returned would be well outside the scope of this module, so the thinking was to simply return an array of all closures defined within the scope being queried. My biggest concern about doing this, is that closures are intrinsically anonymous, so there is no logical way to organize them in a returned array (except order of occurrence, which is likely to change.)
I think I was slightly misunderstood, but mostly because I believe I was being too ambitious:
|
To summarize better not to implement any of #82 (comment), because it will be out of scope of this library and any kind of reflection library shouldn't be doing static analysis of that kind. |
@aik099, basically, I mostly agree with you, and was really sitting on the fence on most of that except I think adding |
My latest changes by no means address all raised concerns yet. I just wanted to share my progress so far. I regret that this PR is taking so long. New in these changes is testing against classes that have actually never been loaded by the PHP interpreter. Much need of refactoring all around. |
src/ReflectionFunction.php
Outdated
if ($phpExt) { | ||
$origin .= ':' . $phpExt->getName(); | ||
} | ||
} |
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.
Expected 1 space after closing brace; newline found
src/NodeVisitor/BuiltinTypeFixer.php
Outdated
throw new \InvalidArgumentException( | ||
sprintf( | ||
"Option 'supportedBuiltinTypeHints's element %s " . | ||
"isn't a valid typehint string.", |
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.
Multi-line function call not indented correctly; expected 28 spaces but found 32
src/NodeVisitor/BuiltinTypeFixer.php
Outdated
|
||
public function enterNode(Node $node) | ||
{ | ||
if ( |
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.
Expected 0 spaces after opening bracket; newline found
847696a
to
357f750
Compare
src/NodeVisitor/BuiltinTypeFixer.php
Outdated
throw new \InvalidArgumentException( | ||
sprintf( | ||
"Option 'supportedBuiltinTypeHints's element %s " . | ||
"isn't a valid typehint string.", |
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.
Multi-line function call not indented correctly; expected 28 spaces but found 32
src/NodeVisitor/BuiltinTypeFixer.php
Outdated
|
||
public function enterNode(Node $node) | ||
{ | ||
if ( |
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.
Expected 0 spaces after opening bracket; newline found
357f750
to
bf8c4f3
Compare
src/NodeVisitor/BuiltinTypeFixer.php
Outdated
throw new \InvalidArgumentException( | ||
sprintf( | ||
"Option 'supportedBuiltinTypeHints's element %s " . | ||
"isn't a valid typehint string.", |
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.
Multi-line function call not indented correctly; expected 28 spaces but found 32
src/NodeVisitor/BuiltinTypeFixer.php
Outdated
|
||
public function enterNode(Node $node) | ||
{ | ||
if ( |
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.
Expected 0 spaces after opening bracket; newline found
bf8c4f3
to
fa91cb4
Compare
src/NodeVisitor/BuiltinTypeFixer.php
Outdated
throw new \InvalidArgumentException( | ||
sprintf( | ||
"Option 'supportedBuiltinTypeHints's element %s " . | ||
"isn't a valid typehint string.", |
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.
Multi-line function call not indented correctly; expected 28 spaces but found 32
src/NodeVisitor/BuiltinTypeFixer.php
Outdated
|
||
public function enterNode(Node $node) | ||
{ | ||
if ( |
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.
Expected 0 spaces after opening bracket; newline found
fa91cb4
to
4082106
Compare
src/NodeVisitor/BuiltinTypeFixer.php
Outdated
throw new \InvalidArgumentException( | ||
sprintf( | ||
"Option 'supportedBuiltinTypeHints's element %s " . | ||
"isn't a valid typehint string.", |
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.
Multi-line function call not indented correctly; expected 28 spaces but found 32
src/NodeVisitor/BuiltinTypeFixer.php
Outdated
|
||
public function enterNode(Node $node) | ||
{ | ||
if ( |
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.
Expected 0 spaces after opening bracket; newline found
4082106
to
5ab7dc3
Compare
src/NodeVisitor/BuiltinTypeFixer.php
Outdated
throw new \InvalidArgumentException( | ||
sprintf( | ||
"Option 'supportedBuiltinTypeHints's element %s " . | ||
"isn't a valid typehint string.", |
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.
Multi-line function call not indented correctly; expected 28 spaces but found 32
src/NodeVisitor/BuiltinTypeFixer.php
Outdated
|
||
public function enterNode(Node $node) | ||
{ | ||
if ( |
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.
Expected 0 spaces after opening bracket; newline found
e0d092f
to
a4c7718
Compare
It looks like nikic/PHP-Parser#449 is more an issue of simulating different versions of the PHP parser. Different typehints were added in different PHP versions and |
The inspection completed: 13 new issues, 47 updated code elements |
*/ | ||
namespace Go\ParserReflection; | ||
|
||
interface ReflectionInterface |
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.
@lisachenko, I'm kind of knee-deep in alligators here, trying to finish and untangle this into two workable PRs, but I wanted to give you an update relevant to the Go\ParserReflection\ReflectionInterface
interface I added. I added it believing that \Reflection
was the parent class of all Reflection classes. I have since discovered (at least in PHP 7) this isn't the case. The \Reflector
interface, however is implemented by all Reflection classes (except, ironically, the purely-static \Reflection
class). So the new interface Go\ParserReflection\ReflectionInterface
should clearly be renamed either Go\ParserReflection\ReflectorInterface
or simply Go\ParserReflection\Reflector
(as \Reflector
is already an interface). I welcome your input.
I am continuing work on this PR here: loren-osborn:losborn_testDirReorgAndPassThrough which will be split into two distinct PRs.
One year later :) Guys, could we have clean PR to merge for 2.0? |
@lisachenko and team, I’m sorry for dragging my feet on this... (It has been a crazy year.) I’ll try to get something cleaned up in the next 2 weeks. (Then again, I’ve been away from this so long i’m almost starting over mentally on this, so any ideas anyone has will likely help me get moving in the right direction.) |
@loren-osborn don't stress ;) |
Closing this one as it's 2020 year ) |
(This is a reissue of #81 to hide initial cleanup noise.)
This patch allows ParsedReflections to fall back to native implementation when dealing with built-in functions that have no parsable source code. This was proposed by @lisachenko in #17.
I think this implementation is complete, but it's still a bit rough. I especially want to move the utility functions I use in my tests into their own class(es) and give them their own tests.
Go\ParserReflection\TestCaseBase::getStringificationOf()
is one such bit of nastiness that should have it's own tests.Thanks for any feedback.