Skip to content

[5.0] Fix Windows path for nested config folders#7632

Merged
taylorotwell merged 1 commit intolaravel:5.0from
RomainLanz:patch-1
Feb 26, 2015
Merged

[5.0] Fix Windows path for nested config folders#7632
taylorotwell merged 1 commit intolaravel:5.0from
RomainLanz:patch-1

Conversation

@RomainLanz
Copy link
Copy Markdown

Fix 9aed416

Thanks to @portonefive

@RomainLanz RomainLanz changed the title [5.0] Fix Windows path [5.0] Fix Windows path for nested config folders Feb 26, 2015
@cristianuibar
Copy link
Copy Markdown

Thank you.

taylorotwell added a commit that referenced this pull request Feb 26, 2015
[5.0] Fix Windows path for nested config folders
@taylorotwell taylorotwell merged commit 1958b70 into laravel:5.0 Feb 26, 2015
@taylorotwell
Copy link
Copy Markdown
Member

Sigh this broke Laravel entirely.

@taylorotwell
Copy link
Copy Markdown
Member

Reverted since this literally broke the entire framework.

@ValsiS
Copy link
Copy Markdown

ValsiS commented Feb 26, 2015

ty , in 5.0.10 :) it works fine

@RomainLanz
Copy link
Copy Markdown
Author

@taylorotwell
Copy link
Copy Markdown
Member

That bug is unrelated.

@RomainLanz
Copy link
Copy Markdown
Author

@taylorotwell
Copy link
Copy Markdown
Member

Yeah, it's broken on Windows. You can stop posting links. I don't have a Windows machine so I can't help you. You're fix broke it on Linux.

@taylorotwell
Copy link
Copy Markdown
Member

I'll probably just rip out this feature entirely unless someone is able to get a fix soon.

@RomainLanz
Copy link
Copy Markdown
Author

I'll try to find a fix, how many time can you wait?

@taylorotwell
Copy link
Copy Markdown
Member

I don't know. Looking at the code I can't even figure out the problem, so...

@portonefive
Copy link
Copy Markdown

Hate to see features get ripped out.. The paths just need to be standardized. When I posted the first fix I knew it needed to be opposite. This will standardize Windows paths to Linux. Please PR it @RomainLanz

if ($tree = trim(str_replace(str_replace('\\', '/', config_path()), '', str_replace('\\', '/', $directory)), DIRECTORY_SEPARATOR))

You can format this better if you'd like to avoid the long line length (maybe do the $directory str_replace where the variable is set), I just don't have the time to do a PR right now.

@taylorotwell
Copy link
Copy Markdown
Member

I don't even understand why any of that is necessary. Can someone please for the love of all that is holy explain it to me? We are using DIRECTORY_SEPARATOR.

@taylorotwell
Copy link
Copy Markdown
Member

Tagged that fix. All good?

@ValsiS
Copy link
Copy Markdown

ValsiS commented Feb 27, 2015

5.0.12 no error message ...

@portonefive
Copy link
Copy Markdown

@taylorotwell

config_path() string (27) "C:\Development\boise/config"
$directory string (27) "C:\Development\boise\config"

You can see how the str_replace on line 91 would be ineffective due to the strings not being equal.

Using DIRECTORY_SEPARATOR is all fine (and preferred even), but you would need to use it where you define the paths.

Illuminate\Foundation\Application: Line 279 - configPath() would need to be:

return $this->basePath.DIRECTORY_SEPARATOR.'config';

Ideally you would want to replace all references of '/' with DIRECTORY_SEPARATOR in that file. I will submit a PR when I get home later if you haven't done so by then.

@taylorotwell
Copy link
Copy Markdown
Member

Gotcha. Thanks for explanation.

@portonefive
Copy link
Copy Markdown

@taylorotwell You can revert the fix that was posted in this thread and merge #7635 if you'd like. Just changes out the hard-coded separators for the constant.

@taylorotwell
Copy link
Copy Markdown
Member

OK I merged #7635. Is anyone actually testing this stuff? I'm not tagging another release until someone posts a screenshot of this working or submits a unit tests (perferrably)

@taylorotwell
Copy link
Copy Markdown
Member

With #7635 merged, those str_replaces are needed anymore so I removed it back to how it was. Can someone set minimum-stability to "dev" and make sure this is working on Windows?

@portonefive
Copy link
Copy Markdown

@taylorotwell I am currently running with minimum-stability at dev, and on Windows, and the above is working. The only fix that is needed is #7635 so you can surely revert the str_replace changes.

@taylorotwell
Copy link
Copy Markdown
Member

So with latest commit hash on laravel/framework it is working for you and loading config directories that are nested?

@portonefive
Copy link
Copy Markdown

That's correct. I am running (dev-master eab95a8) on Windows, with the following config:

config/test/foo.php

<?php

return [
    'bar' => 'baz'
];

Result:
config('test.foo.bar') string (3) "baz"

All seems well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants