Skip to content

Conversation

@JasonTheAdams
Copy link
Contributor

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:

  • I have read the CONTRIBUTING document.
  • 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

@helen
Copy link
Collaborator

helen commented Aug 16, 2021

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.

@JasonTheAdams
Copy link
Contributor Author

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.

@jeffpaul jeffpaul added this to the 2.1.0 milestone Aug 20, 2021
@helen
Copy link
Collaborator

helen commented Aug 25, 2021

@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.

@helen
Copy link
Collaborator

helen commented Sep 1, 2021

So I needed to do this in a different action, and the --prefix flag seems to work for me, what do you think?

@JasonTheAdams
Copy link
Contributor Author

Sorry, can you provide more context? I'm not sure what command you're referring to that supports the --prefix flag.

@helen
Copy link
Collaborator

helen commented Sep 1, 2021

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?

@JasonTheAdams
Copy link
Contributor Author

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.

@helen
Copy link
Collaborator

helen commented Sep 3, 2021

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!

@JasonTheAdams
Copy link
Contributor Author

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!

@JasonTheAdams
Copy link
Contributor Author

JasonTheAdams commented Sep 3, 2021

@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.

@helen
Copy link
Collaborator

helen commented Sep 4, 2021

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!

@JasonTheAdams
Copy link
Contributor Author

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. 😃

dinhtungdu
dinhtungdu previously approved these changes Jan 7, 2022
Copy link
Contributor

@dinhtungdu dinhtungdu left a comment

Choose a reason for hiding this comment

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

The update looks good to me. Good to learn about zipping symlink 🤯

@dinhtungdu dinhtungdu self-requested a review January 7, 2022 14:49
@dinhtungdu dinhtungdu dismissed their stale review January 7, 2022 14:50

The latest commit contains left over command.

@jeffpaul jeffpaul added the needs:feedback This requires reporter feedback to better understand the request. label Jan 7, 2022
@jeffpaul jeffpaul requested a review from iamdharmesh April 19, 2022 17:08
@jeffpaul jeffpaul modified the milestones: 2.1.0, 2.2.0 May 11, 2022
@dkotter dkotter removed the request for review from dinhtungdu August 23, 2022 22:15
@jeffpaul
Copy link
Member

@iamdharmesh bumping back up for your review.

@iamdharmesh iamdharmesh requested a review from dkotter January 6, 2023 09:16
Copy link
Member

@iamdharmesh iamdharmesh left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @JasonTheAdams. LGTM! 🚀

@dkotter I have made some minor changes in the existing PR. So, I requested a quick review from you.

Thanks.

@jeffpaul jeffpaul merged commit ec1eaac into 10up:develop Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs:feedback This requires reporter feedback to better understand the request.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Generated zip won't have plugin-slug folder inside

6 participants