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

[malc0mn]

Q	A
Branch	master
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Documented?	no
Fixed tickets	#592

This PR will make sure the paths generated by the git diff command in the commit-msg and the pre-commit hooks will be relative to where GrumPHP is being executed based on the grumphp.yml config file, see #592 for a full description of the problem.

[malc0mn]

I'm really not sure if this file needs to be updated as well. I did just to be sure, but have no way of testing it.

[malc0mn]

I'm really not sure if this file needs to be updated as well. I did just to be sure, but have no way of testing it.

[malc0mn]

Whoa, not at all sure what those failures are about in travis :/

[malc0mn]

I get the same errors on master without any change...

[veewee]

@malc0mn. Thanks for the PR.
If you change files in the project root directory, the paths won't be correct either.
This little hack in the git hooks is probably not enough. It also won't fix the run command.

I know some directory structures are hard to handle ATM. We are looking into adding e2e tests to cover as many paths as possible. We won't make any path changes in the meantime to make sure we don't break working structures.

The Travis error is probably releated to a new version of symfony/console

[malc0mn]

Right indeed, I knew it wouldn't cover all bases instantly but couldn't think of a good example right away.
Edit: What do you mean with "it also won't fix the run command"?

Why are you using the full git diff? Should the filenames of the files that were changed not be enough?

[veewee]

The run command uses \GrumPHP\Locator\RegisteredFiles which uses the git ls-files.
We are using the full diff since the diff inside the hook was added later. We wanted to reuse the logic inside \GrumPHP\Locator\ConfigurationFile for parsing the diff information. That locator uses the diff functionallity from Gitonomy, which uses the full diff.
We could switch the git command, but then again: we need e2e tests to make sure we don't break anything.

[malc0mn]

Been thinking about your example of changing files in the root dir and with this PR, that still will not be a problem given that the paths will now always be 'relative' (note the quotes) to the grumphp.yml file.

Changing (adding in this example) a file in the root dir will give you this:

diff --git a/../hello.php b/../hello.php
new file mode 100644
index 0000000000000000000000000000000000000000..7bf38ca41723f753057d7712a6c0d5f1161fbd17
--- /dev/null
+++ b/../hello.php
@@ -0,0 +1,3 @@
+<?php
+
+echo "hello"

So that will still work fine as GrumPHP will show:

$ git commit
GrumPHP detected a pre-commit command.
GrumPHP is sniffing your code!
Running task 1/4: Composer... ✔
Running task 2/4: PhpCsFixerV2... ✔
Running task 3/4: PhpStan... ✘
Running task 4/4: Phpunit... ✔
             ▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄▄
           ▄▄▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▌
         ▐▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▄
        ▐▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▌
       ▐▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▌
  ▄███▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▌
 █▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▌
 ▐█▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▌
   ▀█▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▌
     ▀▀▓▓▓▓▓▓▓▓▓▓▓▓█▀▀▀▀▀▀▀▀▀▀▀▀▀▀████████████▄
      ▄███████                       ██████████
     ███████▀  ▀▀▀▀▀▄      ▄▀▀▀▀▀     █████ ▀
      ▐████      ▐██        ▐██        ████▌
      ████▌                            ███
       ▌██▌           ▄▄ ▄▄           ▐███
        ███       ▄▄▄▄▄▄▄▄▄▄▄▄       ▐███
         ██▄ ▐███████████████████████████
        █▀███████████▀     ▀▀███████████
          ██████████▄███████▄███████████
         ▐█████████████████████████████
          █████████████████████████████
           ██ █████████████████████▐██▀
            ▀ ▐███████████████████▌ ▐▀
                ████▀████████▀▐███
                 ▀█▌  ▐█████  ██▌
                        ██▀   ▐▀

       ██████████████████████████████████
       █░░░░░░▀█▀░░░░░░▀█░░░░░░▀█▀░░░░░▀█
       █░░▐█▌░░█░░░██░░░█░░██░░░█░░░██░░█
       █░░▐█▌░░█░░░██░░░█░░██░░░█░░░██░░█
       █░░▐█▌░░█░░░██░░░█░░░░░░▄█░░▄▄▄▄▄█
       █░░▐█▌░░█░░░██░░░█░░░░████░░░░░░░█
       █░░░█░░░█▄░░░░░░▄█░░░░████▄░░░░░▄█
       ██████████████████████████████████

------ ------------------------------------------------------- 
  Line   hello.php                                              
 ------ ------------------------------------------------------- 
  4      Syntax error, unexpected EOF, expecting ';' on line 4  
 ------ ------------------------------------------------------- 

 [ERROR] Found 1 error
To skip commit checks, add -n or --no-verify flag to commit command

[malc0mn]

The run command uses \GrumPHP\Locator\RegisteredFiles which uses the git ls-files.

Given that you do a CD to the folder where grumphp.yml resides, I assume that git ls-files is run from that location as well. git ls-files always generates relative paths to where it is executed, so there is no issue with that particular command.

[veewee]

Ah yes I see, but then there is a mismatch in how pre-commit and run works: pre-commit passes all files in lower folders, but run does only pass those in the cwd.
Also when running the ./vendor/bin/grumphp git:pre-commit directly without std-in, the diff will be loaded inside \GrumPHP\Locator\ChangedFiles, which doesn't contain the prefix logic.

So maybe it's better to introduce a new grumphp parameter, something like: grumphp_operates_in_this_folder.
Next we can change the locators to make sure only files which grumphp operates on are being located.
In that step we can also make the files relative to that newly introduced path.
That way, we can avoid using the prefixes and change how locating works inside the PHP code.

What do you think about that approach?

[malc0mn]

So just to clarify by going back to the example in #592:

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

This would mean that grumphp would only operate on files inside the /project folder, right?
That would indeed seem like the correct approach; it is also the logical approach since you could go wild and do this:

/.git
/micro-service-one
    |- src
        |- Namespace
            |- Vendor
    |- composer.json
    |- composer.lock
    |- grumphp.yml
/micro-service-two
    |- src
        |- Namespace
            |- Vendor
    |- composer.json
    |- composer.lock
    |- grumphp.yml

which is IMHO very wrong, but still GrumPHP could purr along merrily if we leave the contents of the hook scripts aside (that would be another matter entirely).

TL;DR
Yes, that seems like the correct approach!

[veewee]

The case with multiple project directories is quite hard since the hook files we provide in this package only expect one grumphp file. There will be a lot of edge cases like: what to do when files in multiple microservices change etc.

Thanks for your feedback on the issue!

[malc0mn]

Believe me, I wouldn't even think of supporting it since as I said it is IMHO very wrong to do such a thing.
But with the approach you propose here, it would be possible.

So what do we do with this PR? Close it?

[Thomasvc1]

Made file objects created from a git diff rely on the absolute path of the file:
(rename to .patch for easy apply)
grumphp.txt