Skip to content

Fixes to path handling on Windows #2568

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.

Sign up for GitHub

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

Merged
dantleech merged 1 commit into from
Feb 27, 2024
Merged

Fixes to path handling on Windows #2568

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/Extension/ComposerAutoloader/Tests/Unit/ClassLoaderFactoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use PHPUnit\Framework\TestCase;
use Phpactor\Extension\ComposerAutoloader\ClassLoaderFactory;
use Psr\Log\NullLogger;
use Symfony\Component\Filesystem\Path;

class ClassLoaderFactoryTest extends TestCase
{
Expand All @@ -13,6 +14,6 @@ public function testClassLoader(): void
$logger = new NullLogger();
$loader = (new ClassLoaderFactory(__DIR__ . '/../../../../../vendor/composer', $logger))->getLoader();
$file = $loader->findFile(__CLASS__);
self::assertEquals(__FILE__, $file);
self::assertEquals(Path::canonicalize(__FILE__), Path::canonicalize((string) $file));
}
}
3 changes: 2 additions & 1 deletion lib/Extension/WorseReferenceFinder/Tests/Unit/WorseReferenceFinderExtensionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use Phpactor\ReferenceFinder\TypeLocator;
use Phpactor\TextDocument\ByteOffset;
use Phpactor\TextDocument\TextDocumentBuilder;
use Symfony\Component\Filesystem\Path;

class WorseReferenceFinderExtensionTest extends TestCase
{
Expand All @@ -32,7 +33,7 @@ public function testLocateDefinition(): void
ByteOffset::fromInt(3)
)->first()->location();

$this->assertEquals(realpath(__DIR__ . '/../../WorseReferenceFinderExtension.php'), $location->uri()->path());
$this->assertEquals(Path::canonicalize(__DIR__ . '/../../WorseReferenceFinderExtension.php'), $location->uri()->path());
}

public function testLocateType(): void
Expand Down
7 changes: 4 additions & 3 deletions lib/Filesystem/Domain/FilePath.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,9 @@ public static function fromString(string $string): FilePath
public static function fromParts(array $parts): FilePath
{
$path = Path::join(...$parts);
if (!str_starts_with($path, '/')) {
$path = '/'.$path;
if (!Path::isAbsolute($path)) {
// Not sure if this makes sense… Maybe it should just throw?
$path = Path::makeAbsolute($path, '/');
Copy link
Collaborator

@dantleech dantleech Feb 27, 2024

Choose a reason for hiding this comment

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

i think the idea is that this is always absolute, so fromParts(['home','daniel',])

Copy link
Contributor Author

@MatmaRex MatmaRex Feb 27, 2024

Choose a reason for hiding this comment

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

To elaborate, there are two things that confuse me:

  • Where does data in this format come from? I'm not aware of any filesystem APIs that return paths like that, and I've never seen anyone use that format in their configuration. I also couldn't find what in your code generates them. I'm like 70% sure that this code path is unused apart from tests.
  • How is this supposed to work with Windows paths, where absolute paths must have a drive letter (e.g. "C:") in addition to the list of path segments? The way I made it work here, this function actually returns a drive-relative path (e.g. "\Users\daniel"), not an absolute one ("C:\Users\daniel"), and the drive letter will be resolved against the current working directory when trying to read from the path. This will probably often work, but it sometimes won't.

Copy link
Collaborator

@dantleech dantleech Feb 27, 2024

Choose a reason for hiding this comment

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

it's used here:

https://github.com/phpactor/phpactor/blob/master/lib/Filesystem/Adapter/Simple/SimpleFilesystem.php

i guess it won't work for Windows... but feel free to throw an exception here, if the tests pass then it's all good.

Copy link
Contributor Author

@MatmaRex MatmaRex Feb 27, 2024

Choose a reason for hiding this comment

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

Hmm, just this one method?

    public function createPath(string $path): FilePath
    {
        if (Path::isRelative($path)) {
            return FilePath::fromParts([$this->path->path(), $path]);
        }

        ...
    }

If $this->path->path() is an absolute path, and it looks like it should be, then that will be fine.

}

return self::fromString($path);
Expand Down Expand Up @@ -107,7 +108,7 @@ public function extension(): string

public function isWithin(FilePath $path): bool
{
return str_starts_with($this->path(), $path->path().'/');
return Path::isBasePath($path->path(), $this->path());
}

public function isWithinOrSame(FilePath $path): bool
Expand Down
9 changes: 5 additions & 4 deletions lib/Filesystem/Tests/Unit/Domain/FilePathTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use Phpactor\TextDocument\Exception\InvalidUriException;
use RuntimeException;
use SplFileInfo;
use Symfony\Component\Filesystem\Path;
use stdClass;

class FilePathTest extends TestCase
Expand Down Expand Up @@ -66,11 +67,11 @@ public function provideUnknown()

yield 'SplFileInfo' => [
new SplFileInfo(__FILE__),
__FILE__
Path::canonicalize(__FILE__)
];
yield 'SplFileInfo with scheme' => [
new SplFileInfo('file://' . __FILE__),
__FILE__
Path::canonicalize(__FILE__)
];
}

Expand Down Expand Up @@ -180,7 +181,7 @@ public function testIsNamed(): void
public function testAsSplFileInfo(): void
{
$path1 = FilePath::fromUnknown(new SplFileInfo('file://' . __FILE__));
self::assertEquals(__FILE__, $path1->__toString());
self::assertEquals(__FILE__, $path1->asSplFileInfo()->__toString());
self::assertEquals(Path::canonicalize(__FILE__), $path1->__toString());
self::assertEquals(Path::canonicalize(__FILE__), $path1->asSplFileInfo()->__toString());
}
}
3 changes: 2 additions & 1 deletion lib/Indexer/Tests/Unit/Adapter/Worse/IndexerClassSourceLocatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
use Phpactor\Indexer\Adapter\Worse\IndexerClassSourceLocator;
use Phpactor\WorseReflection\Core\Exception\SourceNotFound;
use Phpactor\WorseReflection\Core\Name;
use Symfony\Component\Filesystem\Path;

class IndexerClassSourceLocatorTest extends TestCase
{
Expand Down Expand Up @@ -48,7 +49,7 @@ public function testReturnsSourceCode(): void
$index->write($record);
$locator = $this->createLocator($index);
$sourceCode = $locator->locate(Name::fromString('Foobar'));
$this->assertEquals(__FILE__, $sourceCode->uri()?->path());
$this->assertEquals(Path::canonicalize(__FILE__), $sourceCode->uri()?->path());
}

private function createLocator(InMemoryIndex $index): IndexerClassSourceLocator
Expand Down
3 changes: 2 additions & 1 deletion lib/Indexer/Tests/Unit/Adapter/Worse/IndexerFunctionSourceLocatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use Phpactor\Indexer\Adapter\Php\InMemory\InMemoryIndex;
use Phpactor\Indexer\Adapter\Worse\IndexerFunctionSourceLocator;
use Phpactor\Indexer\Model\Record\FunctionRecord;
use Symfony\Component\Filesystem\Path;
use Phpactor\WorseReflection\Core\Exception\SourceNotFound;
use Phpactor\WorseReflection\Core\Name;

Expand Down Expand Up @@ -44,7 +45,7 @@ public function testReturnsSourceCode(): void
$index->write($record);
$locator = $this->createLocator($index);
$sourceCode = $locator->locate(Name::fromString('Foobar'));
$this->assertEquals(__FILE__, $sourceCode->uri()?->path());
$this->assertEquals(Path::canonicalize(__FILE__), $sourceCode->uri()?->path());
}

private function createLocator(InMemoryIndex $index): IndexerFunctionSourceLocator
Expand Down
6 changes: 3 additions & 3 deletions lib/Phpactor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,13 +296,13 @@ public static function boot(InputInterface $input, OutputInterface $output, stri
*/
public static function normalizePath(string $path): string
{
return Path::makeAbsolute($path, (string) getcwd());
return Path::makeAbsolute($path, (string)getcwd());
}

public static function relativizePath(string $path): string
{
if (str_starts_with($path, (string)getcwd())) {
return substr($path, strlen((string) getcwd()) + 1);
if (Path::isBasePath((string)getcwd(), $path)) {
return Path::makeRelative($path, (string)getcwd());
}

return $path;
Expand Down
3 changes: 2 additions & 1 deletion lib/TextDocument/Tests/Unit/TextDocumentBuilderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use PHPUnit\Framework\TestCase;
use Phpactor\TextDocument\Exception\TextDocumentNotFound;
use Phpactor\TextDocument\TextDocumentBuilder;
use Symfony\Component\Filesystem\Path;

class TextDocumentBuilderTest extends TestCase
{
Expand All @@ -22,7 +23,7 @@ public function testCreate(): void
public function testFromUri(): void
{
$doc = TextDocumentBuilder::fromUri('file://' . __FILE__)->build();
$this->assertEquals('file://' . __FILE__, $doc->uri());
$this->assertEquals('file://' . Path::canonicalize(__FILE__), $doc->uri());
$this->assertEquals(file_get_contents(__FILE__), $doc->__toString());
}

Expand Down
9 changes: 5 additions & 4 deletions lib/TextDocument/Tests/Unit/TextDocumentUriTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,14 @@
use PHPUnit\Framework\TestCase;
use Phpactor\TextDocument\Exception\InvalidUriException;
use Phpactor\TextDocument\TextDocumentUri;
use Symfony\Component\Filesystem\Path;

class TextDocumentUriTest extends TestCase
{
public function testCreate(): void
{
$uri = TextDocumentUri::fromString('file://' . __FILE__);
$this->assertEquals('file://' . __FILE__, (string) $uri);
$this->assertEquals('file://' . Path::canonicalize(__FILE__), (string) $uri);
}

public function testCreateUntitled(): void
Expand All @@ -23,13 +24,13 @@ public function testCreateUntitled(): void
public function testCreatePhar(): void
{
$uri = TextDocumentUri::fromString('phar://' . __FILE__);
$this->assertEquals('phar://' . __FILE__, (string) $uri);
$this->assertEquals('phar://' . Path::canonicalize(__FILE__), (string) $uri);
}

public function testNormalizesToFileScheme(): void
{
$uri = TextDocumentUri::fromString(__FILE__);
$this->assertEquals('file://' . __FILE__, (string) $uri);
$this->assertEquals('file://' . Path::canonicalize(__FILE__), (string) $uri);
}

public function testExceptionOnNonAbsolutePath(): void
Expand Down Expand Up @@ -69,7 +70,7 @@ public function testFromHttpUri(): void
public function testReturnsPath(): void
{
$uri = TextDocumentUri::fromString('file://' . __FILE__);
$this->assertEquals(__FILE__, $uri->path());
$this->assertEquals(Path::canonicalize(__FILE__), $uri->path());
}

public function testScheme(): void
Expand Down
3 changes: 2 additions & 1 deletion lib/WorseReflection/Tests/Integration/Bridge/Phpactor/ClassToFileSourceLocatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use Phpactor\WorseReflection\Bridge\Phpactor\ClassToFileSourceLocator;
use Phpactor\WorseReflection\Core\ClassName;
use Phpactor\WorseReflection\Core\Exception\SourceNotFound;
use Symfony\Component\Filesystem\Path;

class ClassToFileSourceLocatorTest extends IntegrationTestCase
{
Expand All @@ -25,7 +26,7 @@ public function testLocator(): void
{
$source = $this->locator->locate(ClassName::fromString(__CLASS__));
$this->assertEquals(file_get_contents(__FILE__), (string) $source);
$this->assertEquals(__FILE__, $source->uri()->path());
$this->assertEquals(Path::canonicalize(__FILE__), $source->uri()->path());
}

/**
Expand Down
3 changes: 2 additions & 1 deletion lib/WorseReflection/Tests/Unit/Bridge/Composer/ComposerSourceLocatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use PHPUnit\Framework\TestCase;
use Phpactor\WorseReflection\Bridge\Composer\ComposerSourceLocator;
use Phpactor\WorseReflection\Core\Name;
use Symfony\Component\Filesystem\Path;

class ComposerSourceLocatorTest extends TestCase
{
Expand All @@ -13,6 +14,6 @@ public function testLocate(): void
$autoloader = require(__DIR__ . '/../../../../../../vendor/autoload.php');
$locator = new ComposerSourceLocator($autoloader);
$sourceCode = $locator->locate(Name::fromString(ComposerSourceLocatorTest::class));
$this->assertEquals(__FILE__, realpath($sourceCode->uri()->path()));
$this->assertEquals(Path::canonicalize(__FILE__), $sourceCode->uri()->path());
}
}
3 changes: 2 additions & 1 deletion ...eflection/Tests/Unit/Core/SourceCodeLocator/NativeReflectionFunctionSourceLocatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use Phpactor\WorseReflection\Core\Exception\SourceNotFound;
use Phpactor\WorseReflection\Core\Name;
use Phpactor\WorseReflection\Core\SourceCodeLocator\NativeReflectionFunctionSourceLocator;
use Symfony\Component\Filesystem\Path;

class NativeReflectionFunctionSourceLocatorTest extends TestCase
{
Expand All @@ -22,7 +23,7 @@ public function setUp(): void
public function testLocatesAFunction(): void
{
$location = $this->locator->locate(Name::fromString(__NAMESPACE__ . '\\test_function'));
$this->assertEquals(__FILE__, $location->uri()->path());
$this->assertEquals(Path::canonicalize(__FILE__), $location->uri()->path());
$this->assertEquals(file_get_contents(__FILE__), $location->__toString());
}

Expand Down
3 changes: 2 additions & 1 deletion tests/Unit/Extension/CodeTransformExtra/Rpc/GenerateMethodHandlerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
use Phpactor\TextDocument\TextEdit;
use Phpactor\TextDocument\TextEdits;
use Prophecy\Prophecy\ObjectProphecy;
use Symfony\Component\Filesystem\Path;
use function Safe\file_get_contents;

class GenerateMethodHandlerTest extends HandlerTestCase
Expand Down Expand Up @@ -52,7 +53,7 @@ public function testProvidesOriginalSourceFromDiskIfPathIsNotTheGivenPath(): voi

$this->assertInstanceOf(UpdateFileSourceResponse::class, $response);
assert($response instanceof UpdateFileSourceResponse);
$this->assertEquals(__FILE__, $response->path());
$this->assertEquals(Path::canonicalize(__FILE__), $response->path());
$this->assertEquals($thisFileContents, $response->oldSource());
$this->assertEquals($thisFileContents.'1', $response->newSource());
}
Expand Down