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

[Filesystem] fix readlink() for Windows #40866

Merged

Conversation

a1812
Copy link
Contributor

@a1812 a1812 commented Apr 19, 2021

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets
License MIT
Doc PR

How to reproduce

Windows 10.0.19042.928, PHP 8.0.3, PHPUnit 9.5.4
run as Administrator
C:\php\php.exe ./phpunit --bootstrap ./vendor/autoload.php --configuration ./phpunit.xml.dist ./src/Symfony/Component/Filesystem/Tests

There were 2 failures:

  1. Symfony\Component\Filesystem\Tests\FilesystemTest::testRemoveCleansInvalidLinks
    Failed asserting that 'C:\Users\albat\AppData\Local\Temp\1618836823.005.2057903605\directory\dir' is false.

D:\Z__PHP_PROJECT\symfony\src\Symfony\Component\Filesystem\Tests\FilesystemTest.php:379

  1. Symfony\Component\Filesystem\Tests\FilesystemTest::testReadAbsoluteLink
    Failed asserting that two strings are equal.
    --- Expected
    +++ Actual
    @@ @@
    -'C:\Users\albat\AppData\Local\Temp\1618836823.1681.131301953\dir\link'
    +'C:\Users\albat\AppData\Local\Temp\1618836823.1681.131301953\file'

@carsonbot carsonbot added this to the 4.4 milestone Apr 19, 2021
@a1812 a1812 changed the title fix FilesystemTest for Windows [Filesystem] fix FilesystemTest for Windows Apr 19, 2021
@a1812 a1812 force-pushed the fix_test_FilesystemTest_for_windows branch from 98bd15f to 2ca8c35 Compare April 20, 2021 11:22
@a1812
Copy link
Contributor Author

a1812 commented Apr 20, 2021

I'm not sure if skipping a test is a good idea, but the green bars clearly improve my mood :)

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Apr 21, 2021

:D
What changed in PHP 7.3? The Filesystem component is supposed to provide one consistent behavior for all versions and OSes. We might want to decide which behavior is the correct one and fix it.

@a1812
Copy link
Contributor Author

a1812 commented Apr 21, 2021

What changed in PHP 7.3?

<?php

// check behavior functions realpath, readlink, and component Symfony Filesystem for windows

require_once __DIR__ . '/../vendor/autoload.php';

use Symfony\Component\Filesystem\Filesystem;

$dir = __DIR__ . DIRECTORY_SEPARATOR . 'dir';
mkdir($dir);

$file = $dir . DIRECTORY_SEPARATOR . 'file';
$link1 = $dir . DIRECTORY_SEPARATOR . 'link1';
$link2 = $dir . DIRECTORY_SEPARATOR . 'link2';

touch($file);

symlink($file, $link1);
symlink($link1, $link2);

$fs = new Filesystem();

echo 'PHP: ' . PHP_VERSION . PHP_EOL;
echo 'realpath: ' . realpath($link2) . PHP_EOL;
echo 'readlink: ' . readlink($link2) . PHP_EOL;
echo '$fs->readlink($link2, true): ' . $fs->readlink($link2, true) . PHP_EOL;
echo '$fs->readlink($link2): ' . $fs->readlink($link2) . PHP_EOL;

unlink($file);
unlink($link1);
unlink($link2);
rmdir($dir);

b - before patch, a - after patch

PHP realpath readlink (b)$fs->readlink ($link2,true) (b)$fs-> readlink ($link2) (a)$fs->readlink ($link2,true) (a)$fs->readlink ($link2)
8.0.* file link1 file file file link1
7.4.18 - 7.4.10 file link1 file file file link1
7.4.9 - 7.4.0 link1 link1 file link1 file link1
7.3.28 - 7.3.22 file file file file file file
7.3.21 - 7.3.2 link1 file file link1 file link1
7.3.1 - 7.3.0 file file file file file file
7.2.* link1 file file link1 file link1
7.1.3 - 7.1.33 link1 file file link1 file link1

###conclusion:

What to do with PHP 7.3.* ?

Correct me, maybe I am confusing or missing something.

@a1812
Copy link
Contributor Author

a1812 commented Apr 22, 2021

how check

set PROJECT_DIR=C:\symfony
set PHPUNIT=%PROJECT_DIR%/phpunit^
    --colors=never^
    --bootstrap %PROJECT_DIR%/vendor\autoload.php^
    --configuration %PROJECT_DIR%/phpunit.xml.dist %PROJECT_DIR%/src/Symfony/Component/Filesystem

FOR %%V IN (
    7.1.3,
    7.1.33,
    7.2.15,
    7.2.34,
    7.3.0,
    7.3.15,
    7.3.27,
    7.3.28,
    7.4.0,
    7.4.9,
    7.4.10,
    7.4.15,
    7.4.16,
    7.4.18,
    8.0.0,
    8.0.1,
    8.0.2,
    8.0.3,
    8.0.5,
    8.0.6
) DO (
    c:\php\%%V\php %PHPUNIT%
    @if errorlevel 1 (
       exit /b
    )
)

@a1812 a1812 changed the title [Filesystem] fix FilesystemTest for Windows [Filesystem] fix Filesystem for Windows Apr 23, 2021
@a1812 a1812 force-pushed the fix_test_FilesystemTest_for_windows branch from 51a0b9c to 76358d0 Compare April 23, 2021 09:19
@a1812
Copy link
Contributor Author

a1812 commented Apr 23, 2021

Hi @nicolas-grekas, please check my code.

@a1812
Copy link
Contributor Author

a1812 commented Apr 23, 2021

imho need to edit the description, the current one is misleading

"... On Windows systems, readlink() resolves recursively the children links of a link until a final target is found. ... "

https://symfony.com/doc/4.4/components/filesystem.html#readlink

@a1812 a1812 force-pushed the fix_test_FilesystemTest_for_windows branch from 76358d0 to bd5861f Compare April 24, 2021 13:00
@a1812
Copy link
Contributor Author

a1812 commented Apr 27, 2021

Bug report for PHP7.3.27 https://bugs.php.net/bug.php?id=80993

@a1812
Copy link
Contributor Author

a1812 commented Apr 27, 2021

Answer from php.net

The erroneous readlink() has apparently been fixed inadvertently
when php_sys_readlink() has been refactored[1].

Anyhow, PHP 7.3 is in security mode[2], so unless this would be
regarded as security issue, the fix will not be backported.

[1] <https://github.com/php/php-src/commit/91c905e83c338ef66da824be4f90c1d78d134507>
[2] <https://www.php.net/supported-versions.php>

I think we did everything we could (fixed for 8.0 and >=7.4.10) for behavior our component

@a1812 a1812 force-pushed the fix_test_FilesystemTest_for_windows branch from bd5861f to f1b95d3 Compare April 30, 2021 08:44
@a1812 a1812 changed the title [Filesystem] fix Filesystem for Windows [Filesystem] fix readlink() for Windows May 11, 2021
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

(what a mess :) )

@nicolas-grekas
Copy link
Member

Thank you @a1812.

@nicolas-grekas nicolas-grekas merged commit 54bee29 into symfony:4.4 May 26, 2021
javiereguiluz added a commit to javiereguiluz/symfony-docs that referenced this pull request May 28, 2021
This PR was merged into the 4.4 branch.

Discussion
----------

fix readlink description

remove description readlink() for Windows misleading
see symfony/symfony#40866

Commits
-------

a2dd7ee fix readlink description
This was referenced May 31, 2021
@a1812 a1812 deleted the fix_test_FilesystemTest_for_windows branch July 21, 2021 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants