PHP : Give access to command arguments if array type is given in php ^7.4 proc_open function by mho22 · Pull Request #944 · WordPress/wordpress-playground

mho22 · 2024-01-15T07:53:57Z

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 · 2024-01-15T10:16:23Z

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 · 2024-01-15T11:22:48Z

@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 · 2024-01-15T16:18:20Z

@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 · 2024-01-17T16:15:56Z

@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 · 2024-01-17T16:22:34Z

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

…function

mho22 · 2024-01-20T23:56:00Z

@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

adamziel · 2024-01-22T14:36:55Z

 	struct php_proc_open_descriptor_item *descriptors = NULL;
 	int ndescriptors_array;
 	char **argv = NULL;
+    int nargv_array = 0;


This should be a bit easier to understand:

Suggested change

int nargv_array = 0;

int num_argv = 0;

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!

adamziel · 2024-01-22T14:41:02Z

 			return 0;
 		}

+		let jsArgsArray = [];


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 = [];

👍 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 ?

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

adamziel · 2024-01-22T14:42:36Z

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


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.

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

adamziel · 2024-01-22T14:43:58Z

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 · 2024-01-23T16:40:33Z

Superseded by #969

…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>

Give access to command arguments if array type is given in proc_open …

af2508b

…function

mho22 requested a review from a team as a code owner January 19, 2024 07:55

Removed const in file

8e4f52f

adamziel mentioned this pull request Jan 19, 2024

PHP: Make proc_open work without requiring an explicit sleep() #951

Closed

mho22 and others added 2 commits January 20, 2024 18:47

Merge branch 'trunk' into array-command-in-proc-open-php-7-4

130b0ae

Give access to command arguments if array type is given in proc_open …

a4b46e6

…function

mho22 changed the title ~~PHP : Return command from array as string in php ^7.4 proc_open function~~ Jan 21, 2024

mho22 changed the title ~~PHP : Dispatch command from array as [ cmd, args ] in php ^7.4 proc_open function~~ Jan 21, 2024

mho22 mentioned this pull request Jan 22, 2024

PHP : Dispatch available descriptor specs in js_open_process function #963

Closed

adamziel reviewed Jan 22, 2024

View reviewed changes

mho22 mentioned this pull request Jan 23, 2024

PHP : Add args and descriptors dynamic arrays in proc open function #969

Merged

mho22 closed this Jan 26, 2024

mho22 deleted the array-command-in-proc-open-php-7-4 branch January 26, 2024 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

PHP : Give access to command arguments if array type is given in php ^7.4 proc_open function#944
mho22 wants to merge 4 commits into
WordPress:trunkfrom
mho22:array-command-in-proc-open-php-7-4

mho22 commented Jan 15, 2024 •

edited

Loading

adamziel commented Jan 15, 2024 •

edited

Loading

mho22 commented Jan 15, 2024

mho22 commented Jan 15, 2024

adamziel commented Jan 17, 2024 •

edited

Loading

mho22 commented Jan 17, 2024

mho22 commented Jan 20, 2024

adamziel Jan 22, 2024 •

edited

Loading

mho22 Jan 22, 2024

adamziel Jan 22, 2024

mho22 Jan 22, 2024

adamziel Jan 22, 2024 •

edited

Loading

adamziel Jan 22, 2024

mho22 Jan 22, 2024

adamziel commented Jan 22, 2024

adamziel commented Jan 23, 2024

Labels

2 participants

-		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 = [];

Uh oh!

Conversation

mho22 commented Jan 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is this PR doing?

What problem is it solving?

Testing Instructions

adamziel commented Jan 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mho22 commented Jan 15, 2024

mho22 commented Jan 15, 2024

adamziel commented Jan 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mho22 commented Jan 17, 2024

mho22 commented Jan 20, 2024

adamziel Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

mho22 Jan 22, 2024

Choose a reason for hiding this comment

adamziel Jan 22, 2024

Choose a reason for hiding this comment

mho22 Jan 22, 2024

Choose a reason for hiding this comment

adamziel Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

adamziel Jan 22, 2024

Choose a reason for hiding this comment

mho22 Jan 22, 2024

Choose a reason for hiding this comment

adamziel commented Jan 22, 2024

adamziel commented Jan 23, 2024

Labels

2 participants

mho22 commented Jan 15, 2024 •

edited

Loading

adamziel commented Jan 15, 2024 •

edited

Loading

adamziel commented Jan 17, 2024 •

edited

Loading

adamziel Jan 22, 2024 •

edited

Loading

adamziel Jan 22, 2024 •

edited

Loading