Scrutinizer fixes #76

loren-osborn · 2017-07-23T07:58:13Z

This PR addresses most of the issues highlighted by the Scrutinizer code analysis tool this package is already coupled to.

Nitpick-CI · 2017-07-23T07:58:22Z

src/ReflectionMethod.php

@@ -81,6 +81,7 @@ public function ___debugInfo()
    public function __toString()
    {
        $hasReturnType    = $this->hasReturnType();
+        $returnType       = $hasReturnType ? $this->getReturnType() : NULL;


TRUE, FALSE and NULL must be lowercase; expected null but found NULL

Nitpick-CI · 2017-07-23T07:58:22Z

src/ValueResolver/NodeExpressionResolver.php

+        $classToReflect = $node->class;
+        if ($classToReflect instanceof Expr) {
+            $refClass = $this->resolve($classToReflect);
+        }


Expected 1 space after closing brace; newline found

Nitpick-CI · 2017-07-23T07:58:23Z

src/bootstrap.php

-    class ReflectionType {}
-}
+
+require (__DIR__ . '/polyfill.php');


Space before opening parenthesis of function call prohibited

Nitpick-CI · 2017-07-23T07:58:23Z

src/polyfill.php

+ * versions of PHP, (i.e. PHP < 7).
+ */
+if (!class_exists(ReflectionType::class, false)) {
+    class ReflectionType {


Each class must be in a namespace of at least one level (a top-level vendor name)

Opening brace of a class must be on the line after the definition

Nitpick-CI · 2017-07-23T07:58:23Z

src/polyfill.php

+    class ReflectionType {
+        public function allowsNull()
+        {
+            return TRUE;


TRUE, FALSE and NULL must be lowercase; expected true but found TRUE

aik099 · 2017-07-23T08:02:08Z

Isn't that too much? I mean changes made are protection code added for theoretical misuse cases of the library that never happen in reality. Placing protection code only suggests that bad thing are expected to happen.

Nitpick-CI · 2017-07-23T08:08:08Z

src/polyfill.php

+ * versions of PHP, (i.e. PHP < 7).
+ */
+if (!class_exists(ReflectionType::class, false)) {
+    class ReflectionType


Each class must be in a namespace of at least one level (a top-level vendor name)

This is obviously a non-issue for a polyfill class.

loren-osborn · 2017-07-23T08:16:02Z

@aik099, My intent was really just good code hygiene. I certainly didn't want to impact performance. For several of the string argument checking, perhaps a docblock annotation is a better resolution?

These are the results of Scrutinizer's updated report: https://scrutinizer-ci.com/g/goaop/parser-reflection/inspections/eaf283f6-f982-44a4-bb28-51489bcba537/issues/

Update: I checked the code, and the string arguments that are being type verified are already properly annotated as strings. :-(

Update2: I noticed that this reduced code coverage by about 0.8%. I need to add some tests to compensate. I'll have to address this in the morning.

loren-osborn · 2017-07-24T19:47:05Z

Ok... Update3: I'm working on improving test coverage, but it's a bit more drastic than I hoped, so I have decided to put it in another PR that I will submit later.

@lisachenko , @aik099 can you give this a look, and let me know what you think, as it stands right now? @aik099 pointed out that there is some code that somewhat dubiously bullet proofs agains (hopefully) impossible cases, but the goal was to increase the signal-to-noise ratio in the Scrutinizer report, so it's easier to spot real issues when they arise. Additionally, someone with project write-access should also disable the issues that were intentional violations. (For instance, the ReflectionType polyfill class was intentionally put in the Global namespace.) If any of the changes I proposed causes a performance issue, that is a real concern I want to address.

None of this is anything earth-shattering. It is intended to help keep the project clean and contributor-friendly. I welcome hearing any reason these changes should not be merged in. My intent was merely to help out.

An aside: I mentioned several improvements I've made to this project (and Codeception/AspectMock) for a former employer that I wanted to share. I am no longer with this company, and they have declined any interest in contributing these in the foreseeable future, despite my urging. They did mention they may be willing to release these for a fee. No amount was discussed, but I suggested a hypothetical future employer of mine might want to pay "a few grand" (that's a few $1000 USD to non-native speakers) for such changes, that could save them several months of my salary to reproduce such features. I have no idea how receptive to this idea this company still is, but if any of you want to raise some funds for this, I can help you get in touch with those in a position to discuss releasing those changes.

Anyway, I hope you find this PR helpful.

aik099 · 2017-07-25T08:39:02Z

An aside: I mentioned several improvements I've made to this project (and Codeception/AspectMock) for a former employer that I wanted to share. I am no longer with this company, and they have declined any interest in contributing these in the foreseeable future, despite my urging. They did mention they may be willing to release these for a fee. No amount was discussed, but I suggested a hypothetical future employer of mine might want to pay "a few grand" (that's a few $1000 USD to non-native speakers) for such changes, that could save them several months of my salary to reproduce such features. I have no idea how receptive to this idea this company still is, but if any of you want to raise some funds for this, I can help you get in touch with those in a position to discuss releasing those changes.

@loren-osborn , can you list the changes included in there, so that we'll know what's coming?

aik099 · 2017-07-25T08:42:02Z

... @aik099 pointed out that there is some code that somewhat dubiously bullet proofs agains (hopefully) impossible cases, but the goal was to increase the signal-to-noise ratio in the Scrutinizer report, so it's easier to spot real issues when they arise. Additionally, someone with project write-access should also disable the issues that were intentional violations. (For instance, the ReflectionType polyfill class was intentionally put in the Global namespace.) If any of the changes I proposed causes a performance issue, that is a real concern I want to address.

Getting higher score on Scrutinizer CI won't be something worth reaching 😉 It's known for some weird rating calculating logic where some good classes are getting small ratings and attempt to fix rating results in making classes worse.

In my projects I'm reviewing each of the errors and decide if that is even worth fixing or I can simply ignore that. Not sure if ignoring errors would improve my rating though. 😄

aik099 · 2017-07-25T08:44:48Z

By the way I wonder why Scrutinizer CI isn't posting it's build status for this PR. Maybe this feature needs to be enabled manually.

aik099 · 2017-07-25T08:49:20Z

src/bootstrap.php

-    class ReflectionType {}
-}
+
+require(__DIR__ . '/polyfill.php');


It's a fix for https://scrutinizer-ci.com/g/goaop/parser-reflection/inspections/eaf283f6-f982-44a4-bb28-51489bcba537/issues/files/src/bootstrap.php?status=fixed&orderField=path&order=asc&honorSelectedPaths=0 , but I'm not sure it's worth it. Better to reduce file operation count (the require in this case).

Hmm... I think this is a case of minor improvement in PSR-1 compliance for 1 extra small file load once, during execution. I think it's a worthwhile cost... and I think having Polyfills in a file called polyfill.php helps people know where to find them.

aik099 · 2017-07-25T08:56:02Z

src/ReflectionMethod.php

@@ -81,6 +81,7 @@ public function ___debugInfo()
    public function __toString()
    {
        $hasReturnType    = $this->hasReturnType();
+        $returnType       = $hasReturnType ? $this->getReturnType() : null;


The $hasReturnType ? $this->getReturnType() : null; can be replaced with just $this->getReturnType();, because internally hasReturnType method is calling getReturnType and checking result via isset.

Good catch 👍

aik099 · 2017-07-25T08:57:55Z

src/ReflectionMethod.php

@@ -114,7 +115,7 @@ public function __toString()
            $this->getEndLine(),
            count($methodParameters),
            $paramString,
-            $hasReturnType ? ReflectionType::convertToDisplayType($this->getReturnType()) : ''
+            $returnType ? ReflectionType::convertToDisplayType($returnType) : ''


👍 , but also add tests (or post a link in comment if test exists already), that would verify atleast 2 cases:

the return type was null and wasn't present in output

the return type was ReflectionType object and it's string representation was shown in output

Ok... Good point on adding tests. As I said, needing to untangle a branch first, but: Note to self... Implement/find relevant test cases. 👍

Ok... I confirmed that both cases are tested... (I did not flag the specific tests, but they're easy to find.) When $returnType is null the empty string value isn't used, but it prevents the convertToDisplayType() static method call that would be illegal with a null parameter. I am replacing the empty string with a string explaining its purpose.

aik099 · 2017-07-25T09:01:02Z

src/ReflectionType.php

@@ -85,14 +85,14 @@ public static function convertToDisplayType(\ReflectionType $type)
            'int'  => 'integer',
            'bool' => 'boolean'
        ];
-        $displayType = $type->type;
+        $displayType = (string)$type;


👍 , but a test is needed (or post link to existing test), that would make a call to convertToDisplayType method with:

native ReflectionType class object

with library's own ReflectionType object, that has type property

As above... add tests 👍

aik099 · 2017-07-25T09:03:20Z

src/ReflectionType.php

        if (isset($typeMap[$displayType])) {
            $displayType = $typeMap[$displayType];
        }

        $displayType = ltrim($displayType, '\\');

-        if ($type->allowsNull()) {
+        if (method_exists($type, 'allowsNull') && $type->allowsNull()) {


Please return the original code, because Scrutinizer CI issue in https://scrutinizer-ci.com/g/goaop/parser-reflection/inspections/eaf283f6-f982-44a4-bb28-51489bcba537/issues/files/src/ReflectionType.php?status=fixed&orderField=path&order=asc&honorSelectedPaths=0 likely is reported because they're using incorrect PHP stubs for class method signature resolution. On the PHP docs (see http://php.net/manual/en/reflectiontype.allowsnull.php) there is allowsNull method and it always was there (as in out ReflectionType class polyfill).

Hmm... I agree that I should revert this code, but for a different reason. I think this is catching the class definition from the polyfill, which had no $this->allowsNull() method. I actually added a dummy $this->allowsNull() method to my polly fill, so this should no longer be an issue. I don't particularly like the dummy method in the polyfill, but I think it's the best solution currently. Either way, this code should no longer be necessary. Thanks for pointing this out. 👍

I think this is catching the class definition from the polyfill, which had no $this->allowsNull() method.

Then I think you need to add stubs in the polyfill class for all 3 methods: http://php.net/manual/en/class.reflectiontype.php

Fair enough! :-) 👍

aik099 · 2017-07-25T09:18:10Z

src/ValueResolver/NodeExpressionResolver.php

@@ -175,12 +175,12 @@ protected function resolveScalarMagicConstNamespace()
    protected function resolveScalarMagicConstClass()
    {
        if ($this->context instanceof \ReflectionClass) {
-            return $this->context->getName();
+            return $this->context->name;


Looks good.

aik099 · 2017-07-25T09:18:15Z

src/ValueResolver/NodeExpressionResolver.php

        }
        if (method_exists($this->context, 'getDeclaringClass')) {
            $declaringClass = $this->context->getDeclaringClass();
            if ($declaringClass instanceof \ReflectionClass) {
-                return $declaringClass->getName();
+                return $declaringClass->name;


Looks good.

aik099 · 2017-07-25T09:18:20Z

src/ValueResolver/NodeExpressionResolver.php

@@ -213,7 +213,7 @@ protected function resolveScalarMagicConstLine(MagicConst\Line $node)
    protected function resolveScalarMagicConstTrait()
    {
        if ($this->context instanceof \ReflectionClass && $this->context->isTrait()) {
-            return $this->context->getName();
+            return $this->context->name;


Looks good.

aik099 · 2017-07-25T09:19:58Z

src/ValueResolver/NodeExpressionResolver.php

        $isFQNConstant = $node->name instanceof Node\Name\FullyQualified;
        $constantName  = $node->name->toString();

        if (!$isFQNConstant) {
            if (method_exists($this->context, 'getFileName')) {
                $fileName      = $this->context->getFileName();
                $namespaceName = $this->resolveScalarMagicConstNamespace();
+                /** @var ReflectionFileNamespace $fileNamespace */


You can safely remove this, because IDE can detect type from new ... construct by itself 😉

Agreed... This comment just "came along for the ride" when I moved the variable's initialization.

aik099 · 2017-07-25T09:20:37Z

src/ValueResolver/NodeExpressionResolver.php

@@ -256,7 +255,12 @@ protected function resolveExprConstFetch(Expr\ConstFetch $node)

    protected function resolveExprClassConstFetch(Expr\ClassConstFetch $node)
    {
-        $refClass     = $this->fetchReflectionClass($node->class);
+        $classToReflect = $node->class;


Doesn't seem to be related to Scrutinizer CI reported problems. Maybe belongs to separate PR with a description.

I'm not sure why this issue isn't showing up for you, nor why I can't find links to individual Scrutinizer issues, but this is to address the Scrutinizer issue reported for this line that reads:

Bug introduced 11 months ago by @lisachenko
It seems like $node->class can also be of type object<PhpParser\Node\Expr>; however, Go\ParserReflection\Valu...:fetchReflectionClass() does only seem to accept object<PhpParser\Node\Name>, maybe add an additional type check?

Interesting. So class constant can be an expression. Seem to addition in PHP syntax, that PHP-Parser now supports. In either cases a test for that is needed too.

Yes, I know you can do a constant fetch from a class name stored in a variable. I'm unsure what other Exprs are valid. I believe ParserReflection uses this mostly for evaluating constants, so this might not be a valid code path as we are using it, but it is a valid value for $node->class in some circumstances.

As far as adding corresponding test cases, I agree, but I was lumping my test changes into a separate PR. (I'm shooting for 100% code coverage, but I've always been a cockeyed optimist.) I can add these tests to this PR after I've untangled my current repo branches. (I don't really want to do a stash right now.)

Yes, I know you can do a constant fetch from a class name stored in a variable. I'm unsure what other Exprs are valid.

I've meant Example #4 Constant expression example from http://php.net/manual/en/language.oop5.constants.php page:

const ONE = 1; class foo { // As of PHP 5.6.0 const TWO = ONE * 2; const THREE = ONE + self::TWO; const SENTENCE = 'The value of THREE is '.self::THREE; }

This way you can create test for that kind of syntax.

As far as adding corresponding test cases, I agree, but I was lumping my test changes into a separate PR.

The tests for changed code needs to come as part of PR, where that code is changed. Later you can send another PR for improving code coverage.

I agree we need to test for this case, but it isn't demonstrated in the example above. It would be something like this, which I'm doubting is valid PHP:

const LEFT = 'Foo'; const RIGHT = 'Bar'; class FooBar { const FILENAME = 'baz.php'; } require((LEFT . RIGHT)::FILENAME);

Like I said... I'm unsure if this is valid PHP, but I think, given that PhpParser accepts an expression, and we need something that can evaluate at compile time, something like this would be a valid test, even if not legal PHP currently.

Opinions?

Thumbing up to remind myself to add test cases. 👍

Anything that would force PHP-Parser to create Expr class would do. Code won't be ever evaluated by PHP itself and only needs to be syntactically valid.

Just for reference: Variation of above sample code on 3v4l.org

aik099 · 2017-07-25T09:21:23Z

@loren-osborn , review completed with outcome: don't trust Scrutinizer CI proposed fixes blindly 😉

loren-osborn · 2017-07-25T20:47:14Z

src/NodeVisitor/RootNamespaceNormalizer.php

+     *
+     * @param Node[] $nodes Array of nodes
+     *
+     * @return null|Node[] Array of nodes


This is really a failed attempt to fix a false-positive in Scrutinizer. I merely copied the original function docblock from PhpParser. The issue is that Scrutinizer is letting the less specific type hint array take precedence over the compatible and more specific @param Node[], and saying that array does not satisfy the specification @return null|Node[]. I expect this change should actually be reverted, but I didn't want to do so without feedback.

As far as the false-positive, I'm trying to notify Scrutinizer about it, but haven't gotten any confirmation yet.

Thumbing myself up purely so I remember to revert this change: 👍

loren-osborn · 2017-07-25T21:58:58Z

@aik099, I'm sorry that I'm attacking this code review a bit piecemeal. I will push a new commit when I'm finished... In the meantime:

By the way I wonder why Scrutinizer CI isn't posting it's build status for this PR. Maybe this feature needs to be enabled manually.

I actually posted a link to this above. Please let me know if this isn't valid: Scrutinizer-CI issue report for PR #76

aik099 · 2017-07-26T07:10:14Z

I actually posted a link to this above. Please let me know if this isn't valid: Scrutinizer-CI issue report for PR #76

Perfectly fine. I was thinking about proper integration with GitHub where PR build status would fail if issues are found by Scrutinizer CI.

lisachenko · 2017-07-26T11:34:22Z

Perfectly fine. I was thinking about proper integration with GitHub where PR build status would fail if issues are found by Scrutinizer CI.

Hi!
Just enabled integration of Scrutinizer-ci for PRs (previously it was enabled for commit statuses)

lisachenko · 2017-07-26T11:35:26Z

So, guys, is this ready for merge?

loren-osborn · 2017-07-26T11:43:51Z

@lisachenko, thanks, but not ready to merge yet... I've only gone through two-thirds of @aik099's code review. I still need to make suggested changes to PR.

When the PR is would you like these squashed into a single commit?

lisachenko · 2017-07-26T11:54:24Z

When the PR is would you like these squashed into a single commit?

This (squash) will be done automatically during the merge, no need to squash it before.

aik099 · 2017-07-26T15:03:28Z

So, guys, is this ready for merge?

Your response is needed about inlining ReflectionClassLikeTrait.php (see comment about that, I don't know how to link into code comment directly).

loren-osborn · 2017-07-26T15:15:12Z

Your response is needed about inlining ReflectionClassLikeTrait.php (see comment about that, I don't know how to link into code comment directly).

@aik099, @lisachenko, I'm not opposed to inlining the whole trait, if that's the direction we deem appropriate, but I was only suggesting adding a single protected method to Go\ParserReflection\ReflectionClass named something like createReflectionClassFor($className).

aik099 · 2017-07-26T15:18:24Z

@loren-osborn , you seem to have closed this PR by mistake. This usually happens when you rename a branch and push changes.

lisachenko · 2017-07-26T15:21:36Z

Gyus! ) Stop blaming PR and let's move to the merge part, it's impossible to do everything just in one single PR.

@aik099 I think we should review only critical parts, all imrovements in docs, tests and code style should be accepted.

Nitpick-CI · 2017-08-02T00:01:04Z

src/ReflectionFileNamespace.php

+        if (!is_string($fileName)) {
+            throw new \InvalidArgumentException(sprintf(
+                '$fileName must be a string, but a %s was passed',
+                gettype($fileName)));


Multi-line function call not indented correctly; expected 12 spaces but found 16

Closing parenthesis of a multi-line function call must be on a line by itself

Nitpick-CI · 2017-08-02T00:01:05Z

src/ReflectionMethod.php

-            join(' ', \Reflection::getModifierNames($this->getModifiers() & 1792)),
+            join(' ', \Reflection::getModifierNames(
+                $this->getModifiers() &
+                (self::IS_PUBLIC | self::IS_PROTECTED | self::IS_PRIVATE))),


Multi-line function call not indented correctly; expected 12 spaces but found 16

Closing parenthesis of a multi-line function call must be on a line by itself

Nitpick-CI · 2017-08-02T00:01:05Z

src/polyfill.php

+ */
+if (!class_exists(ReflectionType::class, false)) {
+    /* Dummy polyfill class */
+    class ReflectionType


Each class must be in a namespace of at least one level (a top-level vendor name)

Nitpick-CI · 2017-08-02T00:14:30Z

src/ReflectionMethod.php

+                ' ',
+                \Reflection::getModifierNames(
+                    $this->getModifiers() &
+                        (self::IS_PUBLIC | self::IS_PROTECTED | self::IS_PRIVATE)


Multi-line function call not indented correctly; expected 20 spaces but found 24

Nitpick-CI · 2017-08-02T00:14:30Z

src/polyfill.php

+ */
+if (!class_exists(ReflectionType::class, false)) {
+    /* Dummy polyfill class */
+    class ReflectionType


Each class must be in a namespace of at least one level (a top-level vendor name)

loren-osborn · 2017-08-02T00:34:34Z

@aik099, @lisachenko, I hope this is closer to what you were hoping for:

I left the filename string argument type check in place because scrutinizer had been complaining about them, but added tests for these cases. (Like I said, even though @aik099 didn't notice these until later, they were in the PR from the beginning.) If you both agree I should back these out, I'm perfectly willing to.

I also added two more test files, as the cases I was trying to cover didn't seem to fit any of the existing files.

I believe the PR is ready for a second review. @lisachenko, can you weigh in on the filename type checking?

Thanks to both of you.

aik099 · 2017-08-02T06:30:56Z

src/ReflectionMethod.php

-            $hasReturnType ? ReflectionType::convertToDisplayType($this->getReturnType()) : ''
+            (   $returnType ?
+                ReflectionType::convertToDisplayType($returnType) :
+                'UNUSED: Prevents convertToDisplayType() being called.')


😄 , what is that? Are you sure code is never executed under normal conditions?

Ok, I guess my "not quite a comment" explanatory string was unclear. When $returnType is non-null, this is definitely used. When $returnType is null, however, the '%s' substring that includes this in sprintf()'s return value, isn't included in the format string. If you can think of a clearer way to say this, I welcome suggestions.

If UNUSED: Prevents convertToDisplayType() being called. text isn't actually displayed anywhere, then it doesn't make sense to write it.

If it's displayed, but same text isn't displayed in built-in reflection api, then this won't be compatible.

It is indeed not displayed. I can easily change it back to the empty string. I'm trying to decide if I need a comment there to replace it.

I admit that from a clarity standpoint, I don't like the way this function reads, but I think anything but an empty string probably just muddies the water. @aik099, what do you think?

aik099 · 2017-08-02T06:31:55Z

src/ReflectionMethod.php

+                ' ',
+                \Reflection::getModifierNames(
+                    $this->getModifiers() &
+                        (self::IS_PUBLIC | self::IS_PROTECTED | self::IS_PRIVATE)


I guess this more expressive form of same thing? I wonder why you've added this in this PR?

Yeh... probably less relevant to this PR, but I generally hate non-expressive "magic numbers", and I can across this one while working on this PR. It probably doesn't belong, but I felt like it made the code clearer.

OK, let's keep it then.

aik099 · 2017-08-02T06:33:08Z

src/ReflectionMethod.php

@@ -80,7 +80,8 @@ public function ___debugInfo()
     */
    public function __toString()
    {
-        $hasReturnType    = $this->hasReturnType();
+        $returnType       = $this->getReturnType();
+        $hasReturnType    = ($returnType !== null); // Internally same as: $this->hasReturnType();


Not sure, that comment is needed.

It came up in our discussion, and it wasn't obvious to me, so I added the comment.

I thought that you will change

$hasReturnType = ($returnType !== null); // Internally same as: $this->hasReturnType();

into

$hasReturnType = $returnType !== null;

but you haven't.

Adding the parentheses was inadvertent and I'll remove them, but I do think the comment is helpful. It's an explanation why we aren't calling hasReturnType(). @lisachenko seems to be deferring to you on this PR, so it's your call, but I think the comment has value.

if you disagree, I'll remove it; or if you prefer, I can put the comment on it's own line.

Update: To expedite things, I'll give you some unambiguous options ordered by my personal preference:

Put the comment on its own line:

// Internally $this->getReturnType() !== null same as $this->hasReturnType() $returnType = $this->getReturnType(); $hasReturnType = $returnType !== null;

Just remove the parenthesis:

$returnType = $this->getReturnType(); $hasReturnType = $returnType !== null; // Internally same as: $this->hasReturnType();

Strip the comment:

$returnType = $this->getReturnType(); $hasReturnType = $returnType !== null;

aik099 · 2017-08-02T07:08:11Z

tests/ReflectionTypeTest.php

+    public function testTypeConvertToDisplayTypeWithNullableNativeType()
+    {
+        if (PHP_VERSION_ID < 70100) {
+            $this->markTestIncomplete('PHP >= 7.1 implements Nullable types');


If PHP 7.1+ implements nullable types, then test must be marked as skipped on other PHP versions, not an incomplete one.

Agreed. My mistake. 👍

PHP 7.1+ implements explicit nullable types. I was considering including a 7.0+ test for implicit nullable types. It doesn't test a distinct code path, at least not in the code I was operating on here. Do you think I should add this test?

(By "implicit nullable types", I'm referring to parameters with class name type hints that default to null. I believe PHP 7.0+ is supposed to report those as isNullable(). I will double check this.)

aik099 · 2017-08-02T07:08:44Z

tests/ReflectionTypeTest.php

+    public function testTypeConvertToDisplayTypeWithNativeType()
+    {
+        if (PHP_VERSION_ID < 70000) {
+            $this->markTestIncomplete('PHP >= 7.0 implements ReflectionType');


If PHP 7.0+ implements ReflectionType, then test must be marked as skipped on other PHP versions, not an incomplete one.

Noted. I'll fix this. 👍

loren-osborn · 2017-08-02T20:07:37Z

Just so you're aware, my laptop (which is my primary computer) is on the fritz so this may delay additional commits right now.

Nitpick-CI · 2017-08-06T07:50:08Z

src/ReflectionMethod.php

+                ' ',
+                \Reflection::getModifierNames(
+                    $this->getModifiers() &
+                        (self::IS_PUBLIC | self::IS_PROTECTED | self::IS_PRIVATE)


Multi-line function call not indented correctly; expected 20 spaces but found 24

Any way so solve this CS issue?

If I read the issue correctly, @Nitpick-CI wants $this->getModifiers() intended the same as (self::IS_PUBLIC which I was concerned may be easily mistaken as a second argument to getModifierNames(). Despite not liking overly long lines, I think getting rid of the line break is the best option.

As above, unambiguous options in my personal order of preference:

Remove the newline:

join( ' ', \Reflection::getModifierNames( $this->getModifiers() & (self::IS_PUBLIC | self::IS_PROTECTED | self::IS_PRIVATE) ) ),

Square up the two lines:

join( ' ', \Reflection::getModifierNames( $this->getModifiers() & (self::IS_PUBLIC | self::IS_PROTECTED | self::IS_PRIVATE) ) ),

Leave them as-is:

join( ' ', \Reflection::getModifierNames( $this->getModifiers() & (self::IS_PUBLIC | self::IS_PROTECTED | self::IS_PRIVATE) ) ),

Nitpick-CI · 2017-08-06T07:50:08Z

src/polyfill.php

+ */
+if (!class_exists(ReflectionType::class, false)) {
+    /* Dummy polyfill class */
+    class ReflectionType


Each class must be in a namespace of at least one level (a top-level vendor name)

loren-osborn · 2017-08-06T07:53:06Z

I changed the explicit compatibility checks to PHPUnit @requires annotations. I believe this is the last of the open issues.

@aik099, @lisachenko, Any other changes you want me to make before merging this in?

aik099

Hopefully this is last review on this long running PR.

aik099 · 2017-08-06T18:49:40Z

src/ReflectionFileNamespace.php

@@ -83,6 +83,9 @@ class ReflectionFileNamespace
     */
    public function __construct($fileName, $namespaceName, Namespace_ $namespaceNode = null)
    {
+        if (!is_string($fileName)) {


What is the 3rd place, can you post a link to that code line? If total call count in that 3rd place is much smaller than sum of other 2 places, then from performance point it would be better to type cast before calling this method instead of checking in constructor.

Same is valid for other similar place.

aik099 · 2017-08-06T18:50:27Z

src/ReflectionMethod.php

@@ -80,7 +80,8 @@ public function ___debugInfo()
     */
    public function __toString()
    {
-        $hasReturnType    = $this->hasReturnType();
+        $returnType       = $this->getReturnType();
+        $hasReturnType    = ($returnType !== null); // Internally same as: $this->hasReturnType();


I thought that you will change

$hasReturnType = ($returnType !== null); // Internally same as: $this->hasReturnType();

into

$hasReturnType = $returnType !== null;

but you haven't.

aik099 · 2017-08-06T18:50:48Z

src/ReflectionMethod.php

+                ' ',
+                \Reflection::getModifierNames(
+                    $this->getModifiers() &
+                        (self::IS_PUBLIC | self::IS_PROTECTED | self::IS_PRIVATE)


OK, let's keep it then.

aik099 · 2017-08-06T18:51:11Z

src/ReflectionMethod.php

+                ' ',
+                \Reflection::getModifierNames(
+                    $this->getModifiers() &
+                        (self::IS_PUBLIC | self::IS_PROTECTED | self::IS_PRIVATE)


Any way so solve this CS issue?

aik099 · 2017-08-06T18:51:55Z

src/ReflectionMethod.php

            $this->getName(),
            $this->getFileName(),
            $this->getStartLine(),
            $this->getEndLine(),
            count($methodParameters),
            $paramString,
-            $hasReturnType ? ReflectionType::convertToDisplayType($this->getReturnType()) : ''
+            ($returnType ? ReflectionType::convertToDisplayType($returnType) : '')


Parenthesis around construct aren't required.

That's fine... you want me to ditch the parenthesis? Can do...

Yes, please.

aik099 · 2017-08-06T18:52:42Z

src/ReflectionType.php

@@ -85,7 +85,7 @@ public static function convertToDisplayType(\ReflectionType $type)
            'int'  => 'integer',
            'bool' => 'boolean'
        ];
-        $displayType = $type->type;
+        $displayType = (string) $type;


Are you sure, that according to coding standard there should be space between type name and variable?

I don't like the way it looks either... it's what Scrutinizer was suggesting. I'm happy to loose the ugly space. It's your call.

Please locate other type cast operators in the code (not added by you) and see which style is used most. Then use most used style.

loren-osborn · 2017-08-07T04:59:43Z

@aik099, Thanks for the review. I'm ready to accept whatever changes you think are best. Just give me your final decisions on the following few alternatives: (I'm including my understanding of your current perference in parenthases.)

ReflectionFile and ReflectionFileNamespace constructors: (Option 2: Remove the is_string() checks.)
ReflectionMethod $hasReturnType comment: (Option 3: Strip the comment.)
ReflectionMethod visibility bit mask: (Option 1: Remove the linebreak.)
ReflectionType type cast whitespace: (Remove whitespace.)

While I hope I've been able to change your mind about the is_string() checks, I'll take your decisions as final.

Thanks again for all the time and attention to detail.

aik099 · 2017-08-07T12:59:15Z

ReflectionFile and ReflectionFileNamespace constructors: (Option 2: Remove the is_string() checks.)

Nope. Please see #76 (comment) for actual idea.

ReflectionMethod $hasReturnType comment: (Option 3: Strip the comment.)

Actually keeping. After pondering on this for a while I think comment here will help others to understand code better.

ReflectionMethod visibility bit mask: (Option 1: Remove the linebreak.)

Whatever solves CS issue. I like the change in general.

ReflectionType type cast whitespace: (Remove whitespace.)

Please locate other type cast operators in the code (not added by you) and see which style is used most. Then use most used style.

I hope my responses would allow you to make final changes to the PR.

loren-osborn · 2017-08-07T16:18:45Z

@aik099, I'm rather confused. I'm happy to add explicit type casts to the three places ReflectionFileNamespace is instantiated if you like, but I'm fairly confident this won't address the issue. Please see (#76: comment) my rather involved response to your comment.

Thanks

Would you believe that the project's type casts were split 50/50? 2 had whitespace after the cast, and 2 didn't... Made them all consistant.

Nitpick-CI · 2017-08-08T00:14:56Z

src/polyfill.php

+ */
+if (!class_exists(ReflectionType::class, false)) {
+    /* Dummy polyfill class */
+    class ReflectionType


Each class must be in a namespace of at least one level (a top-level vendor name)

scrutinizer-notifier · 2017-08-08T00:17:04Z

The inspection completed: 1 new issues, 9 updated code elements

loren-osborn · 2017-08-08T00:40:30Z

@aik099, would you believe the whitespace after cast operators were split 50-50? There were only 4 casts total, so I made them all consistent by removing the whitespace from 2 existing casts. Everything else is what we discussed.

Thanks again.

aik099 · 2017-08-08T09:18:12Z

Merging, thanks @loren-osborn .

loren-osborn · 2017-08-08T12:41:22Z

@aik099, Thanks again for all your hard work on this PR.

An aside: I mentioned several improvements I've made to this project (and Codeception/AspectMock) for a former employer that I wanted to share. I am no longer with this company, and they have declined any interest in contributing these in the foreseeable future, despite my urging. They did mention they may be willing to release these for a fee. No amount was discussed, but I suggested a hypothetical future employer of mine might want to pay "a few grand" (that's a few $1000 USD to non-native speakers) for such changes, that could save them several months of my salary to reproduce such features. I have no idea how receptive to this idea this company still is, but if any of you want to raise some funds for this, I can help you get in touch with those in a position to discuss releasing those changes.

@loren-osborn , can you list the changes included in there, so that we'll know what's coming?

Hmm... The overarching changes are actually in Codeception/AspectMock:

Adding the ability to Codeception/AspectMock to mock calls from code written in the default namespace

Proper handling of functions and methods returning references, or taking references as arguments.

Intelligent handling of error and exception handling, and custom autoloaders.

Several other features that escape me at the moment.

This was all in support of running Drupal7 within AspectMock. Several changes were made to GoAOP!/ParserReflection to support this including:

Recognizing and using inline parent classes/interfaces/traits without any special handling from the Locator. This was accomplished by bucket-sorting all functions, classes and constants when each file is parsed. (Locator needs to be aware of prerequisite classes/interfaces/traits #73 and Add support for analysing multiple classes in one file without asking the locator #64)

Fixed some behavior inconsistent with native Reflections: (implementsInterface() isSubclassOf() method inconsistancies in ReflectionClass #72)

Added passthrough to native reflection classes, so ParserReflections can be used for builtin classes. (The "\Go\ParserReflection\ReflectionEngine::locateClassFile" doesn't work with built-in classes #17)

Added factory to support seamless reuse for derived reflection classes. (Injected factory instead of explicit "new X()" ? #2)

Added support for multiple concurrent locators, so each locator fails over to the next.

Added support for locators finding functions (and to some extent, constants) in addition to classes. Obviously this is not helpful for working with an autoloader (such as composer's), but can be appropriate for a custom locator.

Added a "brute force" locator as a last-resort measure (to be used in situations where speed isn't a major concern)

I'm sure there's more, but these are the enhancements that come to mind.

Updated: Just to clarify, none of this is "coming" unless someone can persuade my former employer (probably with a non-trivial monetary payment) to share these changes.

I never did see any reaction from you to this list of changes. I any of us is likely to be willing (or in my case able right now) to pay for these by themselves, but:

Would a change list like this be welcome? (I realize getting a big heap of code cleaned up for integration into a project like this is a lot of work. I happen to know that @lisachenko wanted the cache of parsed files to remain shared as a class static, for instance, that I would need to change back.)
I imagine if some of these are really wanted by users (I've seen repeated calls for global namespace support in AspectMock, for example) and a few dozen users want to pool their resources, it might be within reasonable reach? ($100-200/user split 20-35 ways? Perhaps I overestimate the interested user base?)
If I can reproduce any of these features (gradually) here myself, would they be welcome?

Perhaps someone could setup a GoFundMe page for this? (I suspect I would likely be the wrong person to do this. I don't want to rub anyone at my previous contract employer the wrong way.)

What are your thoughts?

aik099 · 2017-08-08T14:45:12Z

Would a change list like this be welcome? (I realize getting a big heap of code cleaned up for integration into a project like this is a lot of work. I happen to know that @lisachenko wanted the cache of parsed files to remain shared as a class static, for instance, that I would need to change back.)

You can create each of mentioned improvements as an issue (lots of small manageable issues) and then anybody who is interested can express that interest.

I haven't personally encountered any of fixed issues or a need to described improvements.

I imagine if some of these are really wanted by users (I've seen repeated calls for global namespace support in AspectMock, for example) and a few dozen users want to pool their resources, it might be within reasonable reach? ($100-200/user split 20-35 ways? Perhaps I overestimate the interested user base?)

If there is a platform for funding such type of work (e.g. KickStarter), then why not. I have no experience with cases like this (when former employer doesn't want to opensource changes for free).

If I can reproduce any of these features (gradually) here myself, would they be welcome?

Time is not an issue here. If you report the issues/features as separate tasks, then you can eventually solve them.

loren-osborn · 2017-08-08T15:16:55Z

@aik099, as was mentioned already, many of the improvements already have open issues for this project.

Oh, one other cool feature I didn't include in the above list (which is really more part of ApectMock, but I feel still relevant here) is ability to capture and intercept some custom autoloaders, overriding their include/require calls, and use them within ParserReflection's locators.

Fixing issues, reported by Scrutinizer CI 1. most of issues addressed 2. normalized typecast operator look 3. replaced magic numbers with PHP constants 4. added parameter type checks for `ReflectionFile` and `ReflectionFileNamespace` classes

loren-osborn added 2 commits July 3, 2017 16:26

Work in Progress: Fixing issues reported by scrutinizer.

8de7f8c

Addressed all remaining Scrutinizer issues.

294ea1e

Nitpick-CI reviewed Jul 23, 2017

View reviewed changes

Resolved (most) issues in PR found by Nitpick

7dbdc50

Nitpick-CI reviewed Jul 23, 2017

View reviewed changes

Minor fix

6d26c55

aik099 requested changes Jul 25, 2017

View reviewed changes

loren-osborn commented Jul 25, 2017

View reviewed changes

loren-osborn closed this Jul 26, 2017

Nitpick-CI reviewed Aug 2, 2017

View reviewed changes

Trying to make Nitpick happy (whitespace changes)

266f378

Nitpick-CI reviewed Aug 2, 2017

View reviewed changes

aik099 requested changes Aug 2, 2017

View reviewed changes

Minor fixes suggested by @aik099

295b2da

Nitpick-CI reviewed Aug 6, 2017

View reviewed changes

aik099 requested changes Aug 6, 2017

View reviewed changes

Addressed the last of @aik099's PR concerns.

98cfabb

Would you believe that the project's type casts were split 50/50? 2 had whitespace after the cast, and 2 didn't... Made them all consistant.

Nitpick-CI reviewed Aug 8, 2017

View reviewed changes

aik099 approved these changes Aug 8, 2017

View reviewed changes

aik099 merged commit c60371e into goaop:master Aug 8, 2017

lisachenko added the enhancement label Aug 10, 2017

lisachenko added this to the 1.3.0 milestone Aug 27, 2017

Scrutinizer fixes #76

Scrutinizer fixes #76

Conversation

loren-osborn commented Jul 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aik099 commented Jul 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loren-osborn commented Jul 23, 2017 • edited Loading

loren-osborn commented Jul 24, 2017 • edited Loading

aik099 commented Jul 25, 2017 • edited Loading

aik099 commented Jul 25, 2017 • edited Loading

aik099 commented Jul 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loren-osborn Jul 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aik099 Jul 26, 2017 • edited Loading

Choose a reason for hiding this comment

loren-osborn Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aik099 commented Jul 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loren-osborn commented Jul 25, 2017

aik099 commented Jul 26, 2017

lisachenko commented Jul 26, 2017

lisachenko commented Jul 26, 2017

loren-osborn commented Jul 26, 2017

lisachenko commented Jul 26, 2017

aik099 commented Jul 26, 2017

loren-osborn commented Jul 26, 2017

aik099 commented Jul 26, 2017

lisachenko commented Jul 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loren-osborn commented Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loren-osborn Aug 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loren-osborn commented Jul 23, 2017 •

edited

Loading

loren-osborn commented Jul 24, 2017 •

edited

Loading

aik099 commented Jul 25, 2017 •

edited

Loading

aik099 commented Jul 25, 2017 •

edited

Loading

loren-osborn Jul 26, 2017 •

edited

Loading

aik099 Jul 26, 2017 •

edited

Loading

loren-osborn Jul 27, 2017 •

edited

Loading

loren-osborn commented Aug 2, 2017 •

edited

Loading

loren-osborn Aug 6, 2017 •

edited

Loading