Code Sniffer version 3

[Paladin]

"Shall we begin?"

[Paladin]

Ah, never checked that. phpcs v3 requires php 5.4. But at least Travis can get the unit tests to run, which was better than I did.

[Paladin]

Finds an issue with: /home/travis/build/joomla/coding-standards/vendor/autoload.php

2 | ERROR | [ ] You must use "/**" style comments for a file comment | | (Joomla.Commenting.FileComment.WrongStyle) 3 | ERROR | [x] Comment must start with a capital letter; found "a" | | (Joomla.Commenting.SingleComment.LowerCaseStart)

I'd fix that if I could, but I don't have access to that file.

[mbabker]

LOL why are we linting third party files?

[Paladin]

I've no idea. Looks like maybe the command was to lint "joomla/coding-standards" and because vendor/autoload.php is in that tree it got picked up. Should be able to exclude vendor from the phpcs check, right?

Or, we could fix the two simple flags and let it stand?

[mbabker]

Yep. @photodude has done the bulk of the heavy lifting on whipping this repo into shape, so I'll defer to him on configurations and whatnot.

[Paladin]

All of the unit tests crashed...appears the class loader is looking in the wrong tree for the files. Wonder if that's a PHPUnitv4 thing or something deeper?

[photodude]

I'll see if I can take a look later today when I'm not on my phone.

[photodude]

Just a quick note, for the 3.x branch. "PHP_CodeSniffer 3.x+ requires PHP version 5.4.0 or greater." So we need to adjust Travis on the 3.x branch to remove php 5.3 from testing, and set the composer minimum to php5.4 The 3.x branch will be primarily for things like J4 so the change in php minimum for that branch isn't an issue since we have the 2.x branch for projects that need php5.3 support.

[Paladin]

OK, made the composer.json change. I'll leave the travis setup change for you?

[Paladin]

Apologies for the cranial flatulence. Forgot the travis file was a hidden file -- out of sight, out of mind. Removed php 5.3 from it.

[mbabker]

In composer.json you need an autoload declaration. Not sure how PHPCS itself handles class loading, but for PHPUnit you'll either need to manual include files or wire up an autoloader.

[photodude]

PHPCS broke composer use https://stackoverflow.com/questions/43707647/how-to-autoload-another-package-with-own-composer/43844960#43844960

https://github.com/squizlabs/PHP_CodeSniffer/commit/566a0cdf6cdb8fbc5a5a248cdf23f7d77a968de7

[mbabker]

:eyeroll:

[mbabker]

Well, the problem boils down to the fact that the file isn't getting loaded. So if PHPCS isn't handling loading the files right, I don't know what else to suggest.

[photodude]

The current suggestion is to use the solution listed in https://stackoverflow.com/questions/43707647/how-to-autoload-another-package-with-own-composer/43844960#43844960

[photodude]

or "you need to include PHPCS directly, (by) includ(e/ing) the autoload.php file"

[Paladin]

I'm at a loss. This works: ./vendor/bin/phpcs --standard=Joomla --ignore=Tests Joomla

so the classes load fine using the code that's in there. But this doesn't: ./vendor/bin/phpunit --filter Joomla vendor/squizlabs/php_codesniffer/tests/AllTests.php

which would tend to indicate that whatever phpcs is doing to screw up the autoloader is unscrewable by it, but is hosing phpunit.

So the question is do we want to go into phpcs and fix its autoloader issues, find another linter that's willing to play nice with others, or leave it be, without being able to run tests? I don't have any other ideas (well there was a whack-o idea to write a custom loader wrapper that first checked to see if phpcs was the application doing the loading and only use the phpcs autoloader when true, skipping it in favor of composer's autoloader if false) but that seems a bit squicky to me).

[photodude]

According to @gsherwood there is likely a namespace issue somewhere

He has asked for a new issue to be opened with a link to this version of the coding standard.

[gsherwood]

I'm at a loss. This works: ./vendor/bin/phpcs --standard=Joomla --ignore=Tests Joomla

so the classes load fine using the code that's in there. But this doesn't: ./vendor/bin/phpunit --filter Joomla vendor/squizlabs/php_codesniffer/tests/AllTests.php

This specific issue is different to the one I was commenting on. You'll need dev-master to fix this one, and it was reported and described here: https://github.com/squizlabs/PHP_CodeSniffer/issues/1564

Basically, the unit testing system wasn't setting up autoloading in the same way as normal phpcs and phpcbf runs. So if your normal runs work fine, you don't have any namespace issues - you have a PHPCS test suite issue.

[photodude]

Thanks @gsherwood

[gsherwood]

@mbabker @photodude Just another note: PHPCS cannot use the composer autoloader because it is essential that the PHPCS autoloader is able to attempt to load classes before any other autoloader has had a go.

This is because the fully qualified class names map to internal sniffs codes (like Joomla.Commenting.FileComment) so PHPCS has to know what class you are talking about when your ruleset references an internal sniff code. If the composer autoloader had already loaded sniff class in a custom coding standard, PHPCS is told to create a sniff of type Joomla.Commenting.FileComment and it has no idea how to do that.

The only way to be 100% sure PHPCS autoloading comes first is to have PHPCS kick off autoloading itself. But it still detects and uses the composer autoloader to load up any classes that it doesn't know about and that a custom standard may be using the composer autoloader for.

Hope that explains things.

[Paladin]

Thanks, @gsherwood . I'd seen that issue and for now I'll take your word for the necessity, but at some more appropriate place and time (like after sessions at a conference) I'd love to understand why reflection tools like getNamespaceName() wouldn't be useful for issues like this; seems at first blush the kind of problem they were created to solve.

Meanwhile, @photodude, I think there's an anomaly in the phpcs execution line in the travis.yml file, line 31, causing the failure of the phpcs run w/o phpunit. What's the purpose of the '*/' after '--standard=Joomla?' That's what is making the code sniffer check the third party autoloader code (or at least, when I remove those two chars it doesn't check it -- I got suspicious of that b/c it has all the hallmarks of an IDE's autocomplete function, closing the "comment" started with the last chars of the ignore pattern, and tweaking it does seem to fix the individual command, outside of the context of the if). Alternatively, dropping the end wildcard from the ignore pattern also seems to work. It's the combination of the two that causes the autoloader code to get sniffed. (If I understand the pattern-matching phpcs does properly, "--ignore=*/vendor/*" should be functionally equivalent to "--ignore=/vendor/" and the latter doesn't trigger the bug. Either of these two approaches seems in the limited scope I worked with to solve the issue:

` - if [[ $RUN_PHPCS == "yes" ]]; then ./vendor/bin/phpcs -sp --extensions=php --ignore=*/vendor/* --standard=Joomla .; fi

• if [[ $RUN_PHPCS == "yes" ]]; then ./vendor/bin/phpcs -sp --extensions=php --ignore=/vendor/ --standard=Joomla */ .; fi`

Have a look and see if you agree, and if you do, whether you want me to include that in this PR or if you want to do it yourself.

Edited to escape the asterisks for markdown's sake.

[Paladin]

And, @gsherwood, as a follow-up I can confirm that with phpcs dev-master I can actually get the unit tests to run, for the first time. Oh goodie, more work to do.

[Paladin]

OK, everything passes except the phpcs run, and that's not the fault of this code. I'll also defer to @photodude on how/if he wants to address the phpcs flags on the vendor autoloader file. I left a TODO note in the composer change commit so that I can remember to get it off dev-master as soon as phpcs v3.1 ships.

[gsherwood]

Thanks, @gsherwood . I'd seen that issue and for now I'll take your word for the necessity, but at some more appropriate place and time (like after sessions at a conference) I'd love to understand why reflection tools like getNamespaceName() wouldn't be useful for issues like this; seems at first blush the kind of problem they were created to solve.

The short answer is: I can't use reflection without having an object of that class. But I can't create an object of that class until I know what the fully qualified class name is. I know the file location of a sniff and I can include the file, but I have no idea what the FQCN is just based on the file location, so I can't create an object of that class in order to use reflection to get the namespace.

More info: Sniffs that include/extend other sniffs are a real problem. The included sniff is include_once()d before it normally would be, so PHPCS is sometimes unaware that it even happened when there is a composer autoloader loading up the file itself. Then when PHPCS gets to that file and tries to include_once() it again, nothing happens. Now it has no chance of even looking at the list of declared classes to figure out the FQCN.

The core problem is: Given just a file path, how can I determine what the FQCN is so I can include the file and create an object of that class? My solution has been to look at the list of declared classes/interfaces/traits before and after including the file, but this requires PHPCS to be the one including all sniff files. If there is a better solution (that is not tokenizing the file - too messy) then please get in touch with me as I'd love to discuss other solutions.

[photodude]

@Paladin I tested the removal of */ in the travis CMD lines as you suggested and then broke a comment to test if the sniffs are still looking at the correct files. My test did not error on the code style check as expected, so I'll have to look to see what's needed here.

[photodude]

Seems there may be an issue in the sniff checking for comments that start lowercased when they are in a comment block. Removal of the */ in the travis CMD lines passed with an expected code style failure in my second test.

I say we go ahead and make that change.

[Paladin]

OK that seems to have done it. I'm not seeing the lowercase error you had, because it's not in the base. But because it happened, we should create a test for it.

(later edit:) Heh, my bad. Before I shoot my mouth off I should probably check the manual. The description of when to start a comment line with u/c and when l/c is required is definitely code I wouldn't want to write. Catching some of the instances is do-able, but a sniffer with unreliable results is worse than no sniffer at all. (It's the "boy who cried wolf" issue: when a particular error result can be ignored, all error results are weakened.)

A sniff that detects whether the word starting the line and printed with an initial cap occurs anywhere in the project codebase (or PHP itself) as a method or a property and therefore should be l/c-ed? Or the converse, that the l/c word doesn't appear anywhere and therefore should be initial cap? Not me, thank you very much. I think that particular part of the standard needs to be ignored by the sniffer.

[photodude]

So the existing sniffs/ruleset.xml should be catching the comments starting with lowercase. The sniff that does the checking for single comments Joomla.Commenting.SingleComment.LowerCaseStart

Since the issue only occurred within a block comment, it's a different sniff that is the issue. (maybe we don't have a sniff for that)

in any case that issue should not hold back merging this PR

[photodude]

@mbabker @wilsonge What do you think about merging this?

[photodude]

thanks @mbabker

[photodude]

And thanks @Paladin for getting the ball rolling on the PHPCS 3.x version