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.

Sign up for GitHub

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

Jump to bottom

Throw an error when activating a theme or plugin that doesn't exist #1391

Merged
merged 12 commits into from
May 16, 2024
Merged

Throw an error when activating a theme or plugin that doesn't exist #1391

merged 12 commits into from
May 16, 2024

Conversation

bgrgicak
Copy link
Collaborator

@bgrgicak bgrgicak commented May 14, 2024 •
edited
Loading

Fixes #1347

What is this PR doing?

Throw an error when activating a theme or plugin that doesn't exist.

What problem is it solving?

Users currently don't know when the activate plugin and theme steps fail.

How is the problem addressed?

By checking if the file exists before trying to activate it.

Testing Instructions

  • Checkout this branch
  • Run this blueprint to activate an unexisting plugin
  • You should see the error modal with an error about the failed plugin install step
  • Run this blueprint to activate an unexisting theme
  • You should see the error modal with an error about the failed theme install step

@adamziel
Copy link
Collaborator

This would detect one type of error, but there may be others. How about checking it activate_plugin() returned a WP_Error instance and throwing an exception if it did?

adamziel and others added 3 commits May 14, 2024 15:19
@adamziel
GitHub export: Create new commits in your fork when writing to the up…
6913e34 
…stream repo isn't allowed (#1392)

When the GitHub export flow used a forked repository, it would still
attempt to create a branch in the upstream repository. This resulted in
the following cryptic error:

> There was an unexpected error (Not Found), please try again. If the
> problem persists, please report it at https://github.com/WordPress/
> wordpress-playground/issues.

This PR ensures that branch is created in the push target

 ## Testing instructions

1. Import an `upsidedown` theme from
https://github.com/Automattic/themes
2. Run `await
playground.writeFile('/wordpress/wp-content/themes/upsidedown/test.txt',
'test');` in devtools
3. Try to export the changes as a PR, confirm a PR got created

Closes #1367
@bgrgicak
Fix theme path
b5428a6
@bgrgicak
Catch wp_error when activating plugin
da5a371
bgrgicak
bgrgicak commented May 15, 2024
View reviewed changes
packages/php-wasm/logger/src/lib/logger.ts
Comment on lines +170 to +173
export const prepareLogMessage = (message: string) => {
return message.replace(/\t/g, '');
};

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unrelated to the PR. It removes tabs from log messages.

@bgrgicak bgrgicak marked this pull request as ready for review May 15, 2024 05:23
@bgrgicak
Copy link
Collaborator Author

This would detect one type of error, but there may be others. How about checking it activate_plugin() returned a WP_Error instance and throwing an exception if it did?

I was able to add it for plugins, but not for themes. switch_theme succeded if a theme didn't exist, but the JS should be enough to catch this.

@bgrgicak bgrgicak requested a review from adamziel May 15, 2024 05:25
@bgrgicak bgrgicak self-assigned this May 15, 2024
@bgrgicak bgrgicak added [Type] Bug An existing feature does not function as intended [Package][@wp-playground] Blueprints labels May 15, 2024
@bgrgicak bgrgicak marked this pull request as draft May 15, 2024 09:53
@adamziel
Copy link
Collaborator

Merging 8f43a9dcfffb770683a00afa4713c08e7d5dfe37 directly to trunk caused E2E failures, I think that step is now broken when pluginPath is relative. I'm reverting it. Let's avoid merging risky changes directly without waiting for CI checks:

CleanShot 2024-05-15 at 15 25 30@2x
Blueprints -- enableMultisite step should re-activate the plugins (failed)

@bgrgicak
Copy link
Collaborator Author

bgrgicak commented May 16, 2024 •
edited
Loading

Merging 8f43a9dcfffb770683a00afa4713c08e7d5dfe37 directly to trunk caused E2E failures, I think that step is now broken when pluginPath is relative. I'm reverting it.

Sorry about that, I made a mistake and somehow pushed to trunk directly. Thank you for fixing it!

bgrgicak
bgrgicak commented May 16, 2024
View reviewed changes
packages/playground/website/cypress/e2e/blueprints.cy.ts
plugins: ['hello-dolly'],
steps: [{ step: 'enableMultisite' }],
steps: [
{ step: 'login' },
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This step started failing because it was missing the login step.
Without login, the user get's stuck on the login screens instead of being redirected to wp-admin.

@bgrgicak bgrgicak marked this pull request as ready for review May 16, 2024 05:13
@bgrgicak bgrgicak requested a review from adamziel May 16, 2024 05:16
@adamziel adamziel merged commit 1e184d8 into trunk May 16, 2024
5 checks passed
@adamziel adamziel deleted the fix/1347-check-if-exists-before-activating branch May 16, 2024 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamziel adamziel Awaiting requested review from adamziel

Assignees

@bgrgicak bgrgicak

Labels
[Package][@wp-playground] Blueprints [Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The activatePlugin step doesn't throw an error when given invalid inputs
2 participants
@bgrgicak @adamziel