Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exotic structure with Composer and GrumPHP in subfolder compared to GIT #592

Open
mattheo-geoffray opened this issue Jan 22, 2019 · 7 comments

Comments

@mattheo-geoffray
Copy link

commented Jan 22, 2019

Q A
Version 0.14.3
Bug? yes
New feature? no
Question? yes
Documentation? no
Related tickets /

Exotic project structure is wrong for GIT diff

Here is my project structure

/.git
/project
    |- src
        |- Namespace
            |- Vendor
    |- composer.json
    |- composer.lock
    |- grumphp.yml

My configuration
grumphp.yml

parameters:
  git_dir: ".."
  bin_dir: "./project/vendor/bin"
[...]

composer.json it is a simplified version of it. The framework behind is a Magento 2

{
    "require-dev": {
        "pdepend/pdepend": "2.5.2",
        "phpmd/phpmd": "^2.6",
        "phpro/grumphp": "^0.14",
        "phpunit/phpunit": "~6.5.0",
        "sebastian/phpcpd": "^3.0",
        "sensiolabs/security-checker": "^5.0",
        "slevomat/coding-standard": "~4.0",
        "squizlabs/php_codesniffer": "^3.3"
    },
    "autoload": {
        "psr-0": {
            "": [
                "src/"
            ]
        },
    },
    "extra": {
        "grumphp": {
            "config-default-path": "grumphp.yml"
        }
    }
}

Steps to reproduce:

cd project
composer install
# modifying a file in project/src/*
git commit -m "reproducing GrumPHP issue"

Result:
GrumPHP is not sniffing the file modified by GIT.

When I check the \GrumPHP\Console\Command\Git\PreCommitCommand::getCommittedFiles there is no file in it.
The \GrumPHP\Locator\ChangedFiles::locateFromRawDiffInput
=> I have the good GIT diff

project/src/*.php

The error is in \GrumPHP\Locator\ChangedFiles::parseFilesFromDiff
Where

if ($file->isDeletion() || !$this->filesystem->exists($fileObject->getPathname())) {
    continue;
}

!$this->filesystem->exists($fileObject->getPathname() => Return false

One simple test in \GrumPHP\Locator\ChangedFiles::parseFilesFromDiff

file_exist('project/src/*.php') => return false

file_exist('src/*.php') => return true

My guess is that the root for Composer is in project but the GIT root is above project, so the autoloader have the wrong path.

I don't think it is a GrumPHP bug it more probably an exotic structure maybe not supported by GrumPHP logic.

Do you have a solution for that?

Thank you!

@mattheo-geoffray

This comment has been minimized.

Copy link
Author

commented Jan 22, 2019

Little information we use that structure because it is a dockerized application so we can not have the Composer at the project root

@malc0mn

This comment has been minimized.

Copy link

commented Mar 5, 2019

I'm having the exact same issue. This project structure is not at all exotic, just a clean way to separate, for example, source from documentation and preventing your git repo to become a mess 😄

Whenever I make a change, it seems as though GrumPHP is doing it's thing, running all tasks, but you can make whatever coding error you want without any of the tasks detecting it.

I have also traced the problem and have an idea for a solution as well:

  1. When we look at the git hooks being installed, the pre-commit hook looks like this:
#!/bin/sh

#
# Run the hook command.
# Note: this will be replaced by the real command during copy.
#

# Fetch the GIT diff and format it as command input:
DIFF=$(git -c diff.mnemonicprefix=false --no-pager diff -r -p -m -M --full-index --no-color --staged | cat)

# Run GrumPHP
(cd "${HOOK_EXEC_PATH}" && printf "%s\n" "${DIFF}" | exec $(HOOK_COMMAND) '--skip-success-output')
  1. You see that a subfolder is handled perfectly because of the cd "${HOOK_EXEC_PATH}" in the very last line.
  2. The problem, however, is the git diff that is being generated, which will look like this:
diff --git a/project/src/path/to/my/BaseController.php b/project/src/path/to/my/BaseController.php
index c9bdffd945057f1202a27c150dc8512139eab3a0..ec20fbe03e4a01d40445716892f6162562a8dd73 100644
--- a/project/src/path/to/my/BaseController.php
+++ b/project/src/path/to/my/BaseController.php
@@ -33,7 +33,7 @@ abstract class BaseController extends Controller
             if ($log) {
                 $this->log($log, 'error');
             }
-            throw $this->createAccessDeniedException($message);
+throw $this->createAccessDeniedException($message);
         }
     }

The problem should be obvious instantly when looking at the paths: they start from the very root of the git repository.

Possible solutions:

  1. Given the fact that you know the HOOK_EXEC_PATH, you can easily strip this path from the git diff that you receive

  2. I'm not at all sure why you need the full diff? I did not dig in to the code deep enough, but there is such a thing as git status -s which can be executed inside the HOOK_EXEC_PATH (so the CD needs to come first) and it will give you a list of changed files relative to the dir you are in:

M src/path/to/my/BaseController.php
R  src/path/to/my/StatusController.php -> src/path/to/my/StatusController2.php
?? ../test.php
?? test.log

But that might not be enough for what GrumPHP does with that output? It will not only show staged stuff and uses colour coding to differentiate between staged and not staged so that's probably a no go

  1. Find a way to make git diff display relative paths. A way to do this would be to simply adjust the existing command you currently use to this: git -c diff.mnemonicprefix=false --no-pager diff -r -p -m -M --full-index --no-color --staged --src-prefix=a/../ --dst-prefix=b/../. Note the addition of the --src-prefix and --dst-prefix which would result in:
diff --git a/../project/src/path/to/my/file.php b/../project/src/path/to/my/file.php
index c9bdffd945057f1202a27c150dc8512139eab3a0..ec20fbe03e4a01d40445716892f6162562a8dd73 100644
--- a/../project/src/path/to/my/BaseController.php
+++ b/../project/src/path/to/my/BaseController.php
@@ -33,7 +33,7 @@ abstract class BaseController extends Controller
             if ($log) {
                 $this->log($log, 'error');
             }
-            throw $this->createAccessDeniedException($message);
+throw $this->createAccessDeniedException($message);
         }
     }

I think option 3 would be quite easy to implement, especially since you already have this prefix in the grumphp.yml file:

parameters:
    git_dir: ..
    bin_dir: vendor/bin

Handling the trailing slash is a breeze ($prefix = rtrim($gitDir, '/') . '/';).

@mattheo-geoffray

This comment has been minimized.

Copy link
Author

commented May 24, 2019

Hello!
Any update on that? 🙂

@open-it

This comment has been minimized.

Copy link

commented May 28, 2019

I have the same issue (for the same reason : docker conf outside the project) so an update would be great. 😺

@DylanGD

This comment has been minimized.

Copy link

commented May 29, 2019

Same issue here :( any news ?

@veewee

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

This specific case should be fixed in #643. However, there are some other exotic structures still not possible. (will be fixed in upcoming prs)

@mattheo-geoffray

This comment has been minimized.

Copy link
Author

commented May 31, 2019

@veewee Great news! Thanks for the fix! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.