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

PHP beautifier does not use global php-cs-fixer #390

Closed
jmenges opened this issue Jun 8, 2015 · 11 comments
Closed

PHP beautifier does not use global php-cs-fixer #390

jmenges opened this issue Jun 8, 2015 · 11 comments

Comments

@jmenges
Copy link

jmenges commented Jun 8, 2015

Hello there,
thanks first of all for your package.

I have a problem when using it, to reformat php code.
Atom is running on a Win7 maschine, with a global installation of php-cs-fixer.
It was installed according to this section in on their github page

Globally (Composer)
To install PHP-CS-Fixer, install Composer and issue the following command:
$ ./composer.phar global require fabpot/php-cs-fixer
Then, make sure you have ~/.composer/vendor/bin in your PATH, and you're good to go:
export PATH="$PATH:$HOME/.composer/vendor/bin"

If i set the executable path in your package, it still gets called with php.

2015-06-08T16:39:23.237Z - debug: [C:\Users.atom\packages\atom-beautify\src\beautifiers\beautifier.coffee] php-cs-fixer indent_size=4, indent_char= , indent_with_tabs=false, cs_fixer_path=php-cs-fixer, fixers=, level=, indent_style=space, end_of_line=lf, insert_final_newline=true, trim_trailing_whitespace=true, tab_width=4 2015-06-08T16:39:23.239Z - debug: [C:\Users.atom\packages\atom-beautify\src\beautifiers\beautifier.coffee] tempFile temp null path=C:\Users\AppData\Local\Temp\temp11558-716-1ndwvai, fd=0 2015-06-08T16:39:23.242Z - debug: [C:\Users.atom\packages\atom-beautify\src\beautifiers\beautifier.coffee] spawn php 0=php-cs-fixer, 1=fix, 2=C:\Users\AppData\Local\Temp\temp11558-716-1ndwvai 2015-06-08T16:39:23.286Z - debug: [C:\Users.atom\packages\atom-beautify\src\beautifiers\beautifier.coffee] spawn done 1 Could not open input file: php-cs-fixer

It does however work, if i set the full path. E.g:
C:\Users\AppData\Roaming\Composer\vendor\fabpot\php-cs-fixer\php-cs-fixer

Regards

@Glavin001
Copy link
Owner

Atom is running on a Win7 maschine, with a global installation of php-cs-fixer.

and

export PATH="$PATH:$HOME/.composer/vendor/bin"

The command there is for Linux / Mac and will not work on Windows. Windows does not handle export PATH=..., did it work when you tried it? I would assume Windows would give you some form of error message.

Note: I would recommend creating an issue for the PHP-CS-Fixer team over at https://github.com/FriendsOfPHP/PHP-CS-Fixer for Windows support. Their documentation is not clear for Windows users. I was able to get everything installed and working for Atom Beautify on AppVeyor's Windows CI with

atom-beautify/appveyor.yml

Lines 92 to 101 in 78b6d55

# PHP
- cinst php -y
- ps: "ls \"C:\\tools\\php\""
- "SET PATH=C:\\tools\\php;%PATH%"
- where php
# PHP-CS-Fixer
- cinst curl -y # Use cURL to download file from URL
- curl http://get.sensiolabs.org/php-cs-fixer.phar -o php-cs-fixer
- "SET PATH=%cd%;%PATH%" # Add current working directory to PATH
- where php-cs-fixer

The export PATH=... is Linux form of Window's "SET PATH=....
So you could try something like:

"SET PATH=PATH_TO_COMPOSER_BIN;%PATH%"

From http://stackoverflow.com/a/9542689/2578205 and in your case, it could likely be that $HOME is the same as %HOMEDRIVE%%HomePath% in Windows, such that:

"SET PATH=%HOMEDRIVE%%HomePath%\\.composer\\vendor\\bin;%PATH%"

Notice above that I replaced / with \\ for Windows. Yet another little different between Windows and the rest.

I am not a Windows users, nor do I recommend Windows for software development. Thus use the above at your own risk and you will have to do any debugging on your own. You can see https://github.com/Glavin001/atom-beautify/blob/master/appveyor.yml#L43 for how I have installed and setup Windows on AppVeyor for Atom Beautify Windows support.


If i set the executable path in your package, it still gets called with php.

and

Could not open input file: php-cs-fixer

and

cs_fixer_path=php-cs-fixer

You did not set the executable path. You set it to php-cs-fixer which is not a path, it's the executable name. The reason why Atom Beautify even takes this path is because Windows is an oddball and does not handle resolving the path from using simply the name, and spawning the process from within Node.js ( see discussion at #288 ). This works well now transparently (globally) with Mac and Linux. Therefore, for Windows, you are able to optionally give it the resolved absolute path such that Node.js does not have to search for (and usually fail) to find the program's path.

It does however work, if i set the full path. E.g:
C:\Users\AppData\Roaming\Composer\vendor\fabpot\php-cs-fixer\php-cs-fixer

This is correct. This is configured correctly, and it now looks like Atom Beautify is working correctly as expected.

Unfortunately, on Windows and with PHP-CS-Fixer, the global does not always get recognized properly. Windows + PHP has been causing a lot of problems. I will be releasing some fixes for Windows with the issue #375. Please follow that issue to see Windows updates.

