Windows usage returns "No input File specified"

Apollon77 commented 6 years ago

Hi,

I use you great library in one of my projects to bring a php visualization framework in aNodeJS project. On Linux and MacOS everything works great. But on Windows I only get "No input file specified".

You can see that roughly in my tests on Appveyor: https://ci.appveyor.com/project/Apollon77/iobroker-smartvisu/build/test-16/job/upy6x1pk3i3yik6t#L335 I currently only use a "phpinfo.php" as test example. (Same code test runs on Linux: https://travis-ci.org/Apollon77/ioBroker.smartvisu/jobs/326288550#L977 ... works). Any idea why?

The code is the easy part without sessions/ws. You can see the code here: https://github.com/Apollon77/ioBroker.smartvisu/blob/master/main.js#L76

What I could offer is to add some basic testing like the onyl from my adapter to your repo so that you can enhance these tests then and have a coverage for Linux, macOS and Windows.

Apollon77 commented 6 years ago

I also have a second test on appveyor for Windows where the same code returns this: https://ci.appveyor.com/project/Apollon77/iobroker-smartvisu/build/test-17/job/qfhle5k3xpkyrid1#L335

paragi commented 6 years ago

Hi, Thank you for your kind offer. I would appreciate your contribution!

I have not really tested on windows. It's a thing that should be done! I'm sure testing and adaption to windows and Mac i needed. However I don't have the time, machines and sufficient experience to do it my self.

How would you like to proceed?

From the log you send, I would be curious as to whether php-cgi.exe is in the path that node is running in? (test: c:>php-cgi -v )

I have hereby noted that the error message is not that helpful :)

Best regards Simon

PS. smartVisu look interesting. do you have more info in English?

Apollon77 commented 6 years ago

Hi, I will look into it this evening and provide a simple test like mine for travis-ci (Linux, MacOS) and appveyor (Windows). The really good thing on these CI systems (free for public Github projects) is that you do not need all the systems, but can see and test on changes.

MacOS works great! Only Wiu´ndows have the told problem.

I install php 7.1 on windows, and added your requested "php-cgi -v" in a more current test run: https://ci.appveyor.com/project/Apollon77/iobroker-smartvisu/build/test-22/job/22j1owsntjvuvkti#L14

SmartVisu: Sorry dont have infos in english. I'm also more one of the developer of iobroker.net which has several visualization ways and we want to add smartvisu as option. So I prepare a generic ioBroker plugin to allow that integration of the php process :-) ioBroker itself is based on nodejs

Apollon77 commented 6 years ago

So, you already got one PR to add testing at all (only the simplest thing, but a basic for you to add more :-)

Additionally I will create two PRs more as next step: 1.) The minimal windows-compat-Fix ... very easy reason as you will see :-) 2.) A friend of mine from ioBroker went over the file too in search for the windows compat and and did a lot of refactoring, changed some core to be more "state of the art" and also fixed a socket bug. YOu need to decide if you want to accept that PR or take things from it. In general it's a big step forward, but also many changes ...

Apollon77 commented 6 years ago

see PRs #9, #10 and/or #11 :-) Have fun

paragi commented 6 years ago

Thanks a lot! I will look into it all, ASAP.

Apollon77 commented 6 years ago

Just that we not run into big merge problems :-)) I think it is best when you start with the "add testing PR" (#9) and if you accept that I can check the other PRs for conflicts. Then you should check the windows topic (#10 or #11) and after that the "relative require" PR(#12) In that order it should be less conflicts

Apollon77 commented 6 years ago

What do you think ... when we could get a new version on npm ;-))

steffanhalv commented 6 years ago

Sorry, I did merge #11 by a mistake into master. I didnt thougth I had the permission. I hopefully didnt broke something ^^

Apollon77 commented 6 years ago

checked it, anything great ... if @paragi wanted to have "the full changes" :-) #11 also included #9 (this was "my fault"). To be honest I personally would like to see that version of the fix. Else he needs to revert and I need to send the PRs again

paragi commented 6 years ago

Hi Ingo

I'm sorry I haven't had much time to review all your commits. I will have a look at it all Monday. Nice that Steffan could help you out!

