wunderio / code-quality Goto Github PK

I think unit tests are quite fast and they could be run on git commit. The script should find find the module path for the files that are being commited so it would not trigger the whole testcase for site.

You might not notice it because composer installs older version of code-quality if requirements are met.

I checked that on my system I have iconv.so but it's just not loaded.

One way to overcome this is enable it on shell:

php -d extension=iconv.so /bin/composer require wunderio/code-quality:1.0.5 --dev

Not ideal but could we use this info somehow to automate this?

Some files are invalid on purpose (for testing). How to deal with them?

lando grum --tasks yaml_lint
GrumPHP is sniffing your code!
Running task 1/1: YamlLintTask... ✘
[ERROR] Indentation problem in "./web/core/tests/Drupal/Tests/Core/Asset/library_test_files/invalid_file.libraries.yml" at line 3 (near "  key2: value2").

Use case: wunderio/drupal-project#122 (comment)

cc: @guncha25

ECS provides cognitive complexity checks that should be included in this package.

I'm talking about the output after git commit:

Running task 1/8: EcsTask... ✔ 
Running task 2/8: PhpcsTask... ✘

It can be confusing for developer who has never used phpcs. Instead couldn't it say what it's really about eg:

Running task 1/8: EcsTask... ✔ 
Running task 2/8: Drupal Coding Standards ... ✘

See, I didn't actually know what I could name EcsTask :)

May be this is issue for GrumPHP to add some annotation or special variable in the class? I guess we could also rename our classes but then it would be something like:

Running task 2/8: DrupalCodingStandards ... ✘

What do you think?

Ragnar got this error in his project.

PHP Fatal error:  Trait 'Symfony\Bridge\PhpUnit\Legacy\PolyfillAssertTrait' not found in /app/web/sites/simpletest/Assert.php on line 68
Fatal error: Trait 'Symfony\Bridge\PhpUnit\Legacy\PolyfillAssertTrait' not found in /app/web/sites/simpletest/Assert.php on line 68

Needed to add:

lando composer require symfony/phpunit-bridge

Is that requiremet for all projects? Could be it's because the project didn't have core-dev package installed (which should not be requirement).

While commiting, I got this error:

$ git commit -a
GrumPHP detected a pre-commit command.
GrumPHP is sniffing your code!
Running task 1/7: EcsTask... ✔
Running task 2/7: PhpcsTask... ✘
Aborted ...
ERROR: PHP_CodeSniffer requires the tokenizer, xmlwriter and SimpleXML extensions to be enabled. Please enable xmlwriter and SimpleXML.

To skip commit checks, add -n or --no-verify flag to commit command

Apparently I have to install php-tokenizer package on my host machine to be able to use this.

GrumpPHP actually supports psalm but not sure yet how much work needs to be done for implementing it Code Quality.

@ragnarkurmwunder said:

The Problem

The striving higher quality for less price to be paid.
My practice is psalm figures out a lot of bugs even before any test would catch them.
So effort to catch bugs ratio compared with writing tests, I'd say is 1:100.

The Solution

Install Psalm as

• part of the workflow – composer require-dev vimeo/psalm
• Initialize psalm – psalm --init. By this Psalm figures out the highest level of errorLevel, and operates on that level.
• Configure Psalm to scan proper folders, namely web/modules/custom, and web/themes/custom.
• Integrate into our workflow. It could be make scan, part of commit scanners, or part in deployment flow. Or any combination of those. I personally use the first option massively to make the feedback loop as tight as possible.
• Commit psalm.xml.
• As the codebase gets cleaner, errorLevel can be reduced.

Note, Psalm operates by tracking types. Therefore, adding types to all member variables, function and method arguments, and return data is important. In addition I suggest to use strict types. Php <8 have a bit limited type functionality, but doable. In some places need to add additional @var, @param, @return docblocks.

Psalm documentation - https://psalm.dev/

Psalm is also able to set a baseline based on existing code, to prevent new bugs from leaking in.
https://psalm.dev/docs/running_psalm/dealing_with_code_issues/#using-a-baseline-file
Otherwise one might find it too intimidating to fix the vast amount items that Psalm may complain about.

These were removed here https://github.com/wunderio/code-quality/pull/71/files because we were not able to upgrade ECS on D9.

Readme should contain instructions how to customize standards.

Ref: https://wunder.slack.com/archives/C5B9YGQSE/p1597407241011400

How can I define working ignore_patterns for folders which are subfolders of ones which are defined in run_on? This doesn’t seem to work:

    ecs:
      run_on: ['web/modules/custom', 'web/themes/custom', 'web/sites']
      ignore_patterns:
        - '/web/sites/default/files/'

cc: @guncha25

Currently running grumphp from globally installed path gives error because of relative path to default standards for phpcs and ecs.

Ascii parameters should be cleaned up to give better developer experience.

We have this line under installation:

cp vendor/wunderio/code-quality/config/phpstan.neon ./phpstan.neon

But that local phpstan.neon is never used. Let's change it so it is.

This was the full install command for me

 composer require wunderio/code-quality:2.0.3 symplify/easy-coding-standard:8.3.48 wunderio/ecs-drupal:1.0.0 composer/xdebug-handler:1.4.6 composer/composer:2.0.13  --dev

on https://packagist.org/packages/composer/composer#2.1.8 you can see that anything below 2.1.8 has security issues.

I think the offending package is this https://github.com/wunderio/ecs-drupal/blob/master/composer.json where it asks for quite old package ECS 8.3 https://packagist.org/packages/symplify/easy-coding-standard#8.3.48

Platform: llinux + lando
Issue:

When I enable xdebug using the command lando xdebug debug, the commit checks fail, and a commit cannot be done. If debug is stopped during the checks, the commit might get accepted (then creating possible issues later in the CI phase).

The output looks like this:

GrumPHP is sniffing your code!
Running tasks with priority 0!
==============================
Running task 1/6: php_compatibility... 
Running task 2/6: check_file_permissions... 
Running task 3/6: php_check_syntax... 
Running task 4/6: phpcs... 
Running task 5/6: yaml_lint... 
Running task 6/6: json_lint... 
Running task 1/6: php_compatibility... ✘
Running task 2/6: check_file_permissions... 
Running task 3/6: php_check_syntax... ✘
Running task 4/6: phpcs... ✘
Running task 5/6: yaml_lint... ✘
Running task 6/6: json_lint... ✘
php_compatibility
=================
Starting the process failed
php_check_syntax
================
Starting the process failed
phpcs
=====
Starting the process failed
yaml_lint
=========
Starting the process failed
json_lint
=========
Starting the process failed
To skip commit checks, add -n or --no-verify flag to commit command

As I see PhpCompatibilityTask just missing logic for ignore_patterns

While I understand the need for static security scanners, it can be quite confusing to see those warnings that block your commit. There's no good documentation which also stated in the package issue queue FloeDesignTechnologies/phpcs-security-audit#44 You have to google and dig into the code to understand what is going on. All in all you get smarter if you're up for it but it can easily lead to frustration or skipping validation the default action.

Some of the examples and answers that I've found

38 | WARNING | The use of function fnmatch() is discouraged 
(Drupal.Functions.DiscouragedFunctions.Discouraged)

On php.net it says "Warning: For now, this function is not available on non-POSIX compliant systems except Windows." As I understand it does not concern us as we are not using non-POSIX systems. https://en.wikipedia.org/wiki/POSIX#POSIX-certified macOS os certififed POSIX. Linux is in the https://en.wikipedia.org/wiki/POSIX#Mostly_POSIX-compliant but obviously it works. So good to know but why is it blocking the commit, one might ask

33 | WARNING | Function array_map() that supports callback detected
|         | (PHPCS_SecurityAudit.BadFunctions.CallbackFunctions.WarnCallbackFunctions)

From code I found link and all the other callback functions in PHP:

// From RIPS and http://stackoverflow.com/questions/3115559/exploitable-php-functions
public static function getCallbackFunctions() {
    return array(
        'ob_start', 'array_diff_uassoc', 'array_diff_ukey', 'array_filter', 'array_intersect_uassoc', 'array_intersect_ukey', 'array_map', 'array_reduce',
        'array_udiff_assoc', 'array_udiff_uassoc', 'array_udiff', 'array_uintersect_assoc', 'array_uintersect_uassoc', 'array_uintersect', 'array_walk_recursive',
        'array_walk', 'assert_options', 'uasort', 'uksort', 'usort', 'preg_replace_callback', 'spl_autoload_register', 'iterator_apply', 'call_user_func',
        'call_user_func_array', 'register_shutdown_function', 'register_tick_function', 'set_error_handler', 'set_exception_handler', 'session_set_save_handler',
        'sqlite_create_aggregate', 'sqlite_create_function'
    );
}

