-
Notifications
You must be signed in to change notification settings - Fork 64
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
Update Reflector.php for PHP 7.2 compatibility #148
Conversation
src/Resolver/Reflector.php
Outdated
@@ -129,7 +129,7 @@ public function getTraits($class) | |||
} while ($class = get_parent_class($class)); | |||
|
|||
// get traits from ancestor traits | |||
while (list($trait) = each($traits)) { | |||
foreach($traits as &$trait) { |
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.
Thank you for the information.
Why did you used &
?
It may be good if you can add a space between foreach
and (
as foreach ($traits
.
Not sure why the tests fails, will look into it.
Thank you.
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 first attempt didn't use the &
. In fact, prefer not to use it.
But running the tests on PHP 7.1 triggered the same errors you're seeing on 5.5 and 5.6.
So while looking at the recursive flow in that code section a colleague suggested the &
might resolve it. It did ... for PHP 7.1.
Still not understanding why it's breaking on 5.5 and 5.6
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.
There should be a space after foreach
and before the opening parenthesis.
I made some changes to .travis.yml and composer.json and made a PR #149 . Currently it fails on 5.5 and 5.6 . I may want to dig more in-order to find the reason behind the same. |
PHP 7.2 has deprecated `each()`
Tests are now passing on PHP 5.5 through 7.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.
Thank you for the changes. @pmjones will verify and merge. I am a bit concerned about the performance with this. But I don't know for sure without bench-marking this.
composer.json
Outdated
@@ -19,7 +19,8 @@ | |||
], | |||
"require": { | |||
"php": ">=5.5.0", | |||
"container-interop/container-interop": "~1.0" | |||
"container-interop/container-interop": "~1.0", | |||
"phpunit/phpunit": "~5.7 || ~4.8" |
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.
move phpunit to require-dev.
@harikt Point taken on performance; no way to know without benchmarking. However, my guess is that, since we're using reflection already, the loop change is not a significant factor. |
@drbyte Thanks for this! |
PHP 7.2 has deprecated
each()