Skip to content

Resolve relative symlink paths correctly #1009

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.

Sign up for GitHub

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

Merged
fredrikekelund merged 4 commits into from
Mar 5, 2025
Merged

Resolve relative symlink paths correctly #1009

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8c66854
Resolve relative symlink paths correctly
fredrikekelund Mar 5, 2025
65eae59
Skip mounting symlinks where target is already inside document root
fredrikekelund Mar 5, 2025
a77c724
Fix
fredrikekelund Mar 5, 2025
a81c93f
Fix tests and remove isTargetInsideDocumentRoot
fredrikekelund Mar 5, 2025
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions src/lib/symlink-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,10 +139,10 @@ export class SymlinkManager {
* @param filename
*/
private async addSymlink( filename: string ) {
const fullPath = path.join( this.projectPath, filename );
let target: string;
const fullPath = path.resolve( this.projectPath, filename );
let hostTarget: string;
try {
target = await fs.realpath( fullPath );
hostTarget = await fs.realpath( fullPath );
} catch ( err ) {
if ( isErrnoException( err ) && err.code === 'ENOENT' ) {
console.error( `Symlink target does not exist: ${ fullPath }` );
Expand All @@ -152,18 +152,19 @@ export class SymlinkManager {
return;
}

const vfsPath = path.posix.join( this.documentRoot, filename );
const vfsPath = path.posix.resolve( this.documentRoot, filename );

// Double check to ensure the symlink exists
if ( ! this.php.fileExists( vfsPath ) ) {
return;
}

// Get the realpath of the symlink within the PHP runtime.
const vfsTarget = this.php.readlink( vfsPath );
const vfsTarget = path.posix.resolve( path.dirname( vfsPath ), this.php.readlink( vfsPath ) );

@fredrikekelund fredrikekelund Mar 5, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

php.realpath does not accomplish the same thing (confirmed in my testing). I believe it just returns false because the target path does not yet exist at this point.

@fredrikekelund fredrikekelund Mar 5, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

To expand on this: realpath works as a more compact way of expressing this logic if the target path is already within the document root. If this is not the case, realpath returns false because the target does not yet exist (see PHP docs).

The SymlinkManager::isTargetInsideDocumentRoot added in this PR stems from the realization that if the target path is already within the document root, there's no need for SymlinkManager to intervene. This is a performance optimization that should help speed up server starts for sites with many symlinks.

@fredrikekelund fredrikekelund Mar 5, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Following up about SymlinkManager::isTargetInsideDocumentRoot: I realized that SymlinkManager::mountTarget already has this performance optimization in that it checks if the file or directory already exists in the Playground FS before attempting to mount it. Hence, I removed this method.

this.symlinks.set( filename, vfsTarget );

await this.mountTarget( vfsTarget, target );
await this.mountTarget( vfsTarget, hostTarget );
}

/**
Expand Down Expand Up @@ -195,6 +196,9 @@ export class SymlinkManager {
vfsTarget,
createNodeFsMountHandler( hostTarget )
);
// For some reason, we must access the target directory after mounting it, otherwise the
// mount will not work.
this.php.listFiles( vfsTarget );

@fredrikekelund fredrikekelund Mar 5, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@bgrgicak @adamziel @ashfame, I'm not sure why this is needed, but the Yoast SEO test case (see PR description) didn't work without it in my testing. Any idea why that's the case..?

@wojtekn wojtekn Mar 5, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It continues to work for me when I comment out this line. Maybe it depends on the underlying filesystem and we use different ones?

@fredrikekelund fredrikekelund Mar 5, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

To quickly double-check: did you restart the entire app after commenting out this line?

@sejas sejas Mar 5, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was able to mount and start a site with wp-data-layer-app-pages plugin without listing the files.

@fredrikekelund fredrikekelund Mar 5, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the wp-data-layer-app-pages test case works without this line for me, too. It's only the Yoast test case that was troublesome

@fredrikekelund fredrikekelund Mar 5, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We may find that this line is unnecessary after further testing, but I have a hard time seeing how it would break anything, so I'll land the PR with this included and we can keep the discussion going after merging.
}

/**
Expand Down
1 change: 1 addition & 0 deletions src/lib/tests/symlink-manage.test.ts → src/lib/tests/symlink-manager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ describe( 'SymlinkManager', () => {
isDir: jest.fn(),
unlink: jest.fn(),
rmdir: jest.fn(),
listFiles: jest.fn(),
requestHandler: {
documentRoot: mockDocumentRoot,
},
Expand Down