At a quick overview it look to me as you have made the following changes:

Introduces test (I haven't really looked into to that yet, but its a much needed feature :)
Added ability to configure options through the express interface
Added a default to "php-cgi.exe" on windows platform
Use the path module to handle platform specific path manipulation

Is that all the changes to the actual code?

It all looks good, by the way :)

Your outstanding issue with current directory for scripts; I will look into the practises of Apache and emulate....

Do you have sufficient access to make commits on your own? Or do you need something else?

I'm very happy to have you on this project. Thanks for your contributions.

Best regards Simon

Den 13-01-2018 kl. 19:59 skrev Ingo Fischer:

checked it, anything great ... if @paragi https://github.com/paragi wanted to have "the full changes" :-) #11 https://github.com/paragi/sphp/pull/11 also included #9 https://github.com/paragi/sphp/pull/9 (this was "my fault"). To be honest I personally would like to see that version of the fix. Else he needs to revert and I need to send the PRs again

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/paragi/sphp/issues/8#issuecomment-357457996, or mute the thread https://github.com/notifications/unsubscribe-auth/AEIhldRXgBxyO633HSysvUjN7YMWDcrgks5tKPz3gaJpZM4RWKYu.

-- Simon Rigét Kløvermarksvej 17 Annisse 3200 Helsinge Tlf. 4096 8217

Apollon77 commented 6 years ago

Hi,

At a quick overview it look to me as you have made the following changes:

Introduces test (I haven't really looked into to that yet, but its a much needed feature :)

Added ability to configure options through the express interface

Added a default to "php-cgi.exe" on windows platform

Use the path module to handle platform specific path manipulation

Is that all the changes to the actual code?

plus:

make sure php-version is requested not on "require" time (because php-executable could not be changed, so failed on windows or when not in PATH at any time)
a socket issue was fixed too
and I changed the directory to the scriptfile directory (as it is in other cases) to allow also relative php-requires. Normally the working directory is set to the scriptfile-directory.

It all looks good, by the way :) Your outstanding issue with current directory for scripts; I will look into the practises of Apache and emulate....

see above :-)

Do you have sufficient access to make commits on your own? Or do you need something else? I'm very happy to have you on this project. Thanks for your contributions.

For me it's fine to provide Pull-Requests ... because you need to decide if/what you accept :-)

Apollon77 commented 6 years ago

PS: Just as note, there is one logline (sphp.js Line 195) still in that version. You should comment that out :-)

paragi commented 6 years ago

Hi Ingo

I intend to go with forward your changes. I appreciate your contribution.

I don't fully understand YAML As I haven't used it so far. But it looks good :) I will accept it and look into it as needed. I will focus on getting this branch ready for publishing on npm. Since there are new features, is constitutes a minor version upgrade. Also an update of the documentation is needed. I will add to your test script, along the way. Great!

I had some unpublished fixes locally, so I will probably have to revert and re-enter your contributions to sphp.js Formatting changes, take up a lot of realestate in the changelog :) I hope I catched all your editions :) Any unmentioned changes I should be aware of?

Sphp.express options Using an options array as parameter to sphp.express is a good idea, as it conform to current practise. sphp is however not just an express middleware, it's also used as stand alone. f.ex. Running an initialization script.

I Suggest the following changes:

Options to sphp.express should include all configuration options.
PHP version and accessibility check, moved to the sphp.exec method to cover stand-alone apps.
Fail if php engine is unavailable. Do you concur?

Current/working directory That is the correct handling. Thanks.

Questions: Windows compliance: It's a great that you introduced a uniform handling with the path module. As I understand it, executables in windows, defaults to adding the .exe extension. Is there any other reason to include the os check at line 107 in sphp.js?

if (/^win/.test(process.platform)) { sphp.cgiEngine = 'php-cgi.exe';

(It seems to work with sphp.cgiEngine = 'php-cgi' on windows?)

Socket issue I was unable to locate the socket issue that was fixed too. Could you be more specific?

Best regards, Simon

Den 14-01-2018 kl. 21:30 skrev Ingo Fischer:

Hi,
At a quick overview it look to me as you have made the following
changes:

  * Introduces test (I haven't really looked into to that yet, but
    its a much needed feature :)
  * Added ability to configure options through the express interface
  * Added a default to "php-cgi.exe" on windows platform
  * Use the path module to handle platform specific path manipulation

Is that all the changes to the actual code?
plus:

make sure php-version is requested not on "require" time (because php-executable could not be changed, so failed on windows or when not in PATH at any time)

a socket issue was fixed too

and I changed the directory to the scriptfile directory (as it is in other cases) to allow also relative php-requires. Normally the working directory is set to the scriptfile-directory.

It all looks good, by the way :) Your outstanding issue with current directory for scripts; I will look into the practises of Apache and emulate....

see above :-)
Do you have sufficient access to make commits on your own? Or do
you need something else? I'm very happy to have you on this
project. Thanks for your contributions.
For me it's fine to provide Pull-Requests ... because you need to decide if/what you accept :-)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/paragi/sphp/issues/8#issuecomment-357539843, or mute the thread https://github.com/notifications/unsubscribe-auth/AEIhlSpqfnQqYuslyq0fI6CISRNHgOcTks5tKmPPgaJpZM4RWKYu.

Apollon77 commented 6 years ago

I don't fully understand YAML As I haven't used it so far. But it looks good :) I will accept it and look into it as needed. I will focus on getting this branch ready for publishing on npm. Since there are new features, is constitutes a minor version upgrade. Also an update of the documentation is needed. I will add to your test script, along the way. Great!

Normally that YAML is relatively streight forward. And the reason why it looks that complex with all the "If" and such is that travis-ci do not support php on macos and also appveyor needs to install php. But normally you do not change on it and simply change your tests :-)

Beside adding to that one test script you can also create additional scripts in the test directory and they will all be executed. maybe easier to test WS stuff in a second testfile to still have an overview. In the tests you can use all nodejs stuff in general and also libs (add in devDependencies to the package.json).

Aditionally I have good experience with "code climate" as service. Also free for open source stuff and provides you code checks and also can monitor test coverage when executing the tests :-) If interested tell me and I can provide the needed (small) changes to start that.

I had some unpublished fixes locally, so I will probably have to revert and re-enter your contributions to sphp.js Formatting changes, take up a lot of realestate in the changelog :) I hope I catched all your editions :) Any unmentioned changes I should be aware of?

I think we covered all in the last posts ... but such a manual merge can be hard. Send me the infos and I can also check after you incorporated anything If I see missed things before you publish. And the good is: enable testing befiore and you have (for now very basic) proof that you not destroyed it :-)

I Suggest the following changes:

Options to sphp.express should include all configuration options.

PHP version and accessibility check, moved to the sphp.exec method to cover stand-alone apps.

Fail if php engine is unavailable. Do you concur?

Sounds reasonable.

Questions: Windows compliance: It's a great that you introduced a uniform handling with the path module. As I understand it, executables in windows, defaults to adding the .exe extension. Is there any other reason to include the os check at line 107 in sphp.js? if (/^win/.test(process.platform)) { sphp.cgiEngine = 'php-cgi.exe'; (It seems to work with sphp.cgiEngine = 'php-cgi' on windows?)

To be honest this change was a "to make sure" change ... normally windows add missing ".exe", but I also had problems on Appveyor that he uses the path at all ... so you can leve it in or try to remove. TAppveyor-tests will not tell you a break here because it sets a custom Path using the options and so overwrites he default.

Socket issue I was unable to locate the socket issue that was fixed too. Could you be more specific?

