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 realpath() failing on Windows. #5530

Closed
wants to merge 5 commits into from
Closed

Fix realpath() failing on Windows. #5530

wants to merge 5 commits into from

Conversation

AnrDaemon
Copy link
Contributor

In certain scenarios, realpath() failing to completely unwind all symlinks, resulting in Composer producing autogenerated mappings similar to this:

config(vendor-dir):C:\Users\anrdaemon\Documents\.github\1/vendor
$vendorPath:C:/home/Daemon/Documents/.github/1/vendor
$targetDir:C:/home/Daemon/Documents/.github/1/vendor/composer
realpath($targetDir):C:\Users\Daemon\Documents\.github\1\vendor\composer
$vendorPathCode:dirname(dirname(dirname(dirname(dirname(dirname(dirname(__DIR__))))))).'/home/Daemon/Documents/.github/1/vendor'

After a rather silly looking patch, it works much better:

config(vendor-dir):C:\Users\anrdaemon\Documents\.github\1/vendor
$vendorPath:C:/Users/Daemon/Documents/.github/1/vendor
$targetDir:C:/Users/Daemon/Documents/.github/1/vendor/composer
realpath($targetDir):C:\Users\Daemon\Documents\.github\1\vendor\composer
$vendorPathCode:dirname(__DIR__)

@Seldaek
Copy link
Member

Seldaek commented Sep 12, 2016

Thanks for the patch, but I do wonder.. is there a case where two realpath() is not enough and we need 3? or 4? Also is there a way to write a unit test for this? It'd be quite nice to avoid future regressions.

@Seldaek Seldaek added this to the 1.2 milestone Sep 12, 2016
@Seldaek Seldaek added the Bug label Sep 12, 2016
@staabm
Copy link
Contributor

staabm commented Sep 12, 2016

on which phpversion does this issue occur?

@AnrDaemon
Copy link
Contributor Author

As this is a bug in PHP itself, it MAY be possible.
Speaking of bugs,

  1. https://bugs.php.net/bug.php?id=72642
  2. https://bugs.php.net/bug.php?id=72738

Pick your one.

@AnrDaemon
Copy link
Contributor Author

@staabm , on all windows versions.

@AnrDaemon
Copy link
Contributor Author

My hope with this patch (yes, I've considered doing a heavier version of it accounting for possible multiple unresolved paths) is that there won't be many people overlinking their filesystems, as conditions for this issue to arise are rather specific.

@SoboLAN
Copy link

SoboLAN commented Oct 6, 2016

@AnrDaemon Maybe reference the link to this ticket in the code comment ?

@AnrDaemon
Copy link
Contributor Author

@SoboLAN , good idea, done.
@Seldaek , I'm not sure about unit tests. Perhaps, someone could scavenge the needed information from the reproduction code provided in https://bugs.php.net/bug.php?id=72738 ?

@Seldaek
Copy link
Member

Seldaek commented Oct 10, 2016

Oh sorry it seems I forgot to close this when I merged your commit. I merged as c774d41 and added the ref in 902a5c3

@Seldaek Seldaek closed this Oct 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants