Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Filesystem] fix readlink() for Windows#40866

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@a1812
Copy link
Contributor

QA
Branch?4.4
Bug fix?yes
New feature?no
Deprecations?no
Tickets
LicenseMIT
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'

@carsonbotcarsonbot added this to the4.4 milestoneApr 19, 2021
@a1812a1812 changed the titlefix FilesystemTest for Windows[Filesystem] fix FilesystemTest for WindowsApr 19, 2021
@a1812a1812force-pushed thefix_test_FilesystemTest_for_windows branch from98bd15f to2ca8c35CompareApril 20, 2021 11:22
@a1812
Copy link
ContributorAuthor

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 commentedApr 21, 2021•
edited
Loading

:D
What changed in PHP 7.3? TheFilesystem 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
ContributorAuthor

a1812 commentedApr 21, 2021•
edited
Loading

What changed in PHP 7.3?

<?php// check behavior functions realpath, readlink, and component Symfony Filesystem for windowsrequire_once__DIR__ .'/../vendor/autoload.php';useSymfony\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 =newFilesystem();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

PHPrealpathreadlink(b)$fs->readlink ($link2,true)(b)$fs-> readlink ($link2)(a)$fs->readlink ($link2,true)(a)$fs->readlink ($link2)
8.0.*filelink1filefilefilelink1
7.4.18 - 7.4.10filelink1filefilefilelink1
7.4.9 - 7.4.0link1link1filelink1filelink1
7.3.28 - 7.3.22filefilefilefilefilefile
7.3.21 - 7.3.2link1filefilelink1filelink1
7.3.1 - 7.3.0filefilefilefilefilefile
7.2.*link1filefilelink1filelink1
7.1.3 - 7.1.33link1filefilelink1filelink1

###conclusion:

What to do with PHP 7.3.* ?

Correct me, maybe I am confusing or missing something.

@a1812
Copy link
ContributorAuthor

a1812 commentedApr 22, 2021•
edited
Loading

how check

setPROJECT_DIR=C:\symfonysetPHPUNIT=%PROJECT_DIR%/phpunit^    --colors=never^    --bootstrap%PROJECT_DIR%/vendor\autoload.php^    --configuration%PROJECT_DIR%/phpunit.xml.dist%PROJECT_DIR%/src/Symfony/Component/FilesystemFOR%%VIN (    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 errorlevel1 (exit /b    ))

@a1812a1812 changed the title[Filesystem] fix FilesystemTest for Windows[Filesystem] fix Filesystem for WindowsApr 23, 2021
@a1812a1812force-pushed thefix_test_FilesystemTest_for_windows branch from51a0b9c to76358d0CompareApril 23, 2021 09:19
@a1812
Copy link
ContributorAuthor

Hi@nicolas-grekas, please check my code.

@a1812
Copy link
ContributorAuthor

a1812 commentedApr 23, 2021•
edited
Loading

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

@a1812a1812force-pushed thefix_test_FilesystemTest_for_windows branch from76358d0 tobd5861fCompareApril 24, 2021 13:00
@a1812
Copy link
ContributorAuthor

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

nicolas-grekas reacted with heart emoji

@a1812
Copy link
ContributorAuthor

Answer from php.net

The erroneous readlink() has apparently been fixed inadvertentlywhen php_sys_readlink() has been refactored[1].Anyhow, PHP 7.3 is in security mode[2], so unless this would beregarded 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

@a1812a1812force-pushed thefix_test_FilesystemTest_for_windows branch frombd5861f tof1b95d3CompareApril 30, 2021 08:44
@a1812a1812 changed the title[Filesystem] fix Filesystem for Windows[Filesystem] fix readlink() for WindowsMay 11, 2021
Copy link
Member

@nicolas-grekasnicolas-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-grekasnicolas-grekas merged commit54bee29 intosymfony:4.4May 26, 2021
javiereguiluz added a commit to javiereguiluz/symfony-docs that referenced this pull requestMay 28, 2021
This PR was merged into the 4.4 branch.Discussion----------fix readlink descriptionremove description readlink() for Windows misleadingseesymfony/symfony#40866Commits-------a2dd7ee fix readlink description
This was referencedMay 31, 2021
@a1812a1812 deleted the fix_test_FilesystemTest_for_windows branchJuly 21, 2021 09:23
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

4 participants

@a1812@nicolas-grekas@xabbuh@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp