PHP.run(): Throw JS exception on runtime error, remove throwOnError flag

[bgrgicak]

What is this PR doing?

With this PR, PHP.run() throws an exception when php.run() is unsuccessful. In other words, it makes php.run({ throwOnError: true }) the default behavior and removes the throwOnError flag.

What problem is it solving?

With the introduction of the Playground logger, we want to collect all errors in the logger and let the user decide what to do with the logged errors. For this to work, we can't allow throwOnError to be false.

The early versions of Playground/PHP.wasm were meant to support this behavior, but they couldn't because of the technical limitations – the exit code captured from the PHP process did not reflect the truth about the actual success/failure.

The throwOnError flag was introduced together with the correct exitCode support. In the typical php.run() usage, you'll want to check for errors. Asking the developers to always inspect the exitCode is a mistake-prone pattern since it's easy to forget or get wrong.

With this PR, PHP.wasm does the error-checking work for the developer and reliably reports any runtime crashes using JavaScript exceptions. It also removes the non throwing– code path to reduce the number of possible ways of using the library, reduce the number of uncaught errors out in the wild, and ease the library maintenance.

Before this PR:

const r = await php.run({
    code: `<?php noSuchFunction();`
});
console.log(r.errors); // Fatal Error: Undefined function...

After this PR:

try {
    // This throws an exception:
    const r = await php.run({
        code: `<?php noSuchFunction();`
    });
} catch( e ) {
    console.log(e.output.stderr); // Fatal Error: Undefined function...
    console.log(e.output.stdout);
}

How is the problem addressed?

By removing all mentions of throwOnError and making sure tests pass.

Testing Instructions

• Confirm that all tests pass

[dmsnell]

How does this relate with the idea of deciding what to do when a plugin fails to install? I may want my Playground to continue booting even if something is missing, but I might instead want to have everything stop if another particular step fails.

[adamziel]

@dmsnell the Blueprint execution function would try/catch and either report the error or rethrow it.

[dmsnell]

the Blueprint execution function would try/catch and either report the error or rethrow it.

I don't understand what this means, but if you've thought about it that sounds good enough. when we were discussing moving forward when steps fail, it did seem like having a more nuanced mechanism than throw or not throw would be useful. maybe this PR brings us closer to having a more capable choice in those situations?

[bgrgicak]

How does this relate with the idea of deciding what to do when a plugin fails to install?

Not sure, we wanted to ensure that all errors are recorded by the logger and @adamziel suggested I remove this feature.

#1102 will allow developers to listen to request.error events if they want to do something custom in case there is an error. But I'm not sure if this answers your question.

[adamziel]

Since this is a breaking change, I'll CC @akirk, @sejas, @eliot-akira, and @wojtekn to assess the impact on the work they're maintaining. I've also reached out on Twitter and the Playground WP.org Slack channel.

Before this PR:

const r = await php.run({
    code: `<?php noSuchFunction();`
});
console.log(r.errors); // Fatal Error: Undefined function...

After this PR:

try {
    // This throws an exception:
    const r = await php.run({
        code: `<?php noSuchFunction();`
    });
} catch( e ) {
    console.log(e.output.stderr); // Fatal Error: Undefined function...
    console.log(e.output.stdout);
}

[eliot-akira]

From a user perspective, I checked some projects that are using PHP.run() in browser and server side, to see how this change would affect it.

None of the method calls used the throwOnError flag, meaning they implicitly depended on it being false - but neither were they checking response.exitCode every time to see if the call succeeded. In most places the call was wrapped in try/catch, and where it wasn't, the error was handled higher up as it bubbled up (or should have been handled as the application's responsibility).

The change to throw the PHP error by default would be beneficial, so that it "fails loudly" as in: "When there is a problem, the software fails as soon and as visibly as possible, rather than trying to proceed in a possibly unstable state."

A potential issue is that the conditional clause in BasePHP.run() has console outputs (warn and error) that cannot be avoided, before it throws.

if (response.exitCode !== 0) {
  ..
  console.warn(`PHP.run() output was:`, output);
  const error = new Error(
    `PHP.run() failed with exit code ${response.exitCode} and the following output: ` +
    response.errors
  );
  ..
  console.error(error);
  throw error;
}

https://github.com/WordPress/wordpress-playground/pull/1137/files#diff-b33e1e6834fb94ac2cb5338e3ae17c60abf7ac88cb057189a1915531e55f7243R279-R293

For my use case, it would be better if it didn't output to console at all, and leave that up to the caller of PHP.run() to handle - such as displaying an error message in a modal, or continue silently if it's not a show-stopper.

[bgrgicak]

For my use case, it would be better if it didn't output to console at all, and leave that up to the caller of PHP.run() to handle - such as displaying an error message in a modal, or continue silently if it's not a show-stopper.

Thanks! I was thinking about this while working on the logger. I thought about changing all console... calls to custom logs and not showing them in the browser by default. Displaying logs could be enabled using a flag, or read from a global logger variable.

[adamziel]

This is excellent work @eliot-akira, thank you so much! Also, I agree about not logging by default and deferring that decision to the developer. Back then we didn't have the logger API, but now we do and it could take care of that as @bgrgicak mentioned.

Let's wait for comments a bit more and merge if nothing comes up.

[adamziel]

Okay, no webapps feedback came in, it does seem like the existing integrations would be in-tact for the most part, or, in the worst case, would need to handle an actual PHP error that was previously ignored silently. Node package consumers won't be affected by merging this until they decide to upgrade the dependencies. Let's merge, hope for the best, and revert if anything goes wrong 🤞