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

[Blueprints] Ensure plugins and themes are installed in subfolders when using url references in the installPlugin/Theme step #2128

Draft
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

bgrgicak
Copy link
Collaborator

@bgrgicak bgrgicak commented Jan 14, 2025

Motivation for the change, related issues

Before this PR, a plugin or theme could be installed to the wp-content/plugins and wp-content/themes folder without a subfolder.
This would happen when the plugin/theme resource was installed using references without a name attribute, for example using a URL reference.

This was caused by the install asset code assuming there will always be a name but the name isn't a required attribute in some reference types, so it can be empty.

To fix this we now generate a random asset name in case it doesn't already exist.

There is a chance that a plugin doesn't use a subfolder (for example Hello Dolly), but because we can't assume this it's safer to install it into a subfolder. Otherwise, multiple plugins installed in the wp-content/plugins or wp-content/themes directory could override each other's files.
Also today the activate plugin script assumes there is only one plugin in the provided folder.

Potential improvements

This approach isn't ideal because it means the plugin/theme will be installed into a randomly generated folder which isn't expected. We should consider other ways of extracting the name. Exploration conclusion

Also, we could add support for names to other resource types, but this wouldn't fix existing Blueprints.

Testing Instructions (or ideally a Blueprint)

  • Open this Playground URL
  • Confirm that the To Do Mvc plugin is active and that there are no errors in the browser console

Comment on lines +81 to +86
/**
* If the plugin was successfully activated, we can return early.
*/
if (activatePluginResult.text === 'true') {
return;
}
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 isn't directly related to this PR, but while I was debugging it, I realized that this will make the activatePlugin step more robust and will avoid the extra check when it's not needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On it's own this fix would allow the PR test to pass because the plugin would successfully activate inside the plugins folder.

* the asset folder name exists.
*/
if (!zipFileName) {
assetNameGuess = `${randomString(32, '')}`;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Technically this could break if the path already exists, but I wasn't sure if checking the path would overcomplicate the implementation.

@bgrgicak
Copy link
Collaborator Author

This approach isn't ideal because it means the plugin/theme will be installed into a randomly generated folder which isn't expected. We should consider other ways of extracting the name.

We should be able to extract the name when fetching the resource because it's available in the content-disposition header. I tried this, but for some reason the header isn't there.

--- a/packages/playground/blueprints/src/lib/resources.ts
+++ b/packages/playground/blueprints/src/lib/resources.ts
@@ -316,7 +316,24 @@ export abstract class FetchResource extends Resource<File> {
                        if (response.status !== 200) {
                                throw new Error(`Could not download "${url}"`);
                        }
-                       return new File([await response.blob()], this.name);
+
+                       let filename = this.name;
+                       if (!filename) {
+                               const contentDisposition = response.headers.get(
+                                       'content-disposition'
+                               );
+                               if (contentDisposition) {
+                                       const matches =
+                                               /filename[^;=\n]*=((['"]).*?\2|[^;\n]*)/.exec(
+                                                       contentDisposition
+                                               );
+                                       if (matches && matches[1]) {
+                                               filename = matches[1].replace(/['"]/g, '');
+                                       }
+                               }
+                       }

@bgrgicak
Copy link
Collaborator Author

We should be able to extract the name when fetching the resource because it's available in the content-disposition header. I tried this, but for some reason, the header isn't there.

We can't get the name from the response because CORS prevents us from reading response headers for cross-origin requests.

@bgrgicak bgrgicak self-assigned this Jan 14, 2025
@bgrgicak
Copy link
Collaborator Author

I'm leaving this as a draft for now, but I would love to get feedback on the direction this PR takes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs review
Development

Successfully merging this pull request may close these issues.

1 participant