fix(core): Check existence of cache symlink without usage of readlink() #10633
Conversation
|
Should we perhaps try both? Without |
|
@hypeJunction is the readlink() call necessary? I only backported 4ca717d which is already in 2.3 and master branch which already removed the readlink() call. I also found out what causes readlink() to fail now. readlink() (and also for example is_link()) fails to work due to the / at the end of the $link string. So, if readlink() should get added again this / must be removed both from the $link and $target string for it to work. |
|
That's odd that your file system can't tell |
|
Does it NOT work for you without the slash at the end? My guess is that the exact functionality of readlink might depend on the OS used. What OS are you testing it on and what version of php? In my case it's Linux on php 5.6. The readlink docs on php.net is of no help. But a comment posted on is_link() (at http://php.net/manual/en/function.is-link.php#72038) gives some indication that a similar issue occurs also with is_link() - where the parameter must be a filename. In case of is_link() it works in case the target of the link is a directory only without the trailing slash for this guy - and me. Again, my guess: it works not much different for readlink as it works for is_link(), i.e. only without the trailing slash. Is a trailing slash at the end of a full path of a filename (here linkname) even logical? If you give the full path to a directory it seems not to matter much. But in case of a filename the trailing slash seems wrong to me (or would you add a trailing slash when including a php file at the end of its name?). Maybe you are just lucky it works for you with the trailing slash. I can only say it doesn't work for me - without any scientific explanation - and Google was also not helpful in finding an explanation. Regardless, the current code in the 2.2 branch is already different from the code in 2.3 and master. So what now to get consistent in code at least? Backport the code from 2.3 (which would solve the problem also for me) or add readlink() again in 2.3 and master - resulting it not working anymore at least for me... |
|
Let's start by adding a test. If it fails on Travis which runs linux, we can take it from there and see where out logic is flawed |
|
Trying to add some tests here: #10637 |
|
Looking at the code now. We have already made that change in 2.3, so we can merge this as a bug fix. I will work on some tests for 2.3. |
|
In the future, please avoid using |
This is a backport from the 2.3 branch.
It's not only a matter of readlink() not being necessary. At least on my server the readlink() call produces an "Invalid argument" warning resulting in the check for existence of the cache symlink to fail even if the symlink is correctly set up.