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

feat: Add support for symlinks #865

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

Conversation

LuigiPulcini
Copy link

Concerning #683, the proposed changes are aimed at guaranteeing the subpath in the output directory is returning the correct values even when libraries are symlinked from a local location.

Since a Symfony Finder component returns files in an associative array where keys are the path of files always relative to the current project even when symlinks are followed, those keys can be effectively used to determine the subpath of the files in the output directory. The advantage is that there will not be any difference when the local library is replaced with the production one.

Please consider that, unlike what we discussed in #683, this first approach replaces the logic behind the current implementation. If this works, we could consider letting the two different strategies coexist through the appropriate configuration.

@theofidry
Copy link
Member

I'm happy it looks like small changes and could only get a quick look at the CI. My guess is that it has to do with the exclude-files, which are currently taken as realpath and I assume need to be adjusted in a similar fashion

@LuigiPulcini
Copy link
Author

Thanks again for taking action so promptly on this PR.

I initially thought that excluded files required special treatment but then I realized that both arrays are built using the same retrieveFilesWithContents() method here and here, so I assumed that changing that method would suffice to make both arrays compatible with the proposed change.

Admittedly, I haven't tested the changes in a scenario where files are excluded so I would take a closer look at this detail and make the appropriate adjustments.

@LuigiPulcini
Copy link
Author

The latest commit attempts to fix the issue with files listed in the exclude-files property. Since we have to assume that the list is not an associative array, $index should be replaced with $fileKey, determined as $file after replacing the leading $dirPath.DIRECTORY_SEPARATOR with an empty string.

@theofidry theofidry added this to the 1.0.0 milestone Nov 2, 2023
@theofidry
Copy link
Member

I think there is some work left to do here:

  • Add a case with symlinks to show that it works
  • ConsoleScoper::getFiles() appears to look for the common path to map the files and in one case (make e2e_027) this appears to not work (which kinda makes sense, but then this common path logic needs to be adapted).

@theofidry theofidry changed the title Changed output subpath logic feat: Add support for symlinks Nov 4, 2023
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.

2 participants