@pierrejoye The email address in the header of ext/standard/link_win32.c should be changed to pajoye@.

Hm, but I don't think this really is a BC break? With the old code, you were simply unable to create relative symlinks under Windows when using PHP's symlink function. This just fixes it, so that it becomes possible under Windows as well - any existing code using symlink shouldn't be affected by it.

Thanks for the patch!

Ok for 7, big no for 5.x, BC

why do you want to change then email? They are aliases and work just fine.

@fritzmg

It is incorrect to to say that relative links are impossible. See the doc, it is even documented how they work: relative to the current cwd.

Now, to break it to make code more portable is fine, just not in 5.x

@pierrejoye which documentation are you referring to?

I didn't mean that relative symlinks are impossible in general, just that they cannot be created with PHP's symlink function under Windows due to this bug.

@pierrejoye I wasn't aware that there are email aliases. Sorry for the noise.

@fritzmg It is currently possible to create working symlinks on Windows when giving a relative path, at least when the symlink is in the same folder as its target:

symlink('foo.txt', 'bar.txt');

Not sure if anybody uses that, but you never know.

@cmb69 actually, the target would have to be in the CWD, otherwise your example won't work currently (on Windows) - and the source too, otherwise the symlink will be invalid.

Oh and something else: even if the target and the source are in the CWD of PHP, it might still not work, depending on how the PHP process is started. The CWD of the Win32API function GetFileAttributes will be the directory from which the root process has been started and is thus not necessarily the same as the CWD defined by PHP. In case of a Windows + XAMPP setup, the CWD for the Win32API will be C:\xampp\, since you typically start the webserver by executing C:\xampp\xampp_start.exe. Using chdir(…) in PHP will have no effect on that (at least according to my tests).

@cmb69 the msdn doc for createlink, it specifically refers that a relative
target is relative to CWD. The check was added to give a friendlier error.
On Apr 18, 2015 11:32 PM, "fritzmg" notifications@github.com wrote:

@cmb69 https://github.com/cmb69 actually, the target would have to be
in the CWD, otherwise your code won't work currently (on Windows) - and the
source too, otherwise the symlink will be invalid.

—
Reply to this email directly or view it on GitHub
#1243 (comment).

@pierrejoye but it does not say that: https://msdn.microsoft.com/en-us/library/windows/desktop/aa363878%28v=vs.85%29.aspx

You can create a relative symlink in 4 different ways:

• Relative: ..\foo - links to foo in the source's parent directory
• Current directory: foo - links to foo in the source's directory
• Root relative: \foo - links to foo in the source's root directory
• CWD relative: C:foo - links to foo in the current working directory, if the current working directory is somewhere within C: - the actual symlink will however be set as an absolute one

Maybe we should un-skip some of those tests? Changing code behavior without having test to verify (or explain) what is the intended one looks wrong to me.

We run them, and there is a good reason for this check.

I will test the PR with all systems and post the results. DFS, shares and
the likes are tricky with that. But I am really not keen to change that in
5.x.
On Apr 19, 2015 7:26 AM, "Stanislav Malyshev" notifications@github.com
wrote:

Maybe we should un-skip some of those tests? Changing code behavior
without having test to verify (or explain) what is the intended one looks
wrong to me.

—
Reply to this email directly or view it on GitHub
#1243 (comment).

It appears that the MSDN man page on CreateSymbolicLink is not particularly clear on how relative links will be interpreted. Looking at the 4 items, however, it becomes clear that relative paths are resolved relative to the symlink, unless a special syntax (C:, not to be confused with C:\\) is used to treat them relative to the CWD when the link is created. This special syntax currently (e.g. PHP 5.6.8) works, but will not work with this PR. ("Root relative" links work neither with or without the PR.) Therefore, and because what has already said above, it seems to be best to stick with the current behavior for PHP 5.x.

Changing the behavior for PHP 7 to be more inline with the way it works on other architectures seems to be appropriate, though. Consider a most basic test. This works under POSIX (tested with Cygwin), and on Windows with this PR but not without it. It's probably good to add a respective PHPT.

With regard to the skipped PHPTs on Windows: there are some which have Win only counterparts (e.g. 001.phpt) and some which would have to be modified because they're using hard-coded /tmp/ paths (e.g. bug39367.phpt).

In any way care must be taken to check for SeCreateSymbolicLinkPrivilege (which is not necessarily granted even for admin accounts) as it's done for e.g. bug47767.phpt.

Yes, we do some trickery to do it and avoid to get false positive reports.

But again, I am ok for 7, given that it does create more troubles than what
it tried to solve. It is much more test required than only local FS :)
On Apr 19, 2015 6:51 PM, "Christoph M. Becker" notifications@github.com
wrote:

It appears that the MSDN man page on CreateSymbolicLink
https://msdn.microsoft.com/de-de/library/windows/desktop/aa363866(v=vs.85).aspx()
is not particularly clear on how relative links will be interpreted.
Looking at the 4 items, however, it becomes clear that relative paths are
resolved relative to the symlink, unless a special syntax (C:, not to be
confused with C:) is used to treat them relative to the CWD when the
link is created. This special syntax currently (e.g. PHP 5.6.8) works, but
will not work with this PR. ("Root relative" links work neither with or
without the PR.) Therefore, and because what has already said above, it
seems to be best to stick with the current behavior for PHP 5.x.

Changing the behavior for PHP 7 to be more inline with the way it works on
other architectures seems to be appropriate, though. Consider a most
basic test https://gist.github.com/cmb69/b6c26ec22db29822ca00. This
works under POSIX (tested with Cygwin), and on Windows with this PR but not
without it. It's probably good to add a respective PHPT.

With regard to the skipped PHPTs on Windows: there are some which have Win
only counterparts (e.g. 001.phpt
https://github.com/php/php-src/blob/master/ext/standard/tests/file/001.phpt)
and some which would have to be modified because they're using hard-coded
/tmp/ paths (e.g. bug39367.phpt
https://github.com/php/php-src/blob/master/ext/standard/tests/file/bug39367.phpt
).

In any way care must be taken to check for SeCreateSymbolicLinkPrivilege
(which is not necessarily granted even for admin accounts) as it's done for
e.g. bug47767.phpt
https://github.com/php/php-src/blob/master/ext/standard/tests/file/bug47767.phpt
.

—
Reply to this email directly or view it on GitHub
#1243 (comment).

This special syntax currently (e.g. PHP 5.6.8) works

It only works, if the working directory of the parent process (or the process that invoked the running process) under which the PHP binary is executed happens to be the same as the working directory of the PHP script, due to the issues with the working directory I mentioned. Assume you are running XAMPP on Windows and you have the following script under C:\xampp\htdocs\symlinktest\index.php:

<?php

mkdir('foo1');
mkdir('foo2');
file_put_contents('foo2/bar.txt', '42');

var_dump(getcwd());

symlink('C:foo2/bar.txt', 'foo1/foo3.txt');

var_dump(file_get_contents('foo1/foo3.txt'));

It will still result in

string(27) "C:\xampp\htdocs\symlinktest"
Warning: symlink(): Could not fetch file information(error 3) in C:\xampp\htdocs\symlinktest\index.php on line 9

Warning: file_get_contents(foo1/foo3.txt): failed to open stream: No such file or directory in C:\xampp\htdocs\symlinktest\index.php on line 11
bool(false) 

if you:

• start the webserver using C:\xampp\xamp_start.exe
• run the script via the webserver (e.g. by opening http://localhost/symlinktest)

Though I have not tested if the Win32API working directory is different, when PHP is running via CGI instead of an Apache module.

IMHO it is not as simple as it is done. Also with the regard to the thread safety. Maybe an algo like this could be more reliable:

• always resolve the target path to the absolute path
• leave the link path untouched
• if the link path is relative, check whether it resolves against the target path

Maybe one could also memorize the _getcwd right at the start of the symlink() call so the _chdir() is not suddenly changed, that were another solution respecting thread safety. Would probably require locking, that would be probably the best.

Thanks.

@fritzmg Apache does chdir() to it's root directory, so it's not htdocs. That's probably a topic for another ticket, if you'd be a spot to create it. We could see whether we can chdir() to the correct htdocs from the PHP module.

Thanks.

@fritzmg it is critical to continue this discussion that you understand the limitations and caveats of symbolic on Windows. They are not, by many meaning, fully compatible with POSIX (if that even means anything anymore) or more specifically linux.

I implemented this part, @weltling and other spent days to debug and fix edge cases in common scenarios like using symlinks on DFS, cross devices and all these scenarios we can see on sharing hosters or Cloud.

Taking one simple example as a go to remove this check is not a good move. As I said earlier, we need to valid for every scenario we have in our infrastructures. If everything passes, we can apply, if not, we need to find another solution, if any.

@fritzmg also the CWD in a TS SAPI is playing a role here, totally unrelated to this discussion but this is something different from what you may have in NTS. Read: there is no such thing than CWD but php does its own machinery to get CWD based on the current request. Just for the record :)

@fritzmg it is critical to continue this discussion that you understand the limitations and caveats of symbolic on Windows. They are not, by many meaning, fully compatible with POSIX (if that even means anything anymore) or more specifically linux.

Yes, I understand that in general symlinks are not fully compatible between POSIX and Windows and actual symlinks cannot be copied between these systems. However, the syntax and semantics of creating relative symlinks are compatible. A relative symlink created with the same parameters in their respective functions on both systems create the same result. e.g.

ln -s "../foo2/bar.txt" "foo1/foo3.txt"

on Linux and

mklink "foo1/foo3.txt" "../foo2/bar.txt"

on Windows.

PHP frameworks like Symfony currently work around this issue by creating absolute symbolic links, if the creation of the relative symbolic link fails.

And this fallback must remain. No matter if this change will happen or
not.
On Apr 20, 2015 5:09 PM, "fritzmg" notifications@github.com wrote:

@fritzmg https://github.com/fritzmg it is critical to continue this
discussion that you understand the limitations and caveats of symbolic on
Windows. They are not, by many meaning, fully compatible with POSIX (if
that even means anything anymore) or more specifically linux.

Yes, I understand that in general symlinks are not fully compatible
between POSIX and Windows and actual symlinks cannot be copied between
these systems. However, the syntax and semantics of creating relative
symlinks are compatible. A relative symlink created with the same
parameters in their respective functions on both systems create the same
result. e.g.

ln -s "../foo2/bar.txt" "foo1/foo3.txt"

on Linux and

mklink "foo1/foo3.txt" "../foo2/bar.txt"

on Windows.

PHP frameworks like Symfony currently work around this issue by creating
absolute symbolic links, if the creation of the relative symbolic link
fails.

—
Reply to this email directly or view it on GitHub
#1243 (comment).

@pierrejoye I fully agree that the PR has to be carefully checked for more complex scenarios than simple local FS. I can't be of much help there, unfortunately, due to missing infrastructure.

@weltling I understand that there may be threading issues wrt. symlink(). However, it seems to me that the code currently is in error wrt. the comment "The target is relative to the link itself, not to the CWD.". That is true, but a few lines above the target (topath) is checked relative to the CWD. Am I missing something?

@cmb69,

I was checking the flow more carefully yet. The target and link paths are always expanded using the CWDG, so no dependency on the CRT. Still CreateSymbolicLink() regarding relative paths depends on the current working dir in CRT. This together means - disregarding whether the target is expanded to the full path or not, the link being relative is an issue.

In the current state where relative links are blocked, there is no issue. When enabling relative links, TS mode needs to ensure that the current working dir in the CRT is the same as in PHP. Your patch shows (and should be OK) no regression with CLI (despite needs more extensive testing) while enabling relative symlinks, however it will cause weird things with a real TS SAPI.

You can check what I'm saying creating a simple snippet with some threaded program using _chdir() and CreateSymbolicLink(), there are templates on MSDN. Also, you can test basic network things creating network shares on the local machine.

Thanks.

@weltling Thanks for the information. I'll try to have a look at the issues within the next days.

Since this did not land in php7, what would be the earliest version where this fix could be introduced?

Hm, that's a shame, that this fix did not make it into PHP 7 - they didn't want to include it in PHP 5 due to BC.

I wouldn't say the patch should be applied as is, it would enable any weird errors with relative paths for symlinks. Symlink can only be created by using system APIs, and those are not thread safe. Note also that it's not Windows only issue.

Thanks.

Since our windows people have rejected this as it is, and since it has merge conflicts, and since the author seems to have abandoned working on it, I'm closing this PR.

Please take this action as encouragement to open a clean PR, that resolves the issues raised here, also get Anatol involved in the conversation again if you do so.

@krakjoe where or why was it rejected?

#1243 (comment)