Skip to content
This repository was archived by the owner on Jul 19, 2025. It is now read-only.

Conversation

@maxjacobson
Copy link
Contributor

This library has a handy upgrade guide here:
https://github.com/nikic/PHP-Parser/blob/master/UPGRADE-2.0.md

Seems there's just one backwards-incompatible change for our wrapper
code.

I'd like to get this into the next release before we release the OOM
fixes. Some php repos are using newer syntax and each file fails to
parse, and then it hits an OOM memory that users aren't seeing.

Example: https://codeclimate.com/github/etsy/phan/builds/277

Would like to verify on our phan fork on the beta channel that this will
work.

@codeclimate/review @ABaldwinHunter

@wfleming
Copy link
Contributor

  1. Is there any risk of PHP that parses successfully today failing to parse with this change?
  2. I am surprised that multiple failures to parse would be related to OOM errors. The parsing is a separate process run for each file, so that memory should be recovered after parsing (or failing to parse). And if all the files fail, the main process isn't going to have many ASTs to hold in memory so that shouldn't increase much either. Can you explain more how these are related?

@maxjacobson
Copy link
Contributor Author

maxjacobson commented Oct 11, 2016

Is there any risk of PHP that parses successfully today failing to parse with this change?

I want to do some QA to feel more confident about it, but I currently feel "pretty confident" about it, going based on

  1. the test suite still passes, which exercises parsing some php 5 code
  2. edit, added: The ParserFactory::PREFER_PHP7 thing is meant to have this behavior: "Try to parse code as PHP 7. If this fails, try to parse it as PHP 5."
  3. the changelog for 2.0 says this:

PHP-Parser now requires PHP 5.4 or newer to run. It is however still possible to parse PHP 5.2 and PHP 5.3 source code, while running on a newer version.

Re: your other question

Can you explain more how these are related?

Not really, no. Agreed it seems unrelated. Ashley pointed out shortly before I left the office that there were more OSS OOM error examples than I realized, and this one jumped out as having a lot of parsing error output and also an OOM error. It might be the case that even with the parser upgrade that phan will still have an OOM error and the parser errors were a red herring. I'm not sure yet.

@maxjacobson
Copy link
Contributor Author

Btw, I added a test exercising some php 7 code, which passes, and would fail without the upgrade (https://circleci.com/gh/codeclimate/codeclimate-duplication/545)

@wfleming
Copy link
Contributor

Not really, no. Agreed it seems unrelated. Ashley pointed out shortly before I left the office that there were more OSS OOM error examples than I realized, and this one jumped out as having a lot of parsing error output and also an OOM error. It might be the case that even with the parser upgrade that phan will still have an OOM error and the parser errors were a red herring. I'm not sure yet.

My bet would be that enough files were still parsing successfully to grow memory usage and cause OOM errors. So this fix might make them happen faster 😄 .

@wfleming
Copy link
Contributor

Code LGTM.

@ABaldwinHunter
Copy link
Contributor

👏

Curious about how our phan test repo runs on with this change.

This library has a handy upgrade guide here:
https://github.com/nikic/PHP-Parser/blob/master/UPGRADE-2.0.md

Seems there's just one backwards-incompatible change for our wrapper
code.

I'd like to get this into the next release before we release the OOM
fixes. Some php repos are using newer syntax and each file fails to
parse, and then it hits an OOM memory that users aren't seeing.

Example: https://codeclimate.com/github/etsy/phan/builds/277

Would like to verify on our phan fork on the beta channel that this will
work.
@maxjacobson maxjacobson force-pushed the mj-upgrade-php-parser branch from d43c71f to 88184da Compare October 11, 2016 17:03
@maxjacobson
Copy link
Contributor Author

OK we're taking bets now 😄

Just pushed this change to the channel/beta branch, so we'll know soon.

@maxjacobson
Copy link
Contributor Author

@maxjacobson
Copy link
Contributor Author

I tested 2 more php 5 repos and they continue to be analyzed correctly, so I'm going to go ahead and ship this.

@maxjacobson maxjacobson merged commit 689c9a4 into master Oct 11, 2016
@maxjacobson maxjacobson deleted the mj-upgrade-php-parser branch October 11, 2016 20:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants