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 : Give access to command arguments if array type is given in php ^7.4 proc_open function #944

Closed
wants to merge 4 commits into from
Closed

PHP : Give access to command arguments if array type is given in php ^7.4 proc_open function #944

wants to merge 4 commits into from

Conversation

mho22
Copy link
Contributor

@mho22 mho22 commented Jan 15, 2024

What is this PR doing?

Based on the issue #930

For now, if you have this PHP code :

$process = proc_open( [ 'ls', '-a' ], $descriptorspec, $pipes );

the php.setSpawnHandler( command => ... ) command parameter will only return the first element of the array.

What problem is it solving?

It gives the ability to get the overall command in one string separated by spaces. In the example above ls -a.

Testing Instructions

proc.php

$process = proc_open( [ 'ls', '-a' ], [ 0 => ['pipe', 'r'], 1 => ['pipe', 'w'], 2 => ['pipe', 'w'] ], $pipes );

sleep( 1 );

cli.js

import { NodePHP } from '@php-wasm/node';


const php = await NodePHP.load( '8.2' );

php.setSpawnHandler( command => console.log( command ) );

php.cli( [ 'php', ...process.argv.slice( 2 ) ] ).finally( () => process.exit( 0 ) );

node wasm/cli.js proc.php

returns :

ls -a

Node.js v18.18.2

@adamziel

P.S.: It seems argv is no longer needed. It was used with command and execvp to probably launch the command with its arguments. I decided to clean the file from the unused variable.

P.S.2 : It seems there is no more sign of PHP_WIN32 in proc_open7.4.c file but there are still in proc_open7.0. Should I make another PR to clean these files from these unneeded definitions, for clarity ?

P.S.3 : I only modified the proc_open7.4.c file for now, but I probably have to run some scripts, what are the steps ?

@adamziel
Copy link
Collaborator

adamziel commented Jan 15, 2024

Thank you @mho22!

This looks great, thank you! My only concern is that it simply joins the command array using the space character as "glue". This is probably fine for the simulated in-browser handlers, but it means these commands cannot be properly escaped in Node.js environment when passing the array to child_process.spawn(), e.g. we lose information about where the "splitting point" is.

Would you be able to preserve the array data structure and call js_open_process maybe like js_open_process(command, args, ...pipes and other existing args)?

P.S.: It seems argv is no longer needed. It was used with command and execvp to probably launch the command with its arguments. I decided to clean the file from the unused variable.

Thank you, good call!

P.S.2 : It seems there is no more sign of PHP_WIN32 in proc_open7.4.c file but there are still in proc_open7.0. Should I make another PR to clean these files from these unneeded definitions, for clarity ?

That would be great! I wouldn't be too opposed to removing those in this PR, too. Up to you :)

P.S.3 : I only modified the proc_open7.4.c file for now, but I probably have to run some scripts, what are the steps ?

To rebuild, say, PHP 8.2, run:

  • nx reset; npm run recompile:php:node:8.2 to rebuild the Node.js version
  • nx reset; npm run recompile:php:web:light:8.2 to rebuild the "light" web version that comes without libxml and other extensions
  • nx reset; npm run recompile:php:web:kitchen-sink:8.2 to rebuild the "full" web version that comes with libxml and other extensions

You may also rebuild all the major PHP versions like nx reset; npm run recompile:php:web and nx reset; recompile:php:node.

This above should be more visibly documented, that's on me. I'd love to revamp the existing docs.

@mho22
Copy link
Contributor Author

mho22 commented Jan 15, 2024

@adamziel

Would you be able to preserve the array data structure and call js_open_process maybe like js_open_process(command, args, ...pipes and other existing args)?

I wish I could. But this will probably be too complex for me to help.

I would really like to contribute to make command args ,dynamic decriptorspec and no more sleep method but I don't have the necessary C + WASM background to help. But I have time.

Another challenge is the locations where js_open_process is used, [ like wasm_open() in php_wasm the data needed generated differently.

@mho22
Copy link
Contributor Author

mho22 commented Jan 15, 2024

@adamziel Do you have some idea or suggestion on where and how I should share a char **args from proc_open7.4.c to php_8_2.js without losing data ? For the moment I keep being stuck with an array of unrelated 3 pointers, when I pass argv to the js_open_process(...

extern void *js_open_process(const char *command, char **argv, int procopen_call_nb, int out_childend, int out_parentend, int err_childend, int err_parentend);

I should easily pass a string but an array of strings is insurmountable. And this doesn't augur well. I suppose if I can go further, I could probably share other data.

@adamziel
Copy link
Collaborator

adamziel commented Jan 17, 2024

@mho You could probably pass it as two separate arguments, e.g.:

extern void *js_open_process(const char *command, char **argv, int argv_length, /* other args */);

And then on the JavaScript end you should be able to read argv_length pointers starting at the *argv location. I didn't test it, but perhaps something like this would work?

js_open_process: function (
	command,
	argv,
	argvLength,
	// ... other args
) {
	// Note we use `var`, not `const` in this file
	var jsArgsArray = [];
	var pointersStart = HEAPU8[argv];
	for(var i=0;i<argvLength;i++) {
		var pointer = HEAPU8[pointersStart + i * 32];
		jsArgsArray.push(UTF8ToString(pointer));
	}
}

@mho22
Copy link
Contributor Author

mho22 commented Jan 17, 2024

@adamziel thanks for the answer, I am on it.

@mho22 mho22 requested a review from a team as a code owner January 19, 2024 07:55
mho22 and others added 2 commits January 20, 2024 18:47

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@mho22
Copy link
Contributor Author

mho22 commented Jan 20, 2024

@adamziel I finally managed to solve the problem and it was not as easy as it seemed, because of the undetermined arguments number or even the undetermined length of argument themselves. I hope this suits your expectations as it suits my needs.

Next steps if validated :

Compile:all
Remove phpversion 8.2 in actual tests
Test:all

@mho22 mho22 changed the title PHP : Return command from array as string in php ^7.4 proc_open function PHP : Dispatch command from array as [ cmd, args ] in php ^7.4 proc_open function Jan 21, 2024
@mho22 mho22 changed the title PHP : Dispatch command from array as [ cmd, args ] in php ^7.4 proc_open function PHP : Give access to command arguments if array type is given in php ^7.4 proc_open function Jan 21, 2024
@@ -355,6 +355,7 @@ PHP_FUNCTION(proc_open)
struct php_proc_open_descriptor_item *descriptors = NULL;
int ndescriptors_array;
char **argv = NULL;
int nargv_array = 0;
Copy link
Collaborator

@adamziel adamziel Jan 22, 2024

Choose a reason for hiding this comment

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

This should be a bit easier to understand:

Suggested change
int nargv_array = 0;
int num_argv = 0;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok! I tried to follow the same logic as in the php legacy code for the descriptors. They used ndescriptors_aray for descriptors. But I will change this!

const cmdstr = UTF8ToString(command);
if (!cmdstr.length) {
return 0;
}

let jsArgsArray = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add a short explanation, e.g.:

Suggested change
let jsArgsArray = [];
// Extracts an array of CLI arguments that should be passed to `command`.
// On the C side, the args are expressed as `**char` so we must go read
// each of the `argsLength` `*char` pointers and convert the associated data into
// a JavaScript string.
let jsArgsArray = [];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I am not familiar with direct commit suggestion here and with the conflicts I fear having errors, can I add these suggestions directly in my next commit ?

Copy link
Collaborator

@adamziel adamziel Jan 22, 2024

Choose a reason for hiding this comment

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

Sure! FYI, merging these direct suggestions should cause no conflicts but of course use any workflows that are convenient for you.

var ptr = args + (((argsLength > 1 ? argsLength : 2) >> 1) + 1) * 8;
for (var i = 0; i < argsLength; i++) {
var str = UTF8ToString(ptr);
ptr +=
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wild! What are these calculations? A comment would be really helpful, especially if it contained a link to some manual page somewhere where this is explained.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, wild is the term. I will explain that part in the next commit, this was not easy to implement.

@adamziel
Copy link
Collaborator

This looks lovely @mho22! Other than a few questions I left and rebuilding all the PHP versions, this seems good to go. Thank you!

@adamziel
Copy link
Collaborator

Superseded by #969

adamziel added a commit that referenced this pull request Jan 26, 2024
…969)

Based on the issue #930

### Support for passing an array of strings as a `$command` when calling
`proc_open`

Without this PR, this `proc_open` call doesn't work:

```
$process = proc_open( [ 'ls', '-a' ], $descriptorspec, $pipes );
```

With this PR, it does and the `php.setSpawnHandler( )` callback receives
all the information from the `[ 'ls', '-a' ]` array.


### Support for an arbitrary number of file descriptors in `proc_open`

Without this PR, this `proc_open` call will error out because,
internally, PHP-WASM always expects three file descriptors:

```
$process = proc_open( 'stty', [ 1 => ['pipe', 'w'], 2 => ['pipe', 'w'] ], $pipes );
```

This PR removes that limitation so that `proc_open()` can work when 0 or
more file descriptors are passed

## Testing Instructions

### Support for passing an array of strings as a `$command` when calling
`proc_open`

`proc.php`

```
$process = proc_open( [ 'ls', '-a' ], [ 0 => ['pipe', 'r'], 1 => ['pipe', 'w'], 2 => ['pipe', 'w'] ], $pipes );
```

`cli.js`

```
import { NodePHP } from '@php-wasm/node';


const php = await NodePHP.load( '8.2' );

php.setSpawnHandler( (cmd, args) => console.log( cmd, args ) );

php.cli( [ 'php', ...process.argv.slice( 2 ) ] ).finally( () => process.exit( 0 ) );
```


running :

`node cli.js proc.php`

returns :

```
ls [ '-a' ]

Node.js v20.11.0
```


### Support for an arbitrary number of file descriptors in `proc_open`

`proc.php`

```
<?php

$command = 'ls -a';

$descriptorspec = [
    0 => [ 'pipe', 'r' ],
    1 => [ 'pipe', 'w' ]
];

$process = proc_open( $command, $descriptorspec, $pipes );

var_dump( stream_get_contents( $pipes[ 1 ] ) );
```

`cli.js`

```
import { NodePHP } from '@php-wasm/node';
import { spawn } from 'child_process';


const php = await NodePHP.load( '8.2' );


php.setSpawnHandler( (command) => spawn( command, [], { shell : true, stdio : [ 'pipe', 'pipe', 'pipe' ], timeout : undefined } ) );


php.cli( [ 'php', ...process.argv.slice( 2 ) ] ).finally( () => process.exit( 0 ) );
```

running :

`node cli.js proc.php`

returns : 

```
string(61) "cli.js
node_modules
package-lock.json
package.json
proc.php
"
```
___ 

Cfr : 

- [PHP : Dispatch available descriptor specs in js_open_process
function](#963)

- [PHP : Give access to command arguments if array type is given in php
^7.4 proc_open
function](#944)

---------

Co-authored-by: Adam Zieliński <adam@adamziel.com>
@mho22 mho22 closed this Jan 26, 2024
@mho22 mho22 deleted the array-command-in-proc-open-php-7-4 branch January 26, 2024 15:38
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.

2 participants