My friend from ioBroker that did this respective change made the diff in "websocket" where I refactored the mtch vs Regex test thing. Also he checks in some places that a object property is really a own one like the dded check like: if (extReq.headers.hasOwnProperty(key)) {

In general I found some smaller thigs:

the code to find the file was taken out of the loop in exec function
in websocked some refactoring with the Regex stuff
in maintain also smaller additions and refactoring stuff
and more such smaller things :-(

Sorry that all of this brings in these many changes with spaces and such :-(

But as said I offer to also check after your "merge" on my side which things may got missed

paragi commented 6 years ago

Hi Ingo

The merge is finished. I'm sorry it took a while to do it. I had to rewrite some old stuff (some of the code dates back to when nodejs was alpha.) to make your changes compatible to non/pre express usages.

Would you please check that your additions are intact and that the code works in your setups?

(It's still not working right with Wordpress)

What is the deal with the logger option (for sphp.express)? Wouldn't it be better and easer to just hi-jack console.log if you need a different logger like winston etc?

I don't have a windows setup with Apache. (We don't use windows much) if you do, Could you please check for me, if windows path separators are used in the $_SERVER super globals SCRIPT_NAME, PHP_SELF and SCRIPT_FILENAME? I expect that it does in DOCUMENT_ROOT.

Could you please create a test case utilising a web-sockets? And any other test you find prudent.

Documentation will need updating as well. But its a lower priority right now.

Apollon77 commented 6 years ago

I will try to check this evening or on weekend latest.

Apollon77 commented 6 years ago

looks good in general ... I have taken it now in my project and will let all tests run ... will come back with the results :-)

Apollon77 commented 6 years ago

Hm ... there is an issue. testing went red partially ... https://travis-ci.org/Apollon77/sphp/jobs/333467598#L787

Reason is a timing/async issue. setOptions Method has a async part (the "php -v" check), but is returned synchronously (and in the test file also unneeded twice). But you made the php version number a requirement to handle requests. So your application flow continues to allow serving stuff but php-version is not yet there, ans so a pot. fast first request (as in the test) is "declined".

So in my eyes two options: 1.) also process requests when php version is not yet existing ... should only result is a seond "php is not found" error 2.) make sure in "sphp.express" that the method only returns after the php version check has been done

But else looks good for now

Apollon77 commented 6 years ago

And once more (also detected by testing got red :-))

sphp.js L24: var moduleVersion = 'Snappy PHP ' + require('.' + path.sep + 'package.json').version;

"." is not working because relavtive paths are bad! use __dirname (https://nodejs.org/docs/latest/api/modules.html#modules_dirname) instead of the "." and this problem should be gone too

paragi commented 6 years ago

Great! Uploaded new version with the following fixes:

PHP version check disabled. (for now) will consider implication of inconsistency to scripts. But expect that few will notice
Relative path to package.json changed to full path
Websocket error reading session cookie fixed.

Apollon77 commented 6 years ago

PHP version check disabled. (for now) will consider implication of inconsistency to scripts. But expect that few will notice Bit in older versions the check was also done async and so nothing will change for them, or ?!

tests are trunning, but looks good. I saw some minor things and will prepare an PR with them later

Apollon77 commented 6 years ago

PHP version check disabled. (for now) will consider implication of inconsistency to scripts. But expect that few will notice Bit in older versions the check was also done async and so nothing will change for them, or ?!

tests are trunning, but looks good. I saw some minor things and will prepare an PR with them later

paragi commented 6 years ago

Thanks!

Had to push a version that throws an error if PHP engine is missing. Otherwise the errors thrown are very hard comprehend :)

chai looks nice. But I'm not familiar with it.

If you have time, I would appropriate test cases for:

setOptions throwing an error. something like: it('Missing PHP engine', (done) => { expect(function() { sphp.setOptions({cgiEngine: 'Invalid_PHP_engine'}) }).to.throw(); done('Should have failed'); });

But working! :)

websocket test

I will then add test cases for sessions.

You should have permissions to commit to master, if you want to.

Apollon77 commented 6 years ago

Have no idea for websockets till now ... will try this evening :-)

Thx for the permissions ...

paragi commented 6 years ago

Hi Ingo

Do you think this issue is solved? I would like to publish our work on NPM

Best regard Simon

Apollon77 commented 6 years ago

Hey, the windows issue is solved for sure :-) So we are currently "beyond" the issue :-) Happy when you publish it. testing is green and works

For testing I started in a branch in my fork for now and only came to the "exception" test: https://github.com/Apollon77/sphp/blob/update1/test/testexpress.js#L56 ... already works :-)

But I don't had time to test with websockets for now because never used them so far :-(

Apollon77 commented 6 years ago

thx for the npm update ... update my project in this minute :-)

paragi / sphp

Windows usage returns "No input File specified" #8