Skip to content

Commit

Permalink
Quit callback overwrites the quit callback template (#937)
Browse files Browse the repository at this point in the history
By default, even if you add a `quit` method in `emscryptenOptions` : 

```
const php = await NodePHP.load( '8.2', { emscriptenOptions : { quit : ( status, toThrow ) => { if( status === 0 ){ process.exit() } return toThrow } } } );
```

 it is overwritten by the actual one on line `57` in `node-php.ts` : 


```
quit: function (code, error) {
	throw error;
},
```

This PR is moving this template callback to be overwritten by
`emscryptenOptions` object given by user.

## What problem is it solving?

To avoid throwing error from `php.exit()` even if it is exiting quietly.


## Testing Instructions

```
const php = await NodePHP.load( '8.2', emscriptenOptions : { quit : ( status, toThrow ) => { if( status === 0 ){ process.exit() } return toThrow } } } );

php.exit(0);
```

This should display nothing.
  • Loading branch information
mho22 authored Jan 12, 2024
1 parent b025f95 commit 4669959
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,6 @@ const LibraryExample = {
// Pass data from child process's stderr to PHP's end of the stdout pipe.
const stderrStream = SYSCALLS.getStreamFromFD(stderrChildFd);
cp.stderr.on('data', function (data) {
console.log('Writing error', data.toString());
PHPWASM.proc_fds[stderrParentFd].hasData = true;
PHPWASM.proc_fds[stderrParentFd].emit('data');
stderrStream.stream_ops.write(
Expand Down
2 changes: 1 addition & 1 deletion packages/php-wasm/node/src/lib/node-php.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ export class NodePHP extends BasePHP {
return await NodePHP.loadSync(phpVersion, {
...options,
emscriptenOptions: {
...(options.emscriptenOptions || {}),
/**
* Emscripten default behavior is to kill the process when
* the WASM program calls `exit()`. We want to throw an
Expand All @@ -57,6 +56,7 @@ export class NodePHP extends BasePHP {
quit: function (code, error) {
throw error;
},
...(options.emscriptenOptions || {}),
},
}).phpReady;
}
Expand Down
21 changes: 17 additions & 4 deletions packages/php-wasm/node/src/test/php.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ describe.each(SupportedPHPVersions)('PHP %s', (phpVersion) => {
$pipes
);
// Yields back to JS event loop to capture and process the
// Yields back to JS event loop to capture and process the
// child_process output. This is fine. Regular PHP scripts
// typically wait for the child process to finish.
sleep(1);
Expand Down Expand Up @@ -133,7 +133,7 @@ describe.each(SupportedPHPVersions)('PHP %s', (phpVersion) => {
// This test fails
if (!['7.0', '7.1', '7.2', '7.3'].includes(phpVersion)) {
/*
There is a race condition in this variant of the test which
There is a race condition in this variant of the test which
causes the following failure (but only sometimes):
src/test/php.spec.ts > PHP 8.2 > proc_open() > cat – stdin=pipe, stdout=file, stderr=file
Expand All @@ -153,7 +153,7 @@ describe.each(SupportedPHPVersions)('PHP %s', (phpVersion) => {
);
fwrite($pipes[0], 'WordPress\n');
// Yields back to JS event loop to capture and process the
// Yields back to JS event loop to capture and process the
// child_process output. This is fine. Regular PHP scripts
// typically wait for the child process to finish.
sleep(1);
Expand Down Expand Up @@ -184,7 +184,7 @@ describe.each(SupportedPHPVersions)('PHP %s', (phpVersion) => {
$pipes
);
// Yields back to JS event loop to capture and process the
// Yields back to JS event loop to capture and process the
// child_process output. This is fine. Regular PHP scripts
// typically wait for the child process to finish.
sleep(1);
Expand Down Expand Up @@ -1073,3 +1073,16 @@ describe.each(['7.0', '7.1', '7.3', '7.4', '8.0', '8.1'])(
}, 500_000);
}
);

describe.each(SupportedPHPVersions)('PHP %s', (phpVersion) => {
describe('emscripten options', () => {
it('calls quit callback', async () => {
let result = '';
const php: NodePHP = await NodePHP.load(phpVersion as any, {
emscriptenOptions: { quit: () => (result = 'WordPress') },
});
php.exit(0);
expect(result).toEqual('WordPress');
});
});
});

0 comments on commit 4669959

Please sign in to comment.