So the link goes to http://stackoverflow.com/questions/3115559/exploitable-php-functions and there it says

These functions accept a string parameter which could be used to call a function of the attacker's choice. Depending on the function the attacker may or may not have the ability to pass a parameter. In that case an Information Disclosure function like phpinfo() could be used.

So all in all you have to be careful and there's another comment on the issue:

This means that programmers should take extra care when using these functions, but if they where all banned then you wouldn't be able to get much done.

At the moment we are banning them if commit does not go through.

Would it be possible to allow warning to go through? I think in the configuration it's possible to change the rule from warning to error and vice versa. It could mean lot of custom configuration though.

Or should we disable the security thing by default?

Should we create documentation in the security package wiki for example?

As a developer I want my contributions to be automatically checked for issues, so that I'm sure that I'm not introducing regressions

I wonder how I've missed this. Maybe because there's more PHP than module/inc/install files in Drupal 8.

This is placeholder task for creating task for the about.yml project we are working on. As we need to enforce it, then adding it to this projects makes sense. This would also block any deploys when it would be missing from Silta.

To reproduce: composer require a contributed module (for example password_policy that has .sh test files and push to CircleCI, validation fails .

GrumPHP is sniffing your code!
Running task 1/8: EcsTask... ✔
Running task 2/8: PhpcsTask... ✔
Running task 3/8: PhpCompatibilityTask... ✔
Running task 4/8: PhpCheckSyntaxTask... ✔
Running task 5/8: CheckFilePermissionsTask... ✘
Aborted ...
Fix file permissions for the following shell scripts:
File: ./web/modules/contrib/password_policy/tests/drupal_ti/before/before_script.sh
Current permissions:  644
Expected permissions: 755
-
To skip commit checks, add -n or --no-verify flag to commit command
Exited with code exit status 1 
CircleCI received exit code 1 

Problematic function at https://github.com/wunderio/code-quality/blob/master/bin/check_perms#L63-L84 - I'd suggest a fix to ignoring the permission checking for files in ./web/modules/contrib

Our project (https://github.com/wunderio/client-fi-thl-innokyla) started failing phpcs validation as a result of drupal/coder update from 8.3.6 to 8.3.7 (it started including file extension that used to be excluded). Looking at the diff (https://git.drupalcode.org/project/coder/compare/8.x-3.6...8.x-3.7) I'm assuming the culprit is https://git.drupalcode.org/project/coder/commit/69b071b610803080e8ac1ca6757ed4e64524171c for issue https://www.drupal.org/node/3074176 where the default file extensions were removed from the rulesets. Not sure if there's anything we can do about it here other than maybe document the change? Anyway adding this to our local phpcs.xml seems to return proper file extensions:

<!-- Set default file extensions -->
<arg name="extensions" value="php,module,inc,install,test,profile,theme,css,info,txt,md,yml"/>

So the issue is that you can't install Code Quality with Drupal 8 and Composer 2. The dependencies just don't match.

You can try it with this small project I just created https://github.com/hkirsman/drupal8-composer2-code-quality See readme. Maybe you can make it work?

I've created 1-x branch from the latest head. From there I downgraded GrumPHP to 0.18.1 because there they added Composer 2 support, it works with our Code Quality and still doesn't have the conflicting dependencies.

PR ready to be tested

Since all projects use yaml and json files we need to lint them also.

I installed it fresh with this command:

php -d extension=iconv.so $(which composer) require wunderio/code-quality   --dev

After commiting, I get:

➜   git commit
GrumPHP detected a pre-commit command.
GrumPHP is sniffing your code!✔
Running task 2/8: PhpcsTask... ✔✔
Running task 4/8: PhpCheckSyntaxTask... ✔✔✘
Aborted ...
PHPStan crashed in the previous run probably because of excessive memory consumption.
It consumed around 55 MB of memory.


To avoid this issue, allow to use more memory with the --memory-limit option.
PHP Deprecated:  Array and string offset access syntax with curly braces is deprecated in /home/hannes/Sites/download_d8/vendor/drush/drush/includes/sitealias.inc on line 175
Deprecated: Array and string offset access syntax with curly braces is deprecated in /home/hannes/Sites/download_d8/vendor/drush/drush/includes/sitealias.inc on line 175
PHP Deprecated:  Array and string offset access syntax with curly braces is deprecated in /home/hannes/Sites/download_d8/vendor/drush/drush/includes/sitealias.inc on line 176
Deprecated: Array and string offset access syntax with curly braces is deprecated in /home/hannes/Sites/download_d8/vendor/drush/drush/includes/sitealias.inc on line 176
PHP Deprecated:  Array and string offset access syntax with curly braces is deprecated in /home/hannes/Sites/download_d8/vendor/drush/drush/includes/command.inc on line 860
Deprecated: Array and string offset access syntax with curly braces is deprecated in /home/hannes/Sites/download_d8/vendor/drush/drush/includes/command.inc on line 860
PHP Deprecated:  Array and string offset access syntax with curly braces is deprecated in /home/hannes/Sites/download_d8/vendor/drush/drush/includes/command.inc on line 862
Deprecated: Array and string offset access syntax with curly braces is deprecated in /home/hannes/Sites/download_d8/vendor/drush/drush/includes/command.inc on line 862
PHP Deprecated:  Array and string offset access syntax with curly braces is deprecated in /home/hannes/Sites/download_d8/vendor/drush/drush/includes/command.inc on line 873
Deprecated: Array and string offset access syntax with curly braces is deprecated in /home/hannes/Sites/download_d8/vendor/drush/drush/includes/command.inc on line 873

In File.php line 208:
                                                         
  Failed to open stream hoa://Library/Regex/Grammar.pp.  
                                                         

analyse [--paths-file PATHS-FILE] [-c|--configuration CONFIGURATION] [-l|--level LEVEL] [--no-progress] [--debug] [-a|--autoload-file AUTOLOAD-FILE] [--error-format ERROR-FORMAT] [--memory-limit MEMORY-LIMIT] [--xdebug] [--] [<paths>...]
To skip commit checks, add -n or --no-verify flag to commit command

When remove composer.lock and vendor, then install with specific (previous) version, it works:

php -d extension=iconv.so $(which composer) require wunderio/code-quality:1.0.5   --dev

➜   git commit
GrumPHP detected a pre-commit command.
GrumPHP is sniffing your code!✔
Running task 2/8: PhpcsTask... ✔✔
Running task 4/8: PhpCheckSyntaxTask... ✔✔✘
Aborted ...
------ -------------------------------------------------------------- 
  Line   src/EventSubscriber/ConfigEventSubscriber.php                 
 ------ -------------------------------------------------------------- 
         Internal error: Call to a member function yes() on bool       
         Run PHPStan with --debug option and post the stack trace to:  
         https://github.com/phpstan/phpstan/issues/new                 
 ------ -------------------------------------------------------------- 

I assume we missed this because we didn't install on clean project (without Code Quality previously installed)

It could go aftre:

You can run the checks manually with: ./vendor/bin/grumphp run

The example command would be

grumphp run --tasks=phpcs

As a developer I want my contributions to be automatically checked for issues, so that I'm sure that I'm not introducing regressions

Drupal 7 and 8 has different defaults. We could provide default configuration template for each of them.

We still have old projects and wondering how to support these. One way is using Docker as we're already using Lando so my first idea is to create Docker image that has PHP and other dependencies installed, this can then be used in Lando.

This could be also solution for fixing any dependencies that host needs that where discussed in wunderio/drupal-project#88

Subj.

Something like this:
./vendor/bin/phpcpd --suffix .php --min-lines 3 --min-tokens 50 --fuzzy src/

Use https://www.tomasvotruba.cz/blog/2018/02/05/how-to-run-symfony-processes-asynchronously/

I have this code:

( isset( $_POST['display_wpstp_post_type'] ) ) ? array_map( 'sanitize_text_field', $_POST['display_wpstp_post_type'] ) : array();

This works fine with sanitize_text_field, but I would prefer to use a custom function such as this:

function sanitize_unslash( $value ) {
    $value = sanitize_text_field( wp_unslash( $value ) );
    return $value;
}

And pass that function to array_map. However, doing so still presents errors in PHPCS.

$_POST data not unslashed before sanitization. Use wp_unslash() or similarphpcs
Detected usage of a non-sanitized input variable: $_POST['display_wpstp_post_type']phpcs

Would it be possible for this to be supported in the future? Or is there a better way to do what I am trying to do?

It is hard to maintain and update package if every extension is separate package. They should all be included in one package.

The default settings seem to need adjustment, 60 seconds timeout is not enough as you can see.

lando grum --tasks phpcs
GrumPHP is sniffing your code!
Running task 1/1: PhpcsTask... 
In Process.php line 1330:

  The process "'/app/vendor/bin/phpcs' '--standard=vendor/wunderio/code-quality/config/phpcs.xml,vendor/wunderio/code-quality/config/phpcs-security.xml' '--report=full' '--report-width=120' '--parallel=20' '--ignore=/vendor/,/node_modules/,/core/,/libra  
  ries/' '--report-json' '.'" exceeded the timeout of 60 seconds.  

I think the correct permission should be 644 or is there any reason it has executable permission?

All php related scanners should add .theme to their list because it's the Drupal 8 theme php file.

Current run functionality is not processing files correctly.

One way to fix this could be custom tasks that would handle this better.

Got the following warning after lando start in https://github.com/wunderio/drupal-project: The configuration file could not be saved. Give me some permissions!. Upstream issue: phpro/grumphp#674

Deivids Briedis discovered that deploy is complaining about issues in javascript files but running grumphp locally there are no errors.

It seems that the issue is that we've not added js as allowed extension for phpcs https://github.com/wunderio/code-quality/blob/master/src/Task/tasks.yml#L126

Should we change that?

PHPCS passes with dockblock @param missing backslashes. See screenshot:

This is the extensions list Drupal core is using: <arg name="extensions" value="inc,install,module,php,profile,test,theme,yml"/>.

Therefore, profile is missing from default extensions as well. Don't know if test should be included.

Re: #23

Looks like there are some issues when trying to remove the package. Removed it from Composer, but got this when tried to commit afterwards:

Git: .git/hooks/pre-commit: line 15: path/to/vendor/bin/grumphp: No such file or directory

I added code-quality to my Drupal 7 project but it fails when commiting:

git commit                                                         
GrumPHP detected a pre-commit command.
GrumPHP is sniffing your code!
Running task 1/8: EcsTask... ✘
Aborted ...
PHP Warning:  Use of undefined constant ICONV_IMPL - assumed 'ICONV_IMPL' (this will throw an Error in a future version of PHP) in /home/hannes/Sites/tekla/campus/vendor/nette/utils/src/Utils/Strings.php on line 154
PHP Fatal error:  Uncaught Error: Call to undefined function Nette\Utils\iconv() in /home/hannes/Sites/tekla/campus/vendor/nette/utils/src/Utils/Strings.php:168
Stack trace:
#0 /home/hannes/Sites/tekla/campus/vendor/nette/utils/src/Utils/Strings.php(180): Nette\Utils\Strings::toAscii('/home/hannes/Si...')
#1 /home/hannes/Sites/tekla/campus/vendor/symplify/easy-coding-standard/packages/ChangedFilesDetector/src/Cache/Simple/FilesystemCacheFactory.php(22): Nette\Utils\Strings::webalize('/home/hannes/Si...')
#2 /tmp/easy_coding_standard/ContainerZdGlkCq/getFilesystemCacheService.php(9): Symplify\EasyCodingStandard\ChangedFilesDetector\Cache\Simple\FilesystemCacheFactory->create()
#3 /tmp/easy_coding_standard/ContainerZdGlkCq/HttpKernelSymplify_EasyCodingStandard_HttpKernel_EasyCodingStandardKernelProdd90fcfcbe4265705e72551ee048dbb9063446Container.php(152): require('/tmp/easy_codin...')
#4 /tmp/easy_coding_standard/ContainerZdGlkCq/getSimpleCacheAdapterService.php(9): ContainerZdGlkCq\HttpKernelSymplify_EasyCodingStandard_HttpKerne in /home/hannes/Sites/tekla/campus/vendor/nette/utils/src/Utils/Strings.php on line 168
To skip commit checks, add -n or --no-verify flag to commit command

My system doesn't seem to have php module iconv enabled and configured. But then again it works fine for Drupal 8. When checking deeper, then I found that Drupal 8 has symfony/polyfill-iconv package installed. This solved my issue:

 composer require symfony/polyfill-iconv:^1 

It's missing --dev intentionally because Drupal 8 web/core/composer.json file has the dependency also in require section as:

"symfony/polyfill-iconv": "^1.0",

Originally GrumPHP showed a line you could copy/paste into shell and it would execute phpcbf that fixes some things like indents, arrays etc. Now it's gone. Also, it should be triggered if there's this line shown:
PHPCBF CAN FIX THE 14 MARKED SNIFF VIOLATIONS AUTOMATICALLY, not when error is detected because not all errors can be fixed.