Always include slug directory in zip
Description of the Change
When zipping a file, the zip command can zip the files with no path, a relative path, or an absolute path:
| Type | Zip File Result | Zip Command |
|---|---|---|
| No Path | my-plugin.php |
zip my-plugin.zip . |
| Relative Path | my-plugin/my-plugin.php |
zip my-plugin.zip my-plugin/ |
| Absolute Path | /path/to/my-plugin/my-plugin.php |
zip my-plugin.zip /path/to/my-plugin |
When unzipping, the path type comes into effect. No Path uses the zip file name as the directory to put the files in, or it unloads into the current directory. Relative creates a new directory with the relative structure. Absolute attempts to unload to the absolute position.
WordPress itself has a limitation when retrieving zip files from remote servers for plugin updates wherein the zip file must be in the Relative Path format. The other two will break.
This PR changes from a No Path zip to a Relative Path zip, with the plugin slug being the relative directory.
Alternate Designs
Presently, the No Path method is used when zipping. The alternatives are as outlined above: Relative and Absolute.
Benefits
When folks use the Plugins > Add New > Upload feature, if the zip files have No Path, then it assumes the zip name to be the directory for the files. The same is true when unzipping in the OS. So if a user downloads the zip, has a zip in the folder already with that name (which renames it to something like plugin.zip(1)), and then installs it, it will be installed to the plugin1 directory. By using a Relative Path, it's ensured that the directory will always reflect the plugin slug.
Possible Drawbacks
No drawbacks that I can think of. The zip file will work more intuitively.
Verification Process
We dealt with this over at GiveWP and had to wrestle this weird issue down. Our scripts now all use a Relative Path. We used this action for deployment and I noticed it's using No Path, so I thought I'd hop over and help! We've generated the zip using all the methods and tested them via upload install into WordPress.
Checklist:
- [X] I have read the CONTRIBUTING document.
- [X] My code follows the code style of this project.
- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [ ] I have added tests to cover my change.
- [ ] All new and existing tests passed.
Applicable Issues
Changelog Entry
Changed
- Generated zip now stores files with relative path for WP installation consistency
I need to basically emergency release a 2.0 to deal with a Debian image failure that means we will have something much faster moving forward anyway, so I'm sorry this will miss that release but I would like to get another one done within the next few weeks.
Could you possibly refresh this and rework it to not move the trunk directory? I don't think moving the directory matters to the job at that point but it is possible somebody is relying on it being in place in later steps, and also just kind of feels a little weird to reach in to the SVN setup like that. Thank you so much for this, I don't currently use the ZIP for anything myself so I hadn't run into these issues.
Hi @helen!
Sure, happy to refresh this. You mention not moving the trunk directory. Do you think copying into another directory is fine? Or renaming it back to trunk after generating the zip? The latter would be much faster, if it's fine, given the repository size. One way or the other, we need a directory with all the repo contents with the repo name to be zipped up.
@JasonTheAdams Good question - renaming trunk is probably fine so long as we name it back. I am just cautious given that somebody could potentially be doing something like adding another step in their workflow that goes in and does more in the SVN directory structure, as slim as that chance may be right now.
So I needed to do this in a different action, and the --prefix flag seems to work for me, what do you think?
Sorry, can you provide more context? I'm not sure what command you're referring to that supports the --prefix flag.
Ah 🤦♀️ that's my fault, I forgot I was dealing with git archive instead of zip. However, in looking at it some more, it seems like making a symlink would work rather than doing a copy or move?
My only concern with a symlink is that I'm not sure how zip would handle it on different OS's. Since this is now a composite action, we want to be a simple as possible to avoid edge cases. Rename, zip, rename back is boring but (I think) reliable.
Aiming to refresh this in the next couple days, FYI.
It's already a little bit risky as a composite action with sed differences (and maybe even rsync), I guess somebody could be trying to do this on a Windows runner or something and I have no idea what would happen if they did. Doing a symlink seems okay to me given what's already going on in here but in reality it probably doesn't matter one way or the other unless for some reason somebody is running this on a private repo and using up their minutes, and even then it's minimal impact. No rush and no pressure, just kinda thinking out loud!
Thanks, Helen! Honestly, I've never tried zipping up a symlink. Sounds like black magic.
That said, I just tested out zipping up a symlink, giving the resulting file a different name than the symlink, and sure enough when I unpacked it the resulting folder had the symlink name. 🤯
So when I'll go with that! Thanks for teaching me something new!
@helen I just refreshed this. 🎉
I see in the Checklist an item for adding tests. I can't see where or how to add tests. Mind pointing me in the right direction?
Also, I suggest squashing this when all is said and done.
Nice! There aren't any real tests besides Shellcheck for linting right now, I just approved it to run and you'll see that it's telling you to wrap something in quotes (I think one of the usages of $SLUG). Zipping a symlink def does feel like magic but I like that it works :)
I'll be taking a look at various things in the next few weeks or so to see what I want to wrap up in a new release, there's a lot of outstanding stuff around custom workspace/accommodating things like Composer better, so I want to make sure I at least know what can be done now or not. It may take me a bit simply because I have a particularly busy few weeks ahead and don't have anybody else working on maintenance here right now, just know that I very much appreciate the tip and your work here!
Just added the double-quotes around $SLUG. I believe this is good to go.
No rush on this. I'm happy to have it added at your leisure. It's not causing us any problems. 😃
@iamdharmesh bumping back up for your review.