-
Notifications
You must be signed in to change notification settings - Fork 53
Follow symlinks in Preview sites when archiving directories #1447
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as expected, and now Preview sites follow symlinks when generating the zip file.
Let's rename the patch file from patches/archiver+6.0.2.patch to patches/archiver+6.0.1.patch. I got a warning when installing the dependencies with npm install.
patch-package 8.0.0
Applying patches...
@automattic/[email protected] ✔
@wordpress/[email protected] ✔
@wordpress/[email protected] ✔
[email protected] ✔
Warning: patch-package detected a patch file version mismatch
Don't worry! This is probably fine. The patch was still applied
successfully. Here's the deets:
Patch file created for
[email protected]
applied to
[email protected]
At path
node_modules/archiver
This warning is just to give you a heads-up. There is a small chance of
breakage even though the patch was applied successfully. Make sure the package
still behaves like you expect (you wrote tests, right?) and then run
patch-package archiver
to update the version in the patch file name and make this warning go away.
---
patch-package finished with 1 warning(s).
patches/archiver+6.0.2.patch
Outdated
| stat: true, | ||
| - dot: true | ||
| + dot: true, | ||
| + follow: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could consider using the same code from your node-archiver PR: archiverjs/node-archiver#810
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. I decided to use this simple approach because it will be discarded if there is a potential upgrade. But thinking about it, and related to this other PR #1448, it is more explicit to set the option from the calling code, i.e. archiver('zip', {followSymlinks: true}), and the changes in the calling code will reflect better why the symlinks are followed, so I will apply your suggested changes instead 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related issues
Proposed Changes
archiverthird party package so it follows the symlinks when archiving directoriesTesting Instructions
This can be tested using Studio or the CLI
npm installnpm startor if using the CLI runnpm run cli:watchwp-contentfolder, add some symlinks for eitherpluginsorthemes(although they can be on any other folder). For example, if you download the following repo to your~/githubfolder, you could add a symbolic link to it in yourpluginsfolder usingln -s ~/github/wp-data-layer-app-pages ~/Studio/my-symlinked-website/wp-content/plugins/wp-data-layer-app-pages(replacingmy-symlinked-websiteby your Studio site).node dist/cli/main.js preview create --path ~/Studio/my-symlinked-website/wp-admin/plugins.phpor navigate to Plugins -> Intalled PluginsWP Data Layer App Pagesand you should also seeMy Test Plugin, for example:Test broken symlinks
Pre-merge Checklist