Skip to content
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

Php7 compatibility update #16

Merged
merged 6 commits into from Nov 15, 2016
Merged

Php7 compatibility update #16

merged 6 commits into from Nov 15, 2016

Conversation

@mallardduck
Copy link
Contributor

mallardduck commented Nov 5, 2016

This set of commits fixes the issue of #15 that I recently reported.

All tests were passing still on my end and this now allows me to run with out commenting out GREs. However once I started using ./vendor/bin/phpunit I got all sorts of errors in tests. I made an attempt at fixing most of them but I wasn't familiar enough with the packing and how to properly fix the remaining test that has issues.

mallardduck added 3 commits Nov 5, 2016
Changes Cli and Analyser to php-parser 2.0 which supports PHP 7 parsing.
I was getting errors when running phpunit and was seeing the following:

"Class 'Prophecy\PhpUnit\ProphecyTestCase' not found"
@mallardduck mallardduck force-pushed the mallardduck:php7-update branch from 8e26b71 to 2f87788 Nov 5, 2016
mallardduck added 2 commits Nov 5, 2016
Well somehow running with the globally installed phpunit I wasn't seeing errors. running with vendor phpunit and I was seeing errors.
First I tried an update to local phpunit, fix a few things and now there's only a single error left in the tests.

While I was able to fix the issue I'm far too naiev to how this and php-parser package function to write a proper fix for the test. Also, since I have never used phrophecy with my unit tests, I'm a bit unfamiliar with how to use that correctly.
@mallardduck mallardduck force-pushed the mallardduck:php7-update branch from fcc5952 to 6112c12 Nov 5, 2016
@rskuipers

This comment has been minimized.

Copy link
Owner

rskuipers commented Nov 6, 2016

@mallardduck Thank you very much for taking the time to submit a pull request!
I'll be picking this up the upcoming week and I'll have a look to see what I can make of it. 👍

* @param NodeTraverserInterface $nodeTraverser
*/
public function __construct(
ParserAbstract $parser,
Multiple $parser,

This comment has been minimized.

Copy link
@nikic

nikic Nov 15, 2016

This should hint against the Parser interface, instead of Multiple.

This comment has been minimized.

Copy link
@mallardduck

mallardduck Nov 15, 2016

Author Contributor

I don't think that's quite right. From what I'm seeing Cli.php creates the class being passed into this construct here:

    private function createParser()
    {
        $parser = (new ParserFactory)->create(ParserFactory::PREFER_PHP7);
        return $parser;
    }

Since this results in the creation of an instance of: PhpParser\Parser\Multiple I'm type hinting for that class.

@mallardduck

This comment has been minimized.

Copy link
Contributor Author

mallardduck commented Nov 15, 2016

Other than requiring PHP 5.6 this patch set seems to be working correctly and is now passing all tests (where possible).

I'm unsure where or how this should be merged in as the updates to PHPunit now require a minimum of PHP 5.6. I personally don't think this is a big deal since anything prior is considered End Of Life.

@rskuipers

This comment has been minimized.

Copy link
Owner

rskuipers commented Nov 15, 2016

@mallardduck Awesome! The tests are indeed passing, and dropping support for PHP 5.5 is fine since it's indeed EOL. I'll merge this and tag 0.5.0.

@rskuipers rskuipers merged commit 02967af into rskuipers:master Nov 15, 2016
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.