Fix: Ignore anonymous classes #34

localheinz · 2017-01-11T10:30:28Z

This PR

asserts that anonymous classes are ignored
ignores anonymous classes

Follows #29.

weierophinney · 2017-01-11T14:47:35Z

@localheinz Please rebase to resolve conflicts, which will trigger a new build as well. Thanks!

localheinz · 2017-01-11T14:48:12Z

@weierophinney

Sorted!

localheinz · 2017-01-11T14:49:54Z

And thanks a lot for reviewing these on such a short notice, I appreciate it!

Since https://github.com/zendframework/modules.zendframework.com (where I am a maintainer) is kind of dead, I wouldn't mind getting involved as a maintainer with a different Zend project, please let me know if there's interest!

localheinz · 2017-01-11T14:56:32Z

src/ClassFileLocator.php

+
+                    // ignore anonymous classes on PHP 7.1 and greater
+                    if (PHP_VERSION_ID >= 70100
+                        && $i > 2


Ha, should this be $i > 1, actually?

You tell me; haven't played with tokenizing anonymous classes yet. :) What happens in the tests?

weierophinney

Noted a few changes. Additionally, in the files changed (but not introduced), update the copyright in the file-level docblock to reference 2017.

Thanks!

weierophinney · 2017-01-11T14:58:54Z

src/ClassFileLocator.php

+
+                    // ignore anonymous classes on PHP 7.1 and greater
+                    if (PHP_VERSION_ID >= 70100
+                        && $i > 2


You tell me; haven't played with tokenizing anonymous classes yet. :) What happens in the tests?

weierophinney · 2017-01-11T14:59:08Z

src/ClassFileLocator.php

+                        && \is_array($tokens[$i - 1])
+                        && $tokens[$i - 1][0] === T_WHITESPACE
+                        && \is_array($tokens[$i - 2])
+                        && $tokens[$i - 2][0] === T_NEW


Use yoda conditions for the constant comparisons here.

weierophinney · 2017-01-11T15:01:16Z

test/TestAsset/Anonymous/WithAnonymousClass.php

+ *
+ * @link      http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2015 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license   http://framework.zend.com/license/new-bsd New BSD License


Three changes:

Reference the zend-file repository, using https.

Since this file is just being introduced, change the copyright date to simply 2017.

Point to https://github.com/zendframework/zend-file/blob/master/LICENSE.md for the license.

weierophinney · 2017-01-11T15:01:29Z

test/TestAsset/Anonymous/with-anonymous-class.php

+ *
+ * @link      http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2015 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license   http://framework.zend.com/license/new-bsd New BSD License


Same notes on this docblock as for the previous.

localheinz · 2017-01-11T15:15:22Z

src/ClassFileLocator.php

+
+                    // ignore anonymous classes on PHP 7.1 and greater
+                    if (PHP_VERSION_ID >= 70100
+                        && $i >= 2


Modified this condition

-&& $i > 2 +&& $i >+ 2

as the index doesn't need to be greater than 2, but at least 2 (since we try to access the element at $i - 2).

weierophinney · 2017-01-11T15:16:34Z

Will merge once I verify the build with Travis. Currently their running on a bit of a delay, due to the queue.

localheinz · 2017-01-11T15:16:50Z

Thanks a ton, @weierophinney!

weierophinney · 2017-01-11T15:23:07Z

@localheinz Already seeing errors on all jobs run so far: https://travis-ci.org/zendframework/zend-file/builds/190986651

localheinz · 2017-01-11T15:29:35Z

test/TestAsset/Anonymous/with-anonymous-class.php

+
+namespace ZendTest\File\TestAsset\Anonymous;
+
+$anonymous = new class extends \stdClass {


The question is, on the other hand, should the ClassFileLocator actually accept a PHP file that doesn't contain any classes?

I would expect that it would ignore it.

To clarify: the intent of the class is to find files that declare named classes. It was originally developed as a utility for creating class maps for autoloading. Anonymous classes, as they are not named, would fall outside that criteria.

Will update later, @weierophinney!

localheinz · 2017-01-11T15:58:24Z

@weierophinney

Amended the previous commit:

diff --git a/src/ClassFileLocator.php b/src/ClassFileLocator.php
index 82619a2..767fee4 100644
--- a/src/ClassFileLocator.php
+++ b/src/ClassFileLocator.php
@@ -124,8 +124,7 @@ class ClassFileLocator extends FilterIterator
                     }

                     // ignore anonymous classes on PHP 7.1 and greater
-                    if (PHP_VERSION_ID >= 70100
-                        && $i >= 2
+                    if ($i >= 2
                         && \is_array($tokens[$i - 1])
                         && T_WHITESPACE === $tokens[$i - 1][0]
                         && \is_array($tokens[$i - 2])

👍

weierophinney · 2017-01-11T16:04:23Z

src/ClassFileLocator.php

@@ -122,6 +122,17 @@ public function accept()
                    if ($i > 0 && is_array($tokens[$i - 1]) && $tokens[$i - 1][0] === T_DOUBLE_COLON) {
                        break;
                    }
+
+                    // ignore anonymous classes on PHP 7.1 and greater
+                    if (&& $i >= 2


Um... && to start the conditional...? Also, will $i >= 2 only ever happen on PHP 7.1, or will this work on any version?

Ha, hehe!

Well, I guess trying to run

$tokens = token_get_all($contents);

when $contents contains usage of anonymous classes will already fail on a PHP version less than PHP 7.1, what do you think?

Actually, no, works fine on all PHP versions, see https://3v4l.org/0enag!

Fix: Ignore anonymous classes

Close #34

Forward port #34 Conflicts: CHANGELOG.md

localheinz · 2017-01-11T17:21:13Z

Thanks a ton, @weierophinney!

localheinz force-pushed the fix/anonymous branch 4 times, most recently from eaea370 to 484e5c8 Compare January 11, 2017 14:43

localheinz force-pushed the fix/anonymous branch from c3b7e29 to 2058712 Compare January 11, 2017 14:47

localheinz commented Jan 11, 2017

View reviewed changes

weierophinney suggested changes Jan 11, 2017

View reviewed changes

Enhancement: Assert that locator ignores anonymous classes

a2cec18

localheinz force-pushed the fix/anonymous branch 2 times, most recently from ee58139 to b91ff16 Compare January 11, 2017 15:12

localheinz commented Jan 11, 2017

View reviewed changes

weierophinney approved these changes Jan 11, 2017

View reviewed changes

localheinz commented Jan 11, 2017

View reviewed changes

localheinz force-pushed the fix/anonymous branch from a0087cb to a2cec18 Compare January 11, 2017 15:56

weierophinney suggested changes Jan 11, 2017

View reviewed changes

Fix: Ignore anonymous classes

8651341

localheinz force-pushed the fix/anonymous branch from c359f7f to 8651341 Compare January 11, 2017 16:06

localheinz assigned weierophinney Jan 11, 2017

localheinz added the bug label Jan 11, 2017

weierophinney approved these changes Jan 11, 2017

View reviewed changes

weierophinney merged commit 8651341 into zendframework:master Jan 11, 2017

weierophinney added a commit that referenced this pull request Jan 11, 2017

Merge pull request #34 from localheinz/fix/anonymous

f999eb5

Fix: Ignore anonymous classes

weierophinney added a commit that referenced this pull request Jan 11, 2017

Added CHANGELOG for #34

7789b0d

weierophinney added a commit that referenced this pull request Jan 11, 2017

Merge branch 'hotfix/34'

888fd48

Close #34

weierophinney added a commit that referenced this pull request Jan 11, 2017

Merge branch 'hotfix/34' into develop

0d977c4

Forward port #34 Conflicts: CHANGELOG.md

localheinz deleted the fix/anonymous branch January 11, 2017 17:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Ignore anonymous classes #34

Fix: Ignore anonymous classes #34

localheinz commented Jan 11, 2017 •

edited

Loading

weierophinney commented Jan 11, 2017

localheinz commented Jan 11, 2017

localheinz commented Jan 11, 2017

localheinz Jan 11, 2017

weierophinney Jan 11, 2017

weierophinney left a comment

weierophinney Jan 11, 2017

weierophinney Jan 11, 2017

weierophinney Jan 11, 2017

weierophinney Jan 11, 2017

localheinz Jan 11, 2017

weierophinney commented Jan 11, 2017

localheinz commented Jan 11, 2017

weierophinney commented Jan 11, 2017

localheinz Jan 11, 2017

weierophinney Jan 11, 2017

weierophinney Jan 11, 2017

localheinz Jan 11, 2017

localheinz commented Jan 11, 2017

weierophinney Jan 11, 2017

localheinz Jan 11, 2017

localheinz Jan 11, 2017

localheinz commented Jan 11, 2017


		namespace ZendTest\File\TestAsset\Anonymous;

		$anonymous = new class extends \stdClass {

Fix: Ignore anonymous classes #34

Fix: Ignore anonymous classes #34

Conversation

localheinz commented Jan 11, 2017 • edited Loading

weierophinney commented Jan 11, 2017

localheinz commented Jan 11, 2017

localheinz commented Jan 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney left a 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

weierophinney commented Jan 11, 2017

localheinz commented Jan 11, 2017

weierophinney commented Jan 11, 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

localheinz commented Jan 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

localheinz commented Jan 11, 2017

localheinz commented Jan 11, 2017 •

edited

Loading