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

False positive for require_once .phar - cannot be loaded via file_get_contents #478

Closed
kkmuffme opened this issue Jun 15, 2020 · 5 comments · Fixed by #586
Closed

False positive for require_once .phar - cannot be loaded via file_get_contents #478

kkmuffme opened this issue Jun 15, 2020 · 5 comments · Fixed by #586
Milestone

Comments

@kkmuffme
Copy link

require_once( 'some.phar' );

will trigger:
WordPressVIPMinimum.Files.IncludingNonPHPFile.IncludingNonPHPFile

.phar files however must be loaded via include/require, as otherwise they won't work.
Please adjust the sniff.

@GaryJones
Copy link
Contributor

What's the use case for needing to include a .phar file?

@kkmuffme
Copy link
Author

various libraries, e.g. some of composer, come as .phar (or some are even only available as phar, but in general it's most often easier to use the phar if its available)
e.g. maxmind geoip, AWS PHP SDK,...

e.g use cases for AWS SDK is translation, SMS, email, customer service,...

@GaryJones
Copy link
Contributor

These are typically development tools to be run locally, and usually available to be installed via Composer - as such, they wouldn't need to be pushed up to a repo. As such, you could ignore them, either at your PHPCS config file level, or with a PHPCS ignore line at the point of inclusion.

I haven't seen requests for this before to make me consider making a change to the sniff itself.

@kkmuffme
Copy link
Author

All things mentioned in my previous comment are NOT development tools and must be pushed in the plugin/repo despite being installed with composer.

If I provide a pull request that includes .phar will it be merged?

@jrfnl
Copy link
Collaborator

jrfnl commented Sep 24, 2020

I agree with @kkmuffme on this. A fix for this is included in PR #586.

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

Successfully merging a pull request may close this issue.

3 participants