Studio: Follow symlinks when exporting site #1427

epeicher · 2025-05-22T16:34:24Z

Related issues

Solves STU-309

Proposed Changes

Add a getSymlinks function that returns recursively all the symlinks of a folder
Avoid adding symlinks when adding files to compress using archive.directory
Add the symlink real paths to the archiver zip file as an additional step

Testing Instructions

This can be tested using Studio or the CLI

Apply this branch and run npm start or npm run cli:watch
Create a site and inside the wp-content folder, add some symlinks for either plugins or themes (although they can be on any other folder). For example, if you download the following repo to your ~/github folder, you could add a symbolic link to it in your plugins folder using ln -s ~/github/wp-data-layer-app-pages ~/Studio/my-symlinked-website/wp-content/plugins/wp-data-layer-app-pages (replacing my-symlinked-website by your Studio site).
On the Studio app, navigate to Previews and create a preview site or on a terminal, open the root folder of the studio repo and run node dist/cli/main.js preview create --path ~/Studio/my-symlinked-website
Once created, navigate to the preview site, and open /wp-admin/plugins.php or navigate to Plugins -> Intalled Plugins
Check you can see the symlinked plugin, if you followed the example, you should see WP Data Layer App Pages

Optional Suggestion: Apart from running one E2E test as explained above, for faster iterations to run multiple different tests, you can see the commit 797a39b that includes a test.ts command that can be used to only run the zip step. You can then check that the zip contains the expected symlinks resolved to their target paths. That test can be executed by node dist/cli/main.js preview test --path ~/Studio/my-symlinked-website

Pre-merge Checklist

Have you checked for TypeScript, React or other console errors?

cli/commands/preview/test.ts

cli/index.ts

cli/lib/archive.ts

bcotrim

Code LGTM.
Added a minor comment, but it shouldn't be a blocker.

Testing instructions worked as described. Something I found during tests that might be an issue: I tried to use the export site functionality but noticed the symlinked plugin isn't included. Creating a preview site works as expected (the symlinked plugin is visible and functional).
I'm not sure if this is the expected behavior, if it is consider the PR approved.

cli/lib/archive.ts

fredrikekelund

Thanks for tackling this! This PR works as expected 👍

I think there's an opportunity to simplify the logic for iterating over the wp-content directory (per my inline comment), but that's only a suggestion. Feel free to merge the current changes if you prefer, @epeicher.

cli/lib/archive.ts

epeicher · 2025-05-27T15:23:48Z

@bcotrim, thanks for your review!

Something I found during tests that might be an issue: I tried to use the export site functionality but noticed the symlinked plugin isn't included.

Well spotted! The export functionality is slightly different than creating the preview archive, it uses the default-exporter.ts. So I think it will be better to tackle that as part of a follow-up task.

Also, please note that I've changed the approach for this PR as suggested here, so the code is now slightly different. Happy to have another round of review.

bcotrim · 2025-05-28T07:58:14Z

@epeicher while re-reviewing the changes, I tested a new scenario with a broken symlink in the plugins folder.
I created the synlink as usual, confirmed it was working, and then moved the target file.
The preview command now fails

Is this the expected behavior?
To me it seems like ideally, we would want to proceed with the preview site creation since their WP site isn't broken and give our user a warning.
What do you think?

epeicher · 2025-05-28T08:27:09Z

I tested a new scenario with a broken symlink in the plugins folder.
I created the synlink as usual, confirmed it was working, and then moved the target file.
The preview command now fails

Is this the expected behavior?
To me it seems like ideally, we would want to proceed with the preview site creation since their WP site isn't broken and give our user a warning.
What do you think?

@bcotrim that is a very good point, to cover the errors and edge cases that the users will face for sure. The current implementation assumes the links point to correct targets, but it can be updated, so when the target does not exist, symlinks can be ignored. I think this is better than the production behavior, which adds the symlinks (which could be broken), what do you think?

bcotrim · 2025-05-28T09:16:39Z

@bcotrim that is a very good point, to cover the errors and edge cases that the users will face for sure. The current implementation assumes the links point to correct targets, but it can be updated, so when the target does not exist, symlinks can be ignored. I think this is better than the production behavior, which adds the symlinks (which could be broken), what do you think?

That sounds good to me!

fredrikekelund

Thanks for considering my suggestion, @epeicher. I realized that I got confused about the behavior of readdir when passing just recursive compared to when withFileTypes is also passed. I left a suggestion for how we could simplify this even further by using only the recursive option.

fredrikekelund · 2025-05-28T09:13:35Z

cli/lib/archive.ts

+	const wpContentFolder = path.join( siteFolder, 'wp-content' );
+	const directoryContents = await fsPromises.readdir( wpContentFolder, {
+		recursive: true,
+		withFileTypes: true,


Suggested change

withFileTypes: true,

Sorry for getting this wrong in my previous comment, but I see now that readdir only resolves symlinks if recursive: true and withFileTypes: false.

I suggest simplifying the logic by removing withFileTypes.

Oh, that's a very key point. I tested that to check if readdir followed symlinks, but didn't think about changing withFileTypes to false. Let me do a quick iteration as I think it will simplify the code a lot.

fredrikekelund · 2025-05-28T09:16:58Z

cli/lib/archive.ts

+		for ( const dirEnt of directoryContents ) {
+			const filePath = path.join( dirEnt.parentPath, dirEnt.name );
+			if ( shouldExcludeEntry( filePath ) ) {
+				continue;
 			}
-		);

+			const archivePath = path.relative( siteFolder, filePath );
+			if ( dirEnt.isSymbolicLink() ) {
+				const realPath = fs.realpathSync( filePath );
+				const stat = fs.statSync( realPath );
+				const isDirectory = stat.isDirectory();
+				if ( isDirectory ) {
+					archive.directory( realPath, archivePath, ( entry: EntryData ) => {
+						if ( shouldExcludeEntry( entry.name ) ) {
+							return false;
+						}
+						return entry;
+					} );
+				} else {
+					archive.file( realPath, { name: archivePath } );
+				}
+			} else if ( dirEnt.isFile() ) {
+				archive.file( filePath, { name: archivePath } );
+			}
+		}


for ( const entryPath of directoryContents ) { if ( entryPath.includes( '.git' ) || entryPath.includes( 'node_modules' ) ) { continue; } const absolutePath = path.join( wpContentFolder, entryPath ); const stat = fs.statSync( absolutePath ); if ( stat.isFile() ) { const archivePath = path.relative( siteFolder, absolutePath ); archive.file( absolutePath, { name: archivePath } ); } }

Without the withFileTypes option to readdir, the recursive option is much more useful, because it also resolves symlinks.

Here's a suggestion for how we could simplify this loop more along the lines of what I had in mind to begin with.

Indeed, let me apply that or a similar one and test.

Thanks for the suggestion @fredrikekelund! I have applied a similar approach but I've needed to check for symlinks that point to files as they are not followed by readdir, you can see my changes in this commit a1ea258. I have added a try..catch to handle errors if the path does not exist. ~~That can be always extracted to an external function that returns the filePath if it exists.~~ Extracted to an external function in 683e81d

fredrikekelund · 2025-05-28T09:20:02Z

I believe my latest suggestion should also fix the issue @bcotrim raised about broken symlinks.

epeicher · 2025-05-28T10:43:21Z

I believe my latest suggestion should also fix the issue @bcotrim raised about broken symlinks.

The suggestion was great, but I have needed to follow symlinks using realpathSync because the target files are not returned by readdir for symlinked files. I am also checking errors for broken symlinks and handling them gracefully in here.

@bcotrim , @fredrikekelund , this is ready for another review 👀

fredrikekelund

I have needed to follow symlinks using realpathSync because the target files are not returned by readdir for symlinked files

Good catch 👍

This is looking great and works as expected!

bcotrim

LGTM 👍

Studio: Follow symlinks when exporting site

797a39b

epeicher commented May 22, 2025

View reviewed changes

cli/commands/preview/test.ts Outdated Show resolved Hide resolved

epeicher commented May 22, 2025

View reviewed changes

cli/index.ts Outdated Show resolved Hide resolved

epeicher added 2 commits May 23, 2025 09:53

Add symbolic link directories as well as files

00228b5

Remove test files

591e7da

epeicher commented May 23, 2025

View reviewed changes

cli/lib/archive.ts Outdated Show resolved Hide resolved

Remove unnecesary code, add some comments

1d98a03

epeicher marked this pull request as ready for review May 23, 2025 09:10

epeicher requested a review from a team May 23, 2025 09:13

wojtekn assigned epeicher May 26, 2025

bcotrim reviewed May 26, 2025

View reviewed changes

cli/lib/archive.ts Outdated Show resolved Hide resolved

fredrikekelund approved these changes May 27, 2025

View reviewed changes

cli/lib/archive.ts Show resolved Hide resolved

epeicher added 3 commits May 27, 2025 11:11

Avoid returning symlinks for excluded entries

5ac3d81

Follow symlinks when creating preview

354c0b3

Exclude git and node_modules from symlinked directories

4d43207

Update tests

4a99ab1

fredrikekelund reviewed May 28, 2025

View reviewed changes

epeicher added 4 commits May 28, 2025 12:03

Follow symlinks directories from readdir to simplify logic

a1ea258

Fix tests after latest updates

84527c3

Extract the get path logic to an external function

683e81d

Update comment and rename function for clarity

d603b33

fredrikekelund approved these changes May 28, 2025

View reviewed changes

bcotrim approved these changes May 28, 2025

View reviewed changes

epeicher added the DO NOT MERGE label May 28, 2025

Studio: Follow symlinks when exporting site #1427

Are you sure you want to change the base?

Studio: Follow symlinks when exporting site #1427

Uh oh!

Conversation

epeicher commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related issues

Proposed Changes

Testing Instructions

Pre-merge Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bcotrim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fredrikekelund left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

epeicher commented May 27, 2025

Uh oh!

bcotrim commented May 28, 2025

Uh oh!

epeicher commented May 28, 2025

Uh oh!

bcotrim commented May 28, 2025

Uh oh!

fredrikekelund left a comment

Choose a reason for hiding this comment

Uh oh!

fredrikekelund May 28, 2025

Choose a reason for hiding this comment

Uh oh!

epeicher May 28, 2025

Choose a reason for hiding this comment

Uh oh!

fredrikekelund May 28, 2025

Choose a reason for hiding this comment

Uh oh!

epeicher May 28, 2025

Choose a reason for hiding this comment

Uh oh!

epeicher May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredrikekelund commented May 28, 2025

Uh oh!

epeicher commented May 28, 2025

Uh oh!

fredrikekelund left a comment

Choose a reason for hiding this comment

Uh oh!

bcotrim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

epeicher commented May 22, 2025 •

edited

Loading

epeicher May 28, 2025 •

edited

Loading