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.

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

#2909: Relative root paths no longer seem to work #4019

Merged
merged 6 commits into from
Mar 29, 2019
Merged

#2909: Relative root paths no longer seem to work #4019

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f3678a9
Return the realpath instead of symlinked path
JTubex Mar 27, 2019
c073932
#4109: Convert symlinked originalCwd to the realpath
JTubex Mar 28, 2019
127da83
Merge branch 'master' into master
JTubex Mar 28, 2019
b0a0653
Fix AppVeyor tests
JTubex Mar 28, 2019
94eba9e
Fix failing test EnvironmentTest::testExportConfigData()
JTubex Mar 28, 2019
594797f
Canonicalize originalCwd after it is converted to the realpath
JTubex Mar 29, 2019
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
2 changes: 1 addition & 1 deletion src/Config/Environment.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class Environment
public function __construct($homeDir, $cwd, $autoloadFile)
{
$this->homeDir = $homeDir;
$this->originalCwd = Path::canonicalize($cwd);
$this->originalCwd = FsUtils::realpath($cwd);
Copy link
Member

Choose a reason for hiding this comment

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

FsUtils::realpath is a lot safer than a raw realpath call, so perhaps this is okay. If we add this, it means that we'll have to work harder in the AppVeyor tests, because this will convert Windows paths from using / to \.

This only introduces a handful of failures in the current tests, so I suppose it is tractable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@greg-1-anderson, I'm not sure what the right way is to fix the tests. Can you point me in the right direction? Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Our typical strategy here is to normalize the output before asserting. For example, if you str_replace each \ with a /, then you can compare the path invariantly again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Let's see if this fixes the tests.

$this->etcPrefix = '';
$this->sharePrefix = '';
$this->drushBasePath = Path::canonicalize(dirname(dirname(__DIR__)));
Expand Down
6 changes: 3 additions & 3 deletions tests/unit/ConfigLocatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public function testOnlyEnvironmentData()
$configLocator = new ConfigLocator('TEST_');
$configLocator->addEnvironment($this->environment());
$config = $configLocator->config();
$this->assertEquals($this->homeDir(), $config->get('env.cwd'));
$this->assertEquals($this->homeDir(), str_replace('\\', '/', $config->get('env.cwd')));
}

/**
Expand All @@ -39,7 +39,7 @@ public function testLoadAll()

$config = $configLocator->config();

$this->assertEquals($this->homeDir(), $config->get('env.cwd'));
$this->assertEquals($this->homeDir(), str_replace('\\', '/', $config->get('env.cwd')));
$this->assertEquals('A system-wide setting', $config->get('test.system'));
$this->assertEquals('A user-specific setting', $config->get('test.home'));
$this->assertEquals('A site-specific setting', $config->get('test.site'));
Expand All @@ -65,7 +65,7 @@ public function testLocalMode()
*/

$config = $configLocator->config();
$this->assertEquals($this->homeDir(), $config->get('env.cwd'));
$this->assertEquals($this->homeDir(), str_replace('\\', '/', $config->get('env.cwd')));
$this->assertFalse($config->has('test.system'));
$this->assertFalse($config->has('test.home'));
$this->assertEquals('A site-specific setting', $config->get('test.site'));
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/EnvironmentTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ class EnvironmentTest extends TestCase
public function testExportConfigData()
{
$data = $this->environment()->exportConfigData();
$this->assertEquals($this->homeDir(), $data['env']['cwd']);
$this->assertEquals($this->homeDir(), str_replace('\\', '/', $data['env']['cwd']));
}

public function testDocsPath()
Expand Down