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

Return commandline when project dir is same #678

Closed
wants to merge 1 commit into from

Conversation

rjwebdev
Copy link

@rjwebdev rjwebdev commented Sep 17, 2019

Q A
Branch master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Documented? no

With this PR, the problem I reported in issue #671 is fixed. Our project dir in the vagrant box is /vagrant and after running the command grumphp git:init, the generated git hooks contains the path without vagrant so I became something like /home./.composer/.... Now. the path in the git hooks is correctly generated.

Also credits to @rvanginneken for searching for a solution

Copy link
Contributor

@veewee veewee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @rjwebdev

Thanks for looking into this.

The line you changed was something I added to make the path relative to the project dir so that I could run GrumPHP both locally and in vagrant. So I am curious why it fails on your setup.

Can you give me some additional information about your setup?

  • Are you running every command in your vagrant box?
  • Where is GrumPHP located? (looks like it is globally installed because you mention /home/.composer
  • Are you also committing to git inside the vagrant box?
  • Can you provide me the guessed directories when running grumphp in verbose mode? (They are logged on top)

Maybe it makes more sense to try to shorten the command at another level instead.

$process->getCommandLine()
);
if (0 ===
strpos($process->getCommandLine(), $this->filesystem->ensureUnixPath($this->paths->getProjectDir()))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this always skip the block?
The getCommandLine() escapes the parameters with ' or ". so the project dir won't ever be the first char?

Copy link

@rvanginneken rvanginneken Sep 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure what you mean by the getCommandLine() escapes the parameters with ' or ".

Also i'm not sure what $process->getCommandLine() is supposed to return. I assumed it always was an absolute path, since the process of this function is making it relative.
That being said, the only valid way to make it relative, is when the project dir shares the same absolute path.

another quick edit: ignore what I said here before. Making it relative without checking checking the absolute path of the project dir, would create problems for our setup anyway.

@rvanginneken
Copy link

Hi @veewee,

I'll try to answer your questions, without having the project at hand.

  • All grumphp commands are triggered inside the vagrant box. git:init is called by a logged in user, hooks are executed through the vagrant hooks_preset.
  • GrumPHP is installed globally inside the box. So are any dev bundles we require to do our grumphp checks.
  • Commits are not done from the vagrant box. These are done from the host machine.
  • I can not access the guessed directories right now, perhaps @rjwebdev can add them tomorrow?

So why does it fail: inside our box, our user and project directory have the same name.
grumphp.yml is located in /vagrant: /vagrant/grumphp.yml
grumphp is installed globally for the vagrant user, meaning grumphp binary is located somewhere in /home/vagrant/.config/composer/..

So when you replace the project dir /vagrant with . in the grumphp binary path /home/vagrant/.config/composer/.. without checking if this is actually the start of the absolute path, you'll end up with /home/..config/composer/..

@veewee
Copy link
Contributor

veewee commented Sep 17, 2019

Thanks for your feedback. It makes sense.
I'll try to take a deeper look and run some tests soon.

@veewee veewee mentioned this pull request Sep 18, 2019
@veewee
Copy link
Contributor

veewee commented Sep 18, 2019

Hello @rjwebdev

Would the solution in #680 fix your situation?
(It still requires some tests, but it should cover your case)

@rvanginneken
Copy link

Hi @veewee,

It does not fix our issue:

Package operations: 0 installs, 2 updates, 0 removals
  - Removing phpro/grumphp (v0.16.0)
  - Installing phpro/grumphp (dev-improve-executable-locator b3678b4): Cloning b3678b4c17 from cache
cd /vagrant && grumphp git:init
Watch out! GrumPHP is sniffing your commits!
#...
printf "%s\n" "\${DIFF}" | 'exec' '/home./.config/composer/vendor/bin/grumphp' 'git:pre-commit' '--ansi' '--skip-success-output
#...

@veewee
Copy link
Contributor

veewee commented Sep 19, 2019

@rjwebdev,

Looks like you are pulling the wrong branch. It is developed at 'dev-improved-git-init-params'

@rvanginneken
Copy link

@veewee

Ah oops, yeah, that branch fixed it:

printf "%s\n" "\${DIFF}" | 'exec' '/home/vagrant/.config/composer/vendor/phpro/grumphp/bin/grumphp' 'git:pre-commit' '--ansi' '--skip-success-output'

@veewee
Copy link
Contributor

veewee commented Sep 19, 2019

Ok, thanks for testing!
I'll close this one in favour of that one :)

@veewee veewee closed this Sep 19, 2019
@rvanginneken
Copy link

@veewee allright. It is possible to push a quick release with that fix?
It would resolve that problem on a lot of our boxes :D

@veewee
Copy link
Contributor

veewee commented Sep 20, 2019

Hopefully somewhere next week. Stay tuned! :)

@veewee
Copy link
Contributor

veewee commented Sep 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants