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

wp-env: Correctly parse basename for wporg zip sources #22093

Merged
merged 5 commits into from
May 22, 2020
Merged

wp-env: Correctly parse basename for wporg zip sources #22093

merged 5 commits into from
May 22, 2020

Conversation

torounit
Copy link
Member

@torounit torounit commented May 5, 2020

Description

In the case of plugin/theme from downloads.wordpress.org, use the directory name as the name of the plugin/theme.

{
	"plugins": [
		"https://downloads.wordpress.org/plugin/wordpress-importer.0.7.zip"
	],
}

before

downloads.wordpress.org%2Fplugin%2Fwordpress-importer.0.7

after

wordpress-importer

How has this been tested?

config wp-env.json

{
	"core": "WordPress/WordPress",
	"plugins": [
		".",
		"https://downloads.wordpress.org/plugin/wordpress-importer.zip",
		"https://downloads.wordpress.org/plugin/debug-bar.1.0.zip"
	],
	"themes": [
		"https://downloads.wordpress.org/theme/go.zip",
		"https://downloads.wordpress.org/theme/chaplin.2.4.2.zip"
	]
}

run ./packages/env/bin/wp-env start

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

workDirectoryPath,
encodeURIComponent( wpOrgFields[ 2 ] )
),
basename: encodeURIComponent( wpOrgFields[ 2 ] ),
Copy link
Member

Choose a reason for hiding this comment

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

should we just make this apply to the zipFields thing below, and use the second index instead of the first? I guess a reason to not do that would be to allow zip sources from elsewhere to continue working :)

@noahtallen noahtallen added the [Tool] Env /packages/env label May 21, 2020
Copy link
Member

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

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

I think this is a good addition. It should make zip sources a lot easier to work with.

@noahtallen
Copy link
Member

I think this needs rebased and it is probably good to go. Imo, it would also be ideal if we just use the zip type for this instead of creating new source types. E.g. inside the zip type condition, check to see if it is also a wporg source, and then set the basename & path correctly in that situation.

@torounit
Copy link
Member Author

Fix to use zip source type. @noahtallen

);
if ( wpOrgFields ) {
return {
type: 'zip',
Copy link
Member

Choose a reason for hiding this comment

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

could this just be added to the zipFields section below?

@noisysocks
Copy link
Member

Should we do this for all ZIP URLs and not just ones hosted on downloads.wordpress.org? For example, example.com/uploads/my-plugin.2.zip can extract into my-plugin.2.

basename: encodeURIComponent( wpOrgFields[ 1 ] ),
};
}

Copy link
Member

Choose a reason for hiding this comment

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

parseSourceString has unit tests. These should be updated to test this new functionality.

Copy link
Member Author

Choose a reason for hiding this comment

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

added unit tests.

@torounit
Copy link
Member Author

@noisysocks Is it better to keep the version number in the directory name?

@noahtallen
Copy link
Member

Should we do this for all ZIP URLs

since we can know (being a WordPress tool and all) that wporg URLs always look this way, maybe that makes it easier to just support the wporg format? Do we know enough to guarantee a different zip source will use a format we can understand?

Though, I guess if it's just https://someurl/path/to/x-y-z.zip, then we could just match everything between the last / and .zip.

@noahtallen
Copy link
Member

I'm gonna merge this so that it can at least be fixed for wporg sources for now. I think we can expand this behavior as a follow up without blocking it for wporg sources :)

@noahtallen noahtallen changed the title [@wordpress/env] convert theme/plugin dirname, In the case of plugin/theme from downloads.wordpress.org. wp-env: Correctly parse basename for wporg zip sources May 22, 2020
@noahtallen noahtallen merged commit 189da69 into WordPress:master May 22, 2020
@epiqueras
Copy link
Contributor

I think that other URLs are more likely to have conflicting basenames.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Tool] Env /packages/env
Projects
None yet
Development

Successfully merging this pull request may close these issues.

env: Using Zip file URLs for plugins prevents their assets from being served
4 participants