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

An invalid plugin slug should not cause following plugins to not be installed #600

Open
akirk opened this issue Jul 15, 2023 · 6 comments
Open
Labels
[Type] Bug An existing feature does not function as intended

Comments

@akirk
Copy link
Member

akirk commented Jul 15, 2023

On playground.wordpress.net, if you happen to specify a plugin slug that doesn't exist in the WordPress Plugin directory, it will only install the plugins until it encounters the first inexistant. In the progress screen you can see that theoretically it dowloads them all but then on the Plugins page you can only see the first plugins.

Example: https://playground.wordpress.net/?plugin=activitypub&plugin=inexistant&plugin=biscotti will give you these installed plugins:

Screenshot 2023-07-15 at 17 15 40

If I omit the inexistant plugin it works: https://playground.wordpress.net/?plugin=activitypub&plugin=biscotti

Screenshot 2023-07-15 at 17 16 31

I came to this through the following snippet which, when executed on /wp-admin/plugins.php will give you a Playground URL for all of your plugins:

'https://playground.wordpress.net/?plugin='+Array.from(document.querySelectorAll('table.plugins tr.active')).map(tr => tr.dataset.slug).join('&plugin=')

But if you have custom or premium plugins, it will trip up the Playground.

@akirk akirk added the [Type] Bug An existing feature does not function as intended label Jul 15, 2023
@eliot-akira
Copy link
Collaborator

eliot-akira commented Jul 15, 2023

Looking into how the Playground installs multiple plugins (here), it creates a separate Blueprint step for each plugin install.

And looking into how Blueprint steps are executed altogether, here is where compiled steps are run.

for (const { run, step } of compiled) {
	const result = await run(playground);
	onStepCompleted(result, step);
}

The steps are run sequentially, but I believe internally the fetch requests are started beforehand and run in parallel in batches. If one step throws an error, the rest of the steps are skipped. In other words, compiledSteps.run() throws once for all steps. I think that's intentional, by design.

To let all plugins continue installing even if some of them fail, it seems the Playground may need to create a single Blueprint step that handles it, with something like Promise.all() but not stopping when there's an error. It could gather any errors, and throw once for all of them. But then, that would still prevent the remaining compiled steps to be run. An alternative would be to not throw an error at all even if any plugin installs fail - but I wonder if that's a reasonable thing to do.

Another approach might be for each Blueprint step to have an option to be "fail-able", so even if it throws, the rest of the steps will continue running.

@dmsnell
Copy link
Member

dmsnell commented Jul 18, 2023

in #540 we're looking at getting toast notifications at least on the main website. not sure how best to handle this in Node environments, but we could announce the failure.

I also came across this today when looking for the import and export buttons on the browser chrome. if a missing plugin is requested they never load.

at one point I remember discussing the use of Promise.allSettled() but I'm not sure if it's here. it seems reasonable to keep on installing whatever plugins we can; and then in a further enhancement notify the user which ones failed.

@eliot-akira what risks do you see in continuing to install plugins after one fails?

@eliot-akira
Copy link
Collaborator

eliot-akira commented Jul 18, 2023

what risks do you see in continuing to install plugins after one fails?

The only risk I imagine is that some of the remaining Blueprint steps might depend on a plugin to be installed, and may error if it's not. For example, if a step calls a plugin function or instantiate a class that doesn't exist. But even then, it seems better than interrupting the entire process.

it seems reasonable to keep on installing whatever plugins we can; and then in a further enhancement notify the user which ones failed.

I agree, at the least it should let other plugins continue installing, and probably let any remaining Blueprint steps (after plugin install) to continue as well.

@eliot-akira
Copy link
Collaborator

eliot-akira commented Jul 19, 2023

OK, I'll start a pull request to explore a possible solution. However, I'm a bit conflicted and uncertain of the best way to solve it.

If I try to imagine what @adamziel would say, who wrote the logic of how blueprints work, I get the feeling he might not agree with letting plugin installs fail silently, or for blueprint steps to continue running after a failed plugin install.

It reminds me of one of the UNIX design principles, "When you must fail, fail noisily and as soon as possible."

An advantage of this approach is to avoid letting a process continue in an unstable state. Failing early and visibly makes the diagnosis of an error or bug easier. I'd say that describes the current behavior of how Blueprint steps are run: if one step fails, stop everything and throw an error. I think that's how it should work when blueprints are used in code (internal or user land).

On the other hand, it's unintuitive how this works with the Playground's query parameter API. Instead of stopping when a single plugin fails to install, it flashes an error message (?) and continues to start up the WordPress instance with the remaining plugin installs silently skipped. That's an issue.

It implies that the workaround should be implemented in the Playground package website where the Query API is, and not the package blueprints where the general-purpose blueprint logic and steps are defined.


Then a technical question is: if all plugin installs are to be consolidated into a single Blueprint step that does not stop when an install fails (for any reason or just network error?) - I wonder if it can still take advantage of the internal logic to batch and parallelize file requests, which is the purpose of "compiling" the blueprint steps. It might take effort to figure out how to do this manually.

It could be easier to keep each plugin install as a separate step, and add an option to not throw an error even if it fails. (Should that be for the installPlugin step only, or all steps in general?)

To prepare for the future when such errors can be shown in a notification (as in #540), the option could be an onError callback, which when defined will be called with the error instead of throwing it. Does that sound reasonable @dmsnell?

@dmsnell
Copy link
Member

dmsnell commented Jul 19, 2023

letting plugin installs fail silently

for this argument the point is moot because that's now how the playground currently works. currently it does fail silently, and the lack of a best-effort for the rest means other things surprisingly break without any obvious issue.

if we want to fail flagrantly with an alert that sounds like a great idea; it's why I linked to #540 which provides a Toast-style alerting mechanism.

at a minimum, I don't want a typo in a plugin name to prevent loading the import and export buttons. it's my guess that this is a related issue but separate from the question of whether to atomically complete all the steps.


in other words, my preferences are:

  • fix current outright breakage (for example, the import/export buttons)
  • communicate failure about plugins
  • expand functionality to handle certain kinds of failures

if there are broader questions about Blueprints behaviors then we can ask those. I bet it's fine to install all the plugins we can and then notify which ones failed, stop processing Blueprint steps after running the plugin (and theme) installs.

it's also my guess that this is more of a problem on the website than it is in Blueprint processing, where presumably someone is testing their Blueprint setup before publishing, but on the web people are willy-nilly adding query arguments.

eliot-akira added a commit to eliot-akira/wordpress-playground that referenced this issue Jul 19, 2023
@eliot-akira
Copy link
Collaborator

A rough draft started:

@adamziel adamziel added this to the Zero Crashes milestone Feb 29, 2024
@adamziel adamziel moved this to Future work in Playground Board Jun 30, 2024
@adamziel adamziel removed this from the Zero Crashes milestone Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
Archived in project
Development

No branches or pull requests

4 participants