Add sniff Generic.Files.FilePermissions to check if file is executable

[MasterOdin]

Closes #1808.

Adds Sniff to detect if a file is executable or not.

Tested this manually to check behavior for a file marked as:

• 400
• 500
• 410
• 401
• 644
• 755

Not sure how I might add an automated test for this sniff. Also, errors for this sniff would be easily fixable, but I'm not sure how I would register a fix in the Fixer since it doesn't change any tokens, and I couldn't find an example anywhere.

[jrfnl]

To test this sniff, you could add two different test case files with basically just a PHP open tag in each of them.
One should have a valid permission, one an invalid one.

You can then use a switch on the received file name in the test file to get it working.
See:

PHP_CodeSniffer/src/Standards/Generic/Tests/Files/EndFileNewlineUnitTest.php

Lines 28 to 40 in b2c1546

	public function getErrorList($testFile='')
	{
	switch ($testFile) {
	case 'EndFileNewlineUnitTest.3.inc':
	case 'EndFileNewlineUnitTest.3.js':
	case 'EndFileNewlineUnitTest.3.css':
	case 'EndFileNewlineUnitTest.4.inc':
	return [2 => 1];
	default:
	return [];
	}//end switch
	}//end getErrorList()

[jrfnl]

You may want to add T_OPEN_TAG_WITH_ECHO as well to also examine files which only use the short echo PHP tag.

[MasterOdin]

Most of the other File standards seem to use a similar register. Would it make sense to also update those to also have that tag? I'm fine with just adding it here though.

[jrfnl]

I think that depends on the individual sniff and should probably be done in a separate PR in that case.

[MasterOdin]

Within just Generic, it includes all the other sniffs that test for the beginning of a file:

• EndFileNewlineSniff
• EndFileNoNewlineSniff
• LineEndingsSniff
• LineLengthSniff
• LowercasedFilenameSniff

Agreed that fixing them would be a separate PR, just trying to clarify what the expected style though as I'm not well-versed enough in this project to know when to use one or both.

[jrfnl]

Without looking at the sniffs you've listed - IMO the consideration here would be "does this rule apply to all PHP files or just to PHP-only files ?"

If the rule applies only to PHP-only files, then don't add it.
If the rule applies to PHP-only files as well as mixed HTML/PHP files, then yes, it should be added.

[jrfnl]

Some questions/remarks:

1. The error message says must, the documentation says should. Possibly friendlier: doesn't need ?
2. Should this be a warning or an error ?
3. Would it be helpful to end-users to know what the permission found was ? I.e. ... be executable. Found file permission set to %s, where %s would be replaced by passing the found file permission in $data to the addError()/addWarning() method.
4. As a PHP file can mix PHP with HTML, the open tag may not be on the first line of the file and the way the error is now thrown, the error wouldn't be thrown on line 1 either.
   Would it be better to always throw this error on line 1 of a file (just pass token 0 to do so) ?

[MasterOdin]

1. Agreed to changing it to should and the must was on an original draft that I changed in the docs, but forgot to change it here. Can change this here and in docs to doesn't need, but I think it depends on if this is an error or a warning.
2. Personally, for my work, I'd like this to be an error, but can change if requested and just set the severity in the ruleset.xml
3. Agreed
4. Thanks for this, I did not consider that. Fixed.

[MasterOdin]

Thank you for your comments/review. I've addressed some of the comments I could and asked for clarification on some points. Added some tests per your suggestion, will see if they commit with the correct set of permissions in Travis.

[gsherwood]

I've just got around to testing this sniff and came across a problem. It doesn't work with the caching system in PHPCS.

PHPCS checks that the file contents (and names) of the files it is checking haven't changed before using the cache, but it doesn't detect permission changes. So that means that when you fix the perms on the file, running PHPCS again still shows the same error.

I've removed this from the 3.5.0 release so I can have more of a think about it. I'm not sure if I should include file perm checking in the cache just in case this sniff is used, or if the sniff should be able to signal that it makes the request uncacheable in some way. Or some other option.

The sniff and tests work great though. Thanks.

[MasterOdin]

From my standpoint, I would think that including file permissions in the cache details would probably be the most straightforward approach and would probably not affect that many people (as I assume that most people are not changing the permissions of files with any great frequency), while also still making this type of sniff benefit from having the cache, which I'd imagine would be useful for very large codebases.

[gsherwood]

I've merged this in. Thanks a lot for the PR.

I ended up renaming the sniff because I thought it sounded too generic, as if it would provide you the ability to check file permissions based on whatever your standard is. It is now called Generic.Files.ExecutableFile instead, so it is limited to checking the executable flag on a file.

I modified the core cache code to include the permissions of a file in its hash so that this sniff works correctly with the existing cache feature. It didn't appear to add much time to the run.