search

magento / marketplace-eqp

Magento 1.x Coding Standard
http://docs.magento.com/marketplace/user_guide/Resources/pdf/Extension_Quality_Program_Overview.pdf
MIT License
224 stars 68 forks source link

Add compatibility with phpcs 3.* #70

Closed paweltatarczuk closed 7 years ago

paweltatarczuk commented 7 years ago
lenaorobei commented 7 years ago

@trawiasty thank you so much for your contribution!

I have a few comments to this pull request:

  1. When I configure PHP_CodeSniffer by adding path to Magento 2 instance and run tests vendor/bin/phpunit vendor/squizlabs/php_codesniffer/tests/AllTests.php I get following error: Fatal error: Class 'PHP_CodeSniffer' not found in MEQP/Utils/Helper.php on line 76 It should be replaced with PHP_CodeSniffer\Config class.

  2. Lines MEQP2/Sniffs/SQL/CoreTablesModificationSniff.php:184 and MEQP2/Sniffs/SQL/CoreTablesModificationSniff.php:187 will also lead to an error.

paweltatarczuk commented 7 years ago

I've updated my fork with an amended commit - it should fix both issues reported in @lenaorobei review.

I hope that amending a commit does not break antything here, if I should've sent a new commit with fixes I can reset master to originally reviewed commit and push changes with a new one. I'm not familiar with review workflow at github.

lenaorobei commented 7 years ago

@trawiasty there is one small issue in MEQP2/Sniffs/SQL/CoreTablesModificationSniff.php:185 $tokenizer = new Tokenizer($content, $sourceFile->config, $eolChar); $sourceFile is undefined variable here. When I change to $tokenizer = new Tokenizer($content, null, $eolChar); it works as expected.

About amending commit I don't see any issue here, so I think you don't need any additional actions only this small fix in CoreTablesModificationSniff.php.

paweltatarczuk commented 7 years ago

Oh I see, I meant to use $sourceFile from process() method, stupid mistake. Do you think it's better to pass null there or should I fix missing $sourceFile instead? If we pass null then some logic in PHP_CodeSniffer\Tokenizers\Tokenizer constructor is skipped. I'm not sure if that's desired behavior.

lenaorobei commented 7 years ago

We don't need additional configuration to parse requested files and find core tables names. So, if we pass null it works as expected.

lenaorobei commented 7 years ago

@trawiasty, great! Thank you!

paweltatarczuk commented 7 years ago

@lenaorobei Thank you for the help here!