Skip to content
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

fix(copy-files): use context realpath #640

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(copy-files): use context realpath #640

wants to merge 2 commits into from

Conversation

QWp6t
Copy link

@QWp6t QWp6t commented Sep 17, 2019

In scenarios where context is within a symlink, path.relative() will return an undesired path because the resource path follows symlinks. We resolve this by using fs.realpathSync(options.context).

In scenarios where context is within a symlink, `path.relative()` will return an undesired path because the resource path follows symlinks. We resolve this by using `fs.realpath(context)`.
@stof
Copy link
Member

stof commented Sep 17, 2019

don't you need to do this both for the resource path and the content though ?

@QWp6t
Copy link
Author

QWp6t commented Sep 18, 2019

In my testing, which was not very extensive at all, the symlink had already been followed for the resourcePath. I didn't investigate where it gets it, though, or if there are any caveats.

At any rate, I can do the same for the resource path too if you want.

Copy link
Collaborator

@Lyrkan Lyrkan left a comment

Choose a reason for hiding this comment

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

Hi @QWp6t,

Thank you for noticing the issue and working on that PR!
Could you maybe add some functional tests using symlinks?

@weaverryan
Copy link
Member

Could you maybe add some functional tests using symlinks?

Yes, this would be great! It can be added in this section - probably we could just create a symlink at the start of the test, make sure that file is copied, then clean up the symlink after. Can you try that @QWp6t?

Thanks!

https://github.com/symfony/webpack-encore/blob/master/test/functional.js#L1753

@QWp6t
Copy link
Author

QWp6t commented Oct 9, 2019

Hey guys, just wanted to stop by to mention I haven't forgotten about this PR. Yes, I can add some tests, but I have a deadline coming up, so it'll have to wait a bit. I think you guys have access to push to my fork if you're wanting to fast-track this for whatever reason, otherwise I'll make the changes you requested just as soon as I'm finished with work, probably end of next week.

Have a good week! 🍻

@weaverryan weaverryan changed the base branch from master to main November 18, 2020 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants