Support PHP_CodeSniffer version 3.3.1

I too am interested in this as I want to use the php language server: https://github.com/felixfbecker/php-language-server which requires version 3.

Same here. I am interested in version 3.0.0 support of phpcs to be able to use the PHP Language Server for VS Code, aka PHP Intellisense by Felix Becker. As of now, the PHP Language Server crashes on the Drupal standard, so I am guessing we could fix this issue by upgrading towards phpcs 3.0.

It looks like we need to make a complete refactoring pass on sniffs to use namespaces: phpcs upgrade guide to 3.0.

I have started an upgrade to PHPCS 3. I am having an autoloading issue, probably due to a missing feature of the PHPCS autoloader (posted an issue on the issue tracker of phpcs' GitHub). But with workarounds, it looks like it works. There are still the Test folders to upgrade and then it should be done.

I will be glad to share my work if someone can help me sharing it, as I am kind of new on Drupal. :)

Thanks Dakwamine - the best thing to do would be to create a patch of what changes you have made and post it here - have a read through of the drupal patching guide: https://www.drupal.org/node/707484

Basically you just clone the module, do some changes, and git diff into a file to upload and then upload the file to an issue queue.

Thanks mikejw.

I have followed the guides you have mentioned to create a diff. Please let me know if I am doing a mistake somewhere. ;)

So for this patch, here are some notes:

1. The sniff part works.
2. Unit testing: I have not tested the unit testing part, as I am still inexperienced on this... It would be great if someone can check this out.
3. composer.json: "squizlabs/php_codesniffer": "~3.0.0RC4"
4. This update changes all sniff files; should we create a branch to not break the current active issues fixes?

Thanks Dakwamine - that is quite the patch!

As this doesn't introduce any new functionality per se might need to introduce new tests. Don't worry about other issues - that will all come out in the wash and can't be helped. I'll take a look at this and get back to you.

Actually, there is a PR that needs to go into that langserver before this is useful: https://github.com/felixfbecker/php-language-server/pull/308

Is there anything to do on our side to make progress on our matter?

Hi Dakwamine - apologies I haven't gotten around to this. Perhaps you could try and run the unit tests on your patch? Have a look through: https://www.drupal.org/docs/8/phpunit/running-phpunit-tests and the related pages.

-= DELETED =-

Tests do not pass when applying that patch to dev

$ ./vendor/bin/phpunit
PHP Fatal error: Class 'PHP_CodeSniffer\Tests\Standards\PHPUnit_Framework_TestCase' not found in ~/coder/coder_sniffer/DrupalPractice/Test/ProjectDetection/ProjectUnitTest.php on line 10

Fatal error: Class 'PHP_CodeSniffer\Tests\Standards\PHPUnit_Framework_TestCase' not found in ~/coder/coder_sniffer/DrupalPractice/Test/ProjectDetection/ProjectUnitTest.php on line 10

I got around the first error but now it looks like things changed a ton in https://github.com/squizlabs/PHP_CodeSniffer, almost every single test failed

./vendor/bin/phpunit
PHPUnit 5.7.20 by Sebastian Bergmann and contributors.

EEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE 65 / 76 ( 85%)
EEEEWWWWWEE 76 / 76 (100%)

Time: 131 ms, Memory: 6.00MB

There were 71 errors:

1) Drupal\Sniffs\Arrays\ArrayUnitTest::testSniff
Error: Class 'PHP_CodeSniffer\Config' not found

/Users/pwaters/coder/coder_sniffer/Drupal/Test/CoderSniffUnitTest.php:41

2) Drupal\Sniffs\Arrays\DisallowLongArraySyntaxUnitTest::testSniff
Error: Class 'PHP_CodeSniffer\Config' not found

/Users/pwaters/coder/coder_sniffer/Drupal/Test/CoderSniffUnitTest.php:41

3) Drupal\Sniffs\Classes\ClassCreateInstanceUnitTest::testSniff
Error: Class 'PHP_CodeSniffer\Config' not found

/Users/pwaters/coder/coder_sniffer/Drupal/Test/CoderSniffUnitTest.php:41

4) Drupal\Sniffs\Classes\ClassDeclarationUnitTest::testSniff
Error: Class 'PHP_CodeSniffer\Config' not found

FYI: Had to pin down the version of php-codesniffer to < 3.0.0 at Archlinux AUR until this is fixed.

Sorry, I just re-read the above comments and it looks like a fair attempt was already made. I'll dive into that now and maybe see how far off it is.

Ok, made quite a bit of progress on this. The only erroring tests are the "bad"/"good" tests. Needed to do quite a bit of rework due to:

• phpcs is manually autoloaded by default
• the base CoderSniffUnitTest class changed *a lot* in phpcs so I totally replaced the old one

I have posted my WIP patch which is based on the one above. Will continue on... soon :)

Ok, a bit more progress, only a few warnings left with respect to the projectdetection test as it is very much geared towards the old version of phpcs. I haven't actually tested that all the sniffs work as you would expect them too against a Drupal site / module etc.

