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

Allow PHPLint to handle large number of files #543

Merged
merged 2 commits into from Nov 30, 2018

Conversation

gsomoza
Copy link
Contributor

@gsomoza gsomoza commented Sep 21, 2018

Q A
Branch gsomoza/grumphp:phplint-input-stream
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Documented? not needed
Fixed tickets none, but see #524

See #524: this task allows phplint to work with large number of files.

@Landerstraeten
Copy link
Contributor

Hi @gsomoza. Thanks for your PR. Can you take a look a the build? There is a problem with the lowest dependencies.

Because \Symfony\Components\Process\InputStream doesn't exist in previous versions.
@gsomoza
Copy link
Contributor Author

gsomoza commented Oct 18, 2018

Fixed it by bumping the minimum required version for symfony/process to 3.2, which is where they introduced the InputStream class. I tried to find an approach where the dependency is injected and typed as an interface, but there's just no appropriate interface for InputStream so that idea is probably a dead end – unless we introduce our own interface and wrap InputStream, but I don't think there's enough value in doing that because the current approach is still not a BC break anyway:

According to semver.org you should still be able to release this as a non-BC break change because the public API didn't really change (see this comment).

In practice, (very old) clients that have a hard dependency on an older version of symfony/process will not be able to upgrade this minor bump because Composer will not let them.

So with the above in mind I'd recommend bumping minor version in this case (but of course that's up to you).

@Landerstraeten Landerstraeten merged commit d2d188b into phpro:master Nov 30, 2018
@Landerstraeten Landerstraeten added this to the Version 0.14.3 milestone Nov 30, 2018
@gsomoza
Copy link
Contributor Author

gsomoza commented Nov 30, 2018

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants