ddev / ddev-contrib

Contrib space for DDEV services, tools, snippets, and approaches.
Apache License 2.0
162 stars 162 forks source link

Add drupal pre-commit phpcs recipe running inside DDEV #197

Closed bserem closed 2 years ago

bserem commented 2 years ago

What is new:

Provide a git-hook recipe that executes PHPCS from inside DDEV during git commit.

This allows for the host machine to not have PHP and/or PHPCS installed at all, everything is happening inside the container, although the actual commit can happen either inside or outside of the container.

How this PR Solves The Problem:

Provides the files and documentation needed.

Manual Testing Instructions:

Follow the instruction in the README, stage a file that would fail in PHPCS and try to commit either from the host machine or from inside the web container in DDEV.

Sample photo (php errors are intentional): image

Related Issue Link(s):

https://github.com/drud/ddev/issues/3518 talks about the DDEV verbose line (last line in the above screenshot)

bserem commented 2 years ago

@rfay this recipe allows phpcs to run without php installed on the host machine. I made that clearer on the readme. At the moment I do not have the time to change this to a pre-push hook, but I really like the idea and will try to do this soon.

@tyler36 I do not have any experience with other projects, this is what Drupal recommends for custom/contrib code. Since this is a recipe, anybody can freely change it to their needs. I am positive it can be altered as one desires.

The whole idea behind it is to remove the necessity for PHP on the host machine, so that our developers only need to care about their code.

PS: A follow-up for commit-msg is coming within the day

rfay commented 2 years ago

This also requires phpcs to be required where you want to use it, either host or in ddev web container, right?

tyler36 commented 2 years ago

@bserem Yes.

But if you changed the command line

recipes/git-hooks/pre-commit-drupal-phpcs/pre-commit.php

- $phpcs_cmd = 'cd /var/www/html && phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme ' . $file;
+ $phpcs_cmd = 'cd /var/www/html && phpcs ' . $file;
<?xml version="1.0" encoding="UTF-8"?>
<ruleset name="my_drupal_module">
    <description>PHP CodeSniffer configuration for a Drupal module.</description>

    <arg name="extensions" value="php,inc,module,install,info,test,profile,theme,css,js"/>
    <arg name="report" value="full"/>
    <arg value="p"/>
    <file>.</file>

    <!-- Exclude third party libraries. -->
    <exclude-pattern>./vendor</exclude-pattern>

    <rule ref="Drupal" />

</ruleset>

This would do exactly the same thing. However, it decouples the solution from specific Drupal-only config, to a extenadable pre-commit PHPCS reciepe.

By moving to a config based solution, PHPStorm / VScode extentions can also use config file.

bserem commented 2 years ago

@rfay I improved the docs to add the dependencies.

@tyler36 I am also ignoring drupal generated yml files in the start of the script, and if I don't do that then the hook takes a long time to execute. In the end the xml file will ignore them, but it will loop through all of them because of the way the hook is structured (to check only the files to be committed).

Switching to a phpcs.xml file is possible like you demonstrated, and it would need the script to be adapted like the following, but it would take a lot of time to run because the ignore doesn't happen on php level.

$return_back = '../../';
// Building an array of ignored files and folders.
$file_path = __DIR__ . '/' . $return_back . '.phpcs_ignore';
$ignore = [];
$ignore[] = CONFIG_ROOT;
if (file_exists($file_path)) {
  $file = fopen($file_path, "r");
  if ($file) {
    while (!feof($file)) {
      $ignore[] = fgets($file);
    }
  }
}

$exit_code = 0;
$files = [];
$return = 0;

// Determine whether this is the first commit or not. If it is, set $against to
// the hash of a magical, empty commit to compare to.
exec('git rev-parse --verify HEAD 2> /dev/null', $files, $return);
$against = ($return == 0) ? 'HEAD' : '4b825dc642cb6eb9a060e54bf8d69288fbee4904';

// Identify changed files.
exec("git diff-index --cached --name-only $against", $files);

print "\nPrecommit PHPCS\n\n";

// The number of ignored items (including CONFIG_ROOT).
$ignored_items_count = count($ignore);
echo "\033[0;31mYou have " . ($ignored_items_count - 1) . " item(s) in your ignore list!\n\033[0m";

foreach ($files as $file) {

  if (file_exists($file) && !is_dir($file)) {

    // Check files to ignore.
    if ($ignored_items_count > 0) {
      foreach ($ignore as $key => $value) {
        // Display a message for ignored files.
        if (substr($file, 0, strlen(trim($value))) == trim($value) && trim($value) != '') {
          echo "\033[0;31mIgnored in .phpcs_ignore file: $file\n\033[0m";
          continue 2;
        }
      }
    }

    echo "\033[0;32mChecking file {$file}\033[0m\n\n";

    $ext = pathinfo($file, PATHINFO_EXTENSION);

    $phpcs_output = [];

    $file = escapeshellarg($file);
    // Add extra path to get warranty for run drush.
    $dir = str_replace('\'', '', dirname($file));
    $extra = '';
    if ($dir !== '.') {
      $extra = $dir . '/';
    }

    // Perform PHP syntax check (lint).
    $return = 0;
    $lint_cmd = "php -l {$file}";
    $lint_output = [];
    exec($lint_cmd, $lint_output, $return);
    if ($return !== 0) {
      // Format error messages and set exit code.
      $exit_code = 1;
    }

    // Perform phpcs test.
    $return = 0;
    $phpcs_cmd = 'cd /var/www/html && phpcs ' . $file;
    exec($phpcs_cmd, $phpcs_output, $return);
    if ($return !== 0) {
      // Format error messages and set exit code.
      echo implode("\n", $phpcs_output), "\n";
      $exit_code = 1;
    }
  }
}

exit($exit_code);

I added an entry to the documentation towards your code and suggestions, so that people interested can find it easily.

rfay commented 2 years ago

Thanks so much, this will be a nice contribution. I'm on vacation for a couple more weeks, so may be some time before I get to review it, but if somebody else manually tests/reviews I'll pull it.

bserem commented 2 years ago

Enjoy the vacation!

bserem commented 2 years ago

@tyler36 after reading a bit about the phpcs.xml file and giving it a spin for a week to make sure I got things right, I updated the PR and the script per your suggestions. Now all configuration happens inside the phpcs.xml file so the script is more readable and does not need to be altered to be used to another project.

Please let me know what you thing about it in the current state and do not hesitate to suggest things.

Side bonus: the pre-commit script and the project phpcs configuration are now the same.

tyler36 commented 2 years ago

Great job!