And one more version as a couple of commits from when dakwamine first created his patch got missed inbetween.

Missed adding in the renamed array sniffs / tests.

Wouldn't it be better to open a pull request on github.com as per https://github.com/pfrenssen/coder#contributing?

Hmmm - I didn't even know that was a thing - I will do this now.

Here is a link to the PR: https://github.com/pfrenssen/coder/pull/7

As mentioned in the comment, not 100% sure what to do re the DrupalPractice project test since it is based around a class that no longer exists in phpcs 3.*

The tests are failing because of this: https://github.com/squizlabs/PHP_CodeSniffer/issues/1639
If we copy the rules from PHP CS' phpcs.xml.dist into Coder's phpcs.xml.dist the test will be able to run, but there are a lot of things to be fixed.
I wonder how I can contribute to your PR.

The tests run fine for me, except, for the DrupalPractice project ones which fail because essentially the old ones are based on something that doesn't exist in phpcs any more. I think I will be bold and just remove them all together and then update the phpcs.xml.dist. Stay tuned.

Be Bold! :)

Is it worth changing my composer to point at https://github.com/fenetikm/coder until this is merged?

I pulled a copy of https://github.com/fenetikm/coder to try to understand where this effort is and if I might be able to help.

One thing I did notice is that when phpcs runs a standard now, it does not seem to find classes in other namespaces - so to get DrupalPractice to run, I had to copy FunctionCall and FunctionDefinition from Semantics in Drupal to a new semantic directory in DrupalPractice and modify several use statements to update the use statement. It's not a perfect solution, but it works. I had to do the same with ClassFilesSniff in DrupalPractice\Project.php.

Maybe this is a php7 thing?

If we go to phpcs v3.2.1, Drupal fails unit tests for ArrayUnitTest, FileCommentUnitTest, and GoodUnitTest. But it's probably best to defer that work until after we can just get to 3.0...

With one change to update class name on line 26 of ProjectUnitTest.php, DrupalPractice all passes.

@mikejw would you be interested in a pull request or patch of some sort?

Note the related 7.x issue at #2953138: Support PHP_CodeSniffer version ^3.2.0. While work is being done here for ^3.0.0, it should probably also consider targeting ^3.2 since this is now the current version.

Any chance on finishing this in the current century?

Sorry, forgot about this. I'll get back on to it this week. I'll bump it to 3.2 too.

Changed title to reflect the new target version.

Ok, new patch. All tests pass for me. Version is no phpcs 3.2.3. Will post to github momentarily.

Pull request in github is here: https://github.com/pfrenssen/coder/pull/9

I note that the travis ci tests are failing when it come to running drupal_coder through phpcs because of some config issues... but looking at the current files they are quite a bit behind drupal coding standards - I wondered if that ever passed?

Either way, hopefully this can now get merged and then we can create another issue around updating the coding standards of the module.

Actually, there is an issue with the phpcs rules in the dist file, fixing.

Ok, fixed it all up. Fingers crossed.

New PR on github: https://github.com/pfrenssen/coder/pull/10

Ok, couple of little things to do - will follow up in the next few days.

Latest version - created another issue around fixing up the Project tests separately.

Over on https://github.com/pfrenssen/coder/pull/10 the maintainer has mentioned that reviews from others would be helpful - anyone keen to pitch in?

I've been using this patch for a few weeks with no issues, functionality appears to be still working as expected. Also it would be nice to get this merged in as old version causes no end of issues in our CI environments with dependency comparability.

Ok, i bucked up and even did a code review of this looooooong patch. It's pretty much just a lot of namespacing, so it's not really that complex.

A few very small issues I noticed, which I don't think are blockers. Overall awesome work on such a huge patch!

-    public function checkAlignment(PHP_CodeSniffer_File $phpcsFile, $stackPtr)
+    public function checkAlignment($phpcsFile, $stackPtr)

Removes the type suggestion and doesn't replace it with File

-     * @param string $filename The name of the file being tested.
+     * @param string                  $filename The name of the file being tested.
+     * @param \PHP_CodeSniffer\Config $config   The config data for the run.
      *
      * @return array
      */
-    public function getCliValues($filename)
+    public function setCliValues($filename, $config)
     {
-        return array();
+        return;
 
-    }//end getCliValues()
+    }//end setCliValues()

Function does nothing now? why even have a return.

+    protected function getTestFiles($testFileBase) {
         return array(__DIR__ . '/test.module');
     }

should the new $testFileBase parameter be used instead of __DIR__ now? The comments describe it as the base filepath. This repeats in a few other functions.

Note: Couldn't a lot of these comments just inherit the interfaces documentation? it seems like absolutely a ton of redundant documentation on implementations of PHP_CodeSniffer_Sniff

@smccabe - do you mind to create a patch as you have fixed? So we can test it and put it back on RTBC? :)

+1
I'm interested in this coming patch to be merged as I have other tools requiring PHP_CodeSniffer 3.

As described in How to install dev version within a Drupal install, I am presently not able to apply a turnaround solution which would have been to simply install Coder dev version and apply patch #40.

Also, this has become more important since Scrutinizer changed something to their system in july and tests everything with PHPCS 3 causing all our analyses to fail since then.

Reg. #45 - I believe #43 is reffering to the patch in https://github.com/pfrenssen/coder/pull/10. Hence back to RTBC.

Setting to "Needs work" since the comments in #43 have not been addressed yet.

Edit: oh now I'm confused. Are the comments in #43 not about the patch in #40?
Is that patch the same as that pull request 10?

I am agreed with @JvE regarding status on the current issue or postponed.

In case, if you guys believe most that the #43, also the patch(s) https://github.com/pfrenssen/coder/pull/10 works fine and smooth - then could someone please to take a look there on the(se) commits and confirm the review changes on the GitHub? So once it is merged there on GitHub, then we can go back here (to test and verify from here on drupal.org). So if everything is fine on both sides which means GitHub (and here), then we can change the status of the issue to RTBC/Fixed.

Since I see the owner of the repo on GitHub (https://github.com/pfrenssen/coder/pull/10) needs your reviews.

Reporting back for those interested - I have done a couple of updates and it now works with the latest version of code sniffer 3.3.1.

TBH I can't (today) be bothered in creating another patch - if you want the latest version just grab it from the pull request on github.

@truls1502 has kindly offered to review the patch in the github queue which is where the action currently is - if we can just get that over the line then we can start trying to tackle other things as per the other comments in additional issues.

Thank you a lot for fantastic work @mikejw!

I can at this moment confirm that the new commits on PR#10 - https://github.com/pfrenssen/coder/pull/10 works fine from my end, and I have also run phpunit with no problem.

However, we appreciate if someone can also approve the changes on GitHub.

I am now a collaborator on the github repo - just waiting to hear back on how the workflow of github repo to drupal repo works then I will get it merged.

waiting for PHP_CodeSniffer-3.+ support

While we wait for official support for PHP_CodeSniffer 3.x+ I've installed @mikejw's pull request and it's been working great. You can install this today if you're comfortable jacking with your global composer.json file:

Sample composer.json:

{
    "repositories": [
        {
            "type": "git",
            "url": "https://github.com/fenetikm/coder.git"
        }
    ],
    "require": {
        "squizlabs/php_codesniffer": "^3.3",
        "drupal/coder": "dev-phpcs-3-2863898"
   }
}

Then a simple composer global update and if you you need phpcs's include_paths to be modified: phpcs --config-set installed_paths ~/.composer/vendor/drupal/coder/coder_sniffer

Final patch is attached. Going to merge this on GH in a minute. I think from here, considering the size of this, please post any further problems unless they are showstoppers in another issue.

Working fine and smooth!

Thanks a lot and hat off to @mikejw!

I have added the parent issue #2996032: Drupal coder maintainer access and hopefully, this one can be fixed soon before a new release! :)

Next step might be checking and maybe it needs to update on https://www.drupal.org/node/1419988?

Thanks a lot to @mikejw for making this happen! I have merged this into 8.x-2.x and will tag a new release soon.

Great job. Thanks to @pfrenssen too :)

Would it be better to create 8.x-3.x branch which is related to PHP CodeSniffer 3? And then 8.x-2 is related to PHP CodeSniffer 2 etc?

So in this project, the version is easy to understand and know which version of PHPCS. What do you think?

Yes I am intending to tag this as 8.3.0 because it might cause conflicts with sites that are directly or indirectly locked on PHP_CodeSniffer 2.x for some reason. Sites can have a really complex dependency chain nowadays, and might have some dependency locked for various reasons.

Thanks for the suggestion!

Great. Looking forward to contributing to this module :)

Unfortunately this had the side effect of breaking d7 testing in drupalci, as we were relying on the executable existing at /vendor/squizlabs/php_codesniffer/scripts/phpcs, and it no longer does in the 3.x branch of phpcs.

So, in the future, something this drastic should probably be a Major api change (9.0.0). Its somewhat confusing because coder started on drupal.org, with 7.x and 8.x versions, then became a library, with 8.3.x versions, but the 8 no longer signifies "this is d8", its now just another major api number.

I've changed the coder requirement in drupalci to be ^8.2.12@stable instead of ^8.2@stable so that we dont upgrade past the 8.2.x branch.

I am getting this error which used to be resolved by installing codesniffer 2x. Now that we have support for 3x and 3x is required when installing coder, what are the steps to resolve this and install coder?

3:20 PM PHP_CodeSniffer
phpcs:
Fatal error: Interface 'PHP_CodeSniffer\Sniffs\Sniff' not found in/ .../.composer/vendor/drupal/coder/coder_sniffer/DrupalPractice/Sniffs/General/OptionsTSniff.php on line 22