Raise the PHP version requirement to >= 7.1

Question

Raise the PHP version requirement to >= 7.1

oliverklee opened this issue 3 years ago · 26 comments

To use the latest version of PHPStan, we'll need at least PHP 7.1 in order to be able to do a composer install.

@danon Would it be okay for you if we raised the version requirement to PHP >= 7.1?

As this will be a kind-of-breaking change, this would warrant a new major release as the next release (i.e., 3.0.0). Maybe a bug-fix version release just before the change of the PHP requirements would make sense so that users with PHP 7.0 still can benefit from the cleanup that has happened since the last release.

@danon What do you think?

Answer 1 · 2021-05-30T13:23:57.000Z

I planned to introduce brraking changes in 3.0.0, for some time now, the implementation is on its way, so maybe that's where we could drop support for PHP 7.0.

For now, can we use a Little older versions of PhpStan?

Answer 2 · 2021-05-30T13:42:47.000Z

For now, can we use a Little older versions of PhpStan?

Not if we want to have the baseline in order to work on the issues one-by-one while preventing new issues from getting in. (Without the baseline file, the PHPStan checks currently will not pass even at the lowest PHPStan level.)

Answer 3 · 2021-05-30T15:04:04.000Z

We could also keep PHPStan in a separate branch (that then requires PHP >= 7.1) and still use the baseline, and then merge it later into master when PHP >= 7.1 is a requirement. This would result in us not checking PRs for whether they introduce new typing issues in the meantime, though.

Answer 4 · 2021-05-31T16:00:36.000Z

I'm not sure.

Seems like a lot of work, a lot of complication and a lot of additional details to keep in my, for seamingly not-so-necessary feature.

Are you sure it's worth it?

Answer 5 · 2021-05-31T16:53:28.000Z

Seems like a lot of work, a lot of complication and a lot of additional details to keep in my, for seamingly not-so-necessary feature.

With "not-so-necessary feature", are you referring to using PHPStan at all, using PHPStan in the CI pipeline, or to the baseline feature?

Answer 6 · 2021-05-31T16:53:41.000Z

And what would be a lot of work?

Answer 7 · 2021-05-31T17:00:44.000Z

@oliverklee Correct me if I'm wrong; but aren't we doing that to make sure that all of the files have declare(strict_types=1) in all of our codebase? And that, in return is to make sure that all types are checked strictly in the source code?

And to keep a separate branch with PhpStan run on PHP 7.1, then apply fixed based on that into the master branch, before droping support for PHP 7.0, I'm not sure.

Seems like that can be done on a fork. One can change PHP 7.0 to 7.1 on his fork, install phpstan there, and use that fork to find errors, then apply fixes via pull requests. And only merge the final fork, when PHP7.0 is in fact dropped.

Answer 8 · 2021-05-31T17:01:03.000Z

I can't really help you with the implementation, since I'm working on an idea suggested here: MyIntervals/emogrifier#1000 (comment)
But I'm open for discussion.

Answer 9 · 2021-06-02T18:51:59.000Z

@danon I think you are missing up things a bit here. 😄 I'll do my best to clear things up now.

First, concerning the tools:
The style checker PHP_CodeSniffer can check mostly for style issues: Indentation, camelCase, braces at the correct place, and also files being strict. PHP_CodeSniffer does not need PHP 7.1, and it has no baseline file, but a file defining the used rules.
The type checker PHPStan checks that called methods actually exist, that they are called with the correct number of parameters and with the correct types etc. PHPStan (in the latest version) requires PHP >= 7.1, has a baseline file, and uses levels to set its strictness. The bug about get_resource (#28) is one of the issues this tool has found.

Then there are several changes:

#16: Make all files strict: can be merged as-is and works fine with PHP 7.0
#18: Add sniffer Composer script (using PHP_CodeSniffer): can be merged as-is, works fine with PHP 7.0, and check that all files are strict if called manually
check during the CI build that all files are strict: I will build this once both #16 and #18 are merged
make all files PSR-12 compliant: I will do this soon
check for PSR-12 compliance: will use PHP_CodeSniffer; I will build this once #18 is merged and all files are PSR-12 compliant

Does this explanation clear things up for you a bit?

Answer 10 · 2021-06-04T13:12:51.000Z

Does this explanation clear things up for you a bit?

Yes, it does. So I understand there are two operations going on. First operation is to make files strict, and second operation is to make files PHP-12 compliant, is that correct?

Answer 11 · 2021-06-04T13:22:15.000Z

Yes, it does. So I understand there are two operations going on. First operation is to make files strict, and second operation is to make files PHP-12 compliant, is that correct?

Exactly. And once each operation is finished, we can enforce that corresponding change is kept with a PHP_CodeSniffer rule(set).

Answer 12 · 2021-06-04T13:27:32.000Z

Note to myself: When planning this kind of multi-PR change with a bigger plan behind it for open source projects, better start by creating tickets that explain this plan to the maintainer(s). :-)

EDIT: Done: #31

Answer 13 · 2021-06-04T13:40:35.000Z

@oliverklee I created two GithubProject:

Could you visit each and everyone of your issues and PRs, and add each of them to one of the projects? On your right-hand side, you will see "Projects" secition. Please add each issue to one of them:

Answer 14 · 2021-06-04T14:15:37.000Z

Thanks for the invitation! I think I'll need some more permissions - I still cannot edit the project association of my PRs. (Once I can do so, I'll be happy to help organize my contributions.)

Answer 15 · 2021-06-04T14:17:38.000Z

@oliverklee I've raised permissions for you. You now also have write access to master barnch, please don't overuse it.

You're free to push typos, small fixes and refactors; but other changes like linters, composer scipts, features etc. please do through pull requests (can be from branch in this repo, or in branch of your fork).

Answer 16 · 2021-06-04T14:22:31.000Z

You're free to push typos, small fixes and refactors; but other changes like linters, composer scipts, features etc. please do through pull requests (can be from branch in this repo, or in branch of your fork).

Actually, I'd prefer to not push directly to master at all, and always create PRs for changes if that okay for you. 🙂

Answer 17 · 2021-06-04T14:23:51.000Z

I propose we also have a project for the PHPStan-related tasks. I can create it if you like. Which template did you use for the other two projects?

Answer 18 · 2021-06-04T14:23:54.000Z

You're free to push typos, small fixes and refactors; but other changes like linters, composer scipts, features etc. please do through pull requests (can be from branch in this repo, or in branch of your fork).

Actually, I'd prefer to not push directly to master at all, and always create PRs for changes if that okay for you. 🙂

Sure it's fine.

Are there issues and pull requests who belong to both operations? Or

Answer 19 · 2021-06-04T14:24:21.000Z

I propose we also have a project for the PHPStan-related tasks. I can create it if you like. Which template did you use for the other two projects?

I don't follow.

I though the PhpStan was for the PSR-12 compliace operation?

Answer 20 · 2021-06-04T14:26:20.000Z

I though the PhpStan was for the PSR-12 compliace operation?

No, that was PHP_CodeSniffer (which I intend to use both for PSR-12 checks as well as for checking that all PHP files are declared as strict.)

PHPStan is for detecting incorrect method calls, non-matching types, undefined variables etc.

Answer 21 · 2021-06-04T14:28:14.000Z

(But there probably are some code issues that both tools can find.)

Answer 22 · 2021-06-04T14:30:32.000Z

@oliverklee So there's acutally three operations? Third of which is using PhpStan as a semi-compile time method verification?

If there' are really 3 operations, then I guess you should create a third project for that. I didn't use any template. I added columns Discussion, Implementation and Done without any preset and automation.

Answer 23 · 2021-06-04T14:35:17.000Z

Thanks, done: https://github.com/T-Regx/CrossDataProviders/projects/3

Answer 24 · 2021-06-05T12:04:08.000Z

With the new approach in #24, we don't need to raise the PHP version requirements after all. Hence closing.

Answer 25 · 2021-06-06T11:30:56.000Z

@oliverklee Wait a minute, how are we going to enfoce PSR-12 compliance? Weren't we planning on enforcing that with PhpStan? Or was it with the other tool?

Answer 26 · 2021-06-06T11:37:01.000Z

@danon

PHP_CodeSniffer for PSR-12, "strict types" declaration, style and naming
PHPStan: type checks, correct number of parameters etc.