Add wp.org/ prefix for sources from wordpress.org #50008
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
noahtallen
added
[Type] Enhancement
|
Size Change: 0 B Total Size: 1.37 MB ℹ️ View Unchanged
|
ObliviousHarmony
requested changes
Apr 22, 2023
There was a problem hiding this comment.
Thanks @noahtallen! Having to grab the full URL is a bit annoying when you know the slug, so, I think this is a great addition. My only concern is that this feels like it's performing a lot of magic as-is. Depending on whether you're using core, themes, or plugins, wp.org/ will download from a completely different location.
How would you feel about removing the sourceType inference and just require them to put it in the URL?
- Core:
wp.org/wordpress-5.0 - Plugins:
wp.org/plugin/gutenberg - Themes:
wp.org/themes/something
This still keeps the much simpler syntax while making it more explicit what you're trying to download. You can then use any of those in "mappings" too, allowing you to download and install plugins/themes without activating them.
| const url = | ||
| sourceType === 'core' | ||
| ? `https://wordpress.org/${ basename }.zip` | ||
| : `https://downloads.wordpress.org/${ sourceType }/${ basename }.zip`; |
There was a problem hiding this comment.
If you use a wp.org/ source in "mappings" it will construct a URL like ttps://downloads.wordpress.org//gutenberg.zip since the sourceType is undefined.
| @@ -59,3 +59,47 @@ describe.each( gitTests )( 'parseSourceString', ( source ) => { | |||
| expect( basename ).toBe( source.basename ); | |||
| } ); | |||
| } ); | |||
|
|
|||
| const wpOrgTests = [ | |||
There was a problem hiding this comment.
We should cover the undefined case as well!
Imo, it kind of makes sense. We have three different types of dependencies (core, themes, plugins). Each comes from a different "repository". As a result, the way you download them is different. Does it make a big difference whether that's exposed to the user or not? In wp-env we always know what type of source we're trying to access based on where in the config you're adding the dependency. (And for mappings, wp-env doesn't manage dependencies directly like this.) So to me, it makes sense to simplify it for the user with just |
|
True @noahtallen, this seems aligned with the magic we're already doing with
Are you sure? I'm not in a position to test at the moment, but, as far as I can tell, there's no reason they shouldn't: There doesn't seem to be any meaningful distinction here between plugins, themes, and mappings. Wouldn't it be a reasonable workflow here to install a plugin but not activate it automatically? I think it would download it and keep it updated just like any other? What if the As an aside, I do kind of dislike the use of |
|
Apologies for doing a drive-by opinion, but I was just setting up Firstly, I'd love to see something like this. I personally believe the deference given to GitHub is a little odd and would have been fine with a "github/" prefix, but that's a little besides the point. Why not just use "wordpress/" as the prefix? And similar to how you would indicate a GitHub user/org, I'd actually prefer if something like "wordpress/plugin/foo" was used. I assume it's possible that a theme and a plugin might share the same slug, and this makes it explicit. You then don't have to explain how these shortcuts work differently depending on where they're used in the config. I prefer consistency over black-box magic. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Thanks for the feedback! At this point, I no longer have time to work on these, but PRs are always welcome. :) wp-env unfortunately doesn't have a lot of active maintainers at the moment! It might be worth also commenting on the original issue (#44848.) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Resolves #44848. Allows you to pull sources from WordPress.org using the
wp.org/prefix, followed by the slug.Why?
Just a small enhancement suggested by #44848. Since this is a first party tool, it makes sense to make it easy to access WordPress.org sources.
How?
Add a matcher for this scenario in the source parser.
Testing Instructions
npx wp-env start