Raise the PHP version requirement to >= 7.1 about phpunit-data-provider HOT 26 CLOSED

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

from phpunit-data-provider.

@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).

from phpunit-data-provider.

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. 🙂

from phpunit-data-provider.

@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.

from phpunit-data-provider.

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?

from phpunit-data-provider.

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.)

from phpunit-data-provider.

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.

from phpunit-data-provider.

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?

from phpunit-data-provider.

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?

from phpunit-data-provider.

And what would be a lot of work?

from phpunit-data-provider.

@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.

from phpunit-data-provider.

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.

from phpunit-data-provider.

@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?

from phpunit-data-provider.

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?

from phpunit-data-provider.

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).

from phpunit-data-provider.

@oliverklee I created two GithubProject:

• https://github.com/T-Regx/CrossDataProviders/projects/1
• https://github.com/T-Regx/CrossDataProviders/projects/2

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:

from phpunit-data-provider.

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.)

from phpunit-data-provider.

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?

from phpunit-data-provider.

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

from phpunit-data-provider.

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?

from phpunit-data-provider.

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.

from phpunit-data-provider.

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

from phpunit-data-provider.

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

from phpunit-data-provider.

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

from phpunit-data-provider.

@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?

from phpunit-data-provider.

@danon

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

from phpunit-data-provider.