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

Restricting supported PHP versions to 7.1.* #111

Conversation

Ocramius
Copy link
Contributor

Parsing changes with every version, and I'm sure that this package isn't supporting all PHP versions.

Since .travis.yml specifies that you are testing against PHP 7.1, the composer.json should lock onto "require":{"php":"7.1.*"} (and no further, since parsing may change in 7.2)

Parsing changes with every version, and I'm sure that this package isn't supporting all PHP versions.

Since `.travis.yml` specifies that you are testing against PHP 7.1, the `composer.json` should lock onto `"require":{"php":"7.1.*"}` (and no further, since parsing may change in 7.2)
@msftclas
Copy link

Hi @Ocramius, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. Real humans will now evaluate your PR.

TTYL, MSBOT;

@mousetraps
Copy link
Contributor

@Ocramius thanks for the PR!

Clarification question - what exactly do you mean by "parsing changes with every version"? The only thing that changes with every version is the tokens produced by the built-in lexer (which is pretty stable between versions). The parser itself is handspun, and the tree produced remains constant between versions of PHP.
https://github.com/Microsoft/tolerant-php-parser/blob/master/src/Parser.php

That said, for memory and performance reasons, it does require PHP7, and therefore I agree that all earlier versions should indeed be discluded in composer.json

Re travis: I would be open to adding a (PHP 7.0, VALIDATION=false) run. The validation tests take a while to run, and I don't think the added time is worth the potential benefit (the invariants/grammar/api tests should be sufficient to detect issues due to PHP runtime versions)

@Ocramius
Copy link
Contributor Author

(which is pretty stable between versions)

It really is a mine-field there: already had massive issues while working on https://github.com/Roave/BetterReflection when switching from 7.0 to 7.1 (nullability, iterable, etc).

In general, I simply suggest using strict constraints unless you are totally sure that you are also supporting the next release (covered by tests). That sort of strictness is necessary due to PHP not being semver compliant.

@Ocramius
Copy link
Contributor Author

The validation tests take a while to run, and I don't think the added time is worth the potential benefit (the invariants/grammar/api tests should be sufficient to detect issues due to PHP runtime versions)

The fact that it takes time is fine: let machines do their work, especially if it improves stability from a consumer perspective.

@mousetraps
Copy link
Contributor

@Ocramius - As I mentioned, happy to take a PR that adds PHP 7.0, VALIDATION=false to the travis build. This will run the invariants, grammar, and api tests, which are plenty sufficient to catch the sort of errors we're concerned about here. The validation tests are primarily to track our progress through the grammar, rather than unit tests in the traditional sense.

@Ocramius
Copy link
Contributor Author

@mousetraps if the supposedly supported versions are 7.0.*|7.1.*, I can give it a shot. My point was really about shouting out what is being supported :-)

@nikic
Copy link
Collaborator

nikic commented Feb 17, 2017

There is no reason to believe that this library will not be automatically compatible with any 7.x version. It may not be able to parse newer code, but it should run on any 7.x version. As such, the correct version constraint is ^7.0.

@mousetraps
Copy link
Contributor

@Ocramius - no worries, and agreed it's important to be clear about this stuff :-). There wasn't a particularly good reason for not including that configuration in the .travis.yml file to begin with, so thanks for pointing that out!

@roblourens
Copy link
Member

We are now running Travis tests in both 7.0 and 7.1.

@roblourens roblourens closed this Jun 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants