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

IncludingFile: Support runtime setting for custom variables in paths. #466

Open
GaryJones opened this issue Feb 6, 2020 · 0 comments
Open

Comments

@GaryJones
Copy link
Contributor

What problem would the enhancement address for VIP?

When a file includes a load of other includes/requires, then part of the path might be assigned to a variable, to make code DRY.

e.g.

$my_path = 'path/to/includes/;
require $my_path . 'foo.php';
require $my_path . 'bar.php';
require $my_path . 'baz.php';

The IncludingFileSniff checks the path, and other than a few exclusions for common functions and constants, it throws warnings for any other constants, functions and variables.

Since the prefix is often consistent for the file, the bot will add comment for each line.

The arrays of known constants and functions are all in public properties, so they can be overridden (though not implemented in the best way - see #234), but variables cannot.

Describe the solution you'd like

How about we made the sniff look for a runtime setting comment that defined that a particular variable was OK / should be ignored for file inclusion?
It would be set it at the top before all the includes, do the includes, then unset it again after.

$my_path = 'path/to/includes/;

// phpcs:set WordPressVIPMinimum.Files.IncludingFile custom_variables[] $my_path
require $my_path . 'foo.php';
require $my_path . 'bar.php';
require $my_path . 'baz.php';
// phpcs:set WordPressVIPMinimum.Files.IncludingFile custom_variables[] 

The bot workflow should be able to follow it since it would be an inline comment, not in a PHPCS config file.

The same could already be done with the public properties for known functions and constants, but as can be seen from the snippet above, the ideal unsetting would need to include the current values, which is awkward. As such, public custom* properties that are merged with the existing properties would be better.

What code should be reported as a violation?

No new code.

What code should not be reported as a violation?

Same as example above, where a runtime setting matches the name of a variable.

Anything else

Unit tests should also cover items like require $my_path . $my_other_path . 'baz.php'; where there are multiple variables, or require MY_CONSTANT . $my_path . 'baz.php'; where there are multiple custom items.

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

No branches or pull requests

1 participant