I hope that with the improvements I have made for #375 that this may actually work for you, globally and automatically, like it does for Linux and Mac users. Thanks for your patience.

@Glavin001 Glavin001 added this to the v0.28.0 milestone Jun 8, 2015
@Glavin001 Glavin001 self-assigned this Jun 8, 2015
@jmenges
Copy link
Author

jmenges commented Jun 8, 2015

Oh, my bad. I was just referring to the general concept.
I modified the path using the advanced system settings (enviroment variables). I was also able to execute it directly using the commandline. It being php-cs-fixer.

Unfortunatly i am limited to using windows, since its the default for me at the office. At home i am also using a linux based system.

Anyway thanks for your effort and i'll check it at work tomorrow. If it doesn't work i might look at writing a fix, if i find the time.

@Glavin001
Copy link
Owner

Not a problem. Atom Beautify should properly use the PATH environment variables now on Windows as well ( #375 was just published ). Any executables you need should be installed and then configured in your PATH to work. If that does not work, then we have a problem -- although this is unlikely. I think you should be good from now on. 👍

@jmenges
Copy link
Author

jmenges commented Jun 10, 2015

I just updated the winows machine and unfortunatly the problem persists. Whats causing it is that php-cs-fixer is not called directly but as an argument to php. E.g:

2015-06-10T12:49:57.402Z - debug: [C:\Users\<username>\.atom\packages\atom-beautify\src\beautifiers\beautifier.coffee] exeName, args: php 0=php-cs-fixer, 1=fix, 2=undefined, 3=undefined, 4=C:\Users\<username>\AppData\Local\Temp\temp115510-2844-zf89oe

I can execute it directly from command line like this:

C:\Users\<username>>php-cs-fixer
PHP CS Fixer version 1.8.1 by Fabien Potencier

Usage:
  command [options] [arguments]
...

I tried fixing it myself within php-cs-fixer.coffee but was not successful thus far, since i am not that familiar with coffeescript & nodejs.

@Glavin001
Copy link
Owner

2015-06-10T12:49:57.402Z - debug: [C:\Users\<username>\.atom\packages\atom-beautify\src\beautifiers\beautifier.coffee] exeName, args: php 0=php-cs-fixer, 1=fix, 2=undefined, 3=undefined, 4=C:\Users\<username>\AppData\Local\Temp\temp115510-2844-zf89oe

It shows php-cs-fixer instead of C:\Users\AppData\Roaming\Composer\vendor\fabpot\php-cs-fixer\php-cs-fixer. Please change that back and it should work again as expected.

Whats causing it is that php-cs-fixer is not called directly but as an argument to php.

Windows requires the php command with php-cs-fixer as argument, while Mac/Linux does not. See #269 (comment) and other related issues for more details. We have been unable to reproduce how the Windows shell executes php-cs-fixer without being run as an argument of php. If you can figure that out, please feel free to submit a pull request.

@jmenges
Copy link
Author

jmenges commented Jun 10, 2015

Well i have successfully executed the beautify with the unix version of @run.
I'll do some more testing on monday and submit the pull request.

@Glavin001
Copy link
Owner

I would prefer to get rid of the php command with php-cs-fixer as argument, for Windows.

/cc @guillempascual could you give us some insight as to why you found that php was required? Ideally we should be able to remove it.

@Glavin001
Copy link
Owner

By using node-which I believe I am able to get rid of the php command running php-cs-fixer as an argument. which will find the php-cs-fixer program by looking through the PATH environment variable listed directories. This will resolve php-cs-fixer to an absolute path to the executable and that will be used to run the command. This dramatically cleans up the PHP-CS-Fixer beautifier code and specs, too! Doing some Windows tests now.

Update: I was able to get the absolute path to php-cs-fixer and then used spawn to run it, however it still errored on Windows (worked on Mac). spawn C:\\tools\php\php-cs-fixer failed on Windows.
I am going to revert back to using php to call php-cs-fixer... This is unfortunate. I am not really sure what is going on in Windows that is causing such a hassle.

Glavin001 added a commit that referenced this issue Jun 12, 2015
By using [node-which](https://github.com/isaacs/node-which)
I believe I am able to get rid of the `php` command running
`php-cs-fixer` as an argument.
`which` will find the `php-cs-fixer` program by looking through
the `PATH` environment variable listed directories.
This will resolve `php-cs-fixer` to an absolute path to the executable
and that will be used to run the command.
This dramatically cleans up the `PHP-CS-Fixer beautifier` code and specs, too!

Doing some Windows tests now.
Glavin001 added a commit that referenced this issue Jun 12, 2015
@Glavin001
Copy link
Owner

Published an improvement to PHP-CS-Fixer beautifier for Windows to v0.28.4 release. Hopefully that helps. It still uses php with php-cs-fixer as an argument. It resolves php-cs-fixer to an absolute path before passing to php and that should mitigate some issues where php-cs-fixer is not found.

@Glavin001
Copy link
Owner

Published a Windows patch to v0.28.5

@Glavin001
Copy link
Owner

I really want to focus on improving the installation experience for users. I have created a new Issue, #1687, to target this problem. Please provide your feedback! Thanks in advance.

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

No branches or pull requests

2 participants