Bug: Inconsistent behavior of view renderer on Windows and Linux

[sfadschm]

Direction
The view renderer behaves differently on Windows and Linux in specific cases, which should be fixed for consistency.
Corresponding forum thread: https://forum.codeigniter.com/thread-77305.html

Describe the bug
If a view file is called incorrectly by using namespace syntax (i.e. backslashes) when folder syntax (i.e. forward slashes) should be used, the view will still be loaded in Windows environments, but will fail on Linus systems (expected behaviour).

CodeIgniter 4 version
4.0.4 (latest develop)

Affected module(s)
View renderer

Expected behavior, and steps to reproduce if appropriate

1. View file is located in folder e.g. 'app/Views/Auth/login.php'.
2. Try to call view (incorrectly) by view('Auth\login').
3. Behavior on Windows systems: File loaded successfully. Behavior on Linus systems: File-not-Found-Exception (expected).
   This can lead to problems, when, e.g., a view is successfully loaded in a Windows development environment, but subsequently fails to load in the Linux production environment.

Background
Looking inside the render() function of the View class reveals the following behaviour:

Windows:
$view: Auth\login
$renderVars['file']: C:\XAMPP\htdocs\ci-ttv\app\Config/../Views/Auth\login.php
is_file: bool(true)

Debian:
$view: Auth\login
$renderVars['file']: /var/www/xxx/html/intern/app/Config/../Views/Auth\login.php
is_file: bool(false) (hence, FileLocator is called)

On Debian, the FileLocator subsequently cannot find the view file, as is (rightly) assumes the string to be namespaced, but cannot locate the namespace location.

Suggested solution

1. If a string containing backslashes is passed to the view() method, it should be captured and treated as a namespaced string (similar to how the FileLocator handles it). In this case, the is_file check and other stuff in the View->render() function can be skipped and the parameter can directly be passed to the FileLocator.
2. When checking and loading the passed $view via is_file all backslashed should be replaced with forward slashed temporarily (this is also somewhat present in the fileloader).

Personally, I prefer the first solution as I feel that the second option promotes wrong coding and also the first option seems to be more consistent with the way FileLocator works.

Context

• OS: Windows 10 / Debian
• Web server: Apache 2.4
• PHP version: 7.2

[paulbalandan]

On your preferred solution, please be aware that the separator for Windows is also backslashes. So checking for a "namespaced string" on Windows will give a false positive.

Since the view() can handle both classes and file paths in its 1st parameter, I'm thinking of normalizing how the renderer detects what is a class from what is a file. Current implementation is all inputs are checked for a file extension and appends '.php' if none was found. I'm thinking of all input with no file ext are treated as classes while the rest are file paths. Then classes go directly to check with FileLocator while the files are checked with is_file first.

Opinions? @michalsn @MGatner @lonnieezell @samsonasik

[sfadschm]

I think you're right about the false positives. However, I feel requiring a file extension for a view to be loaded via the 'path'-way might be a BC breaking change, as then, loading views via view('folderName/fileName) would not be possible any more?
The FileLocator->locateFile() function currently uses this:

// Is not namespaced? Try the application folder.  
if (strpos($file, '\\') === false)
{
	return $this->legacyLocate($file, $folder);
}

So if a filename does not contain backslashes, it is handled as a folder/file path. If it does contain backslashes, als forward slashes are replaced by backslashes and namespaces are checked.
Maybe we can do kind of a hybrid thing such as this:

if(!$empty($fileExt) || strpos($file, '\\') === false)
{
  // -> load directly via path
}
else
{
  // -> use FileLocator
}

This way any view with a file extension will be handled as a path (as far as I understand, namespaced files cannot have extensions, right?) and also views that only contain forward slashes.

[paulbalandan]

Spot on for the BC break. I did not think of that. The legacyLocate method is already deprecated so reliance on that must be removed. I was also thinking somewhat the same on your conditional check. That may somehow solve this.

[sfadschm]

Ah alright, I didn't see that legacyLocate is deprecated.
However, I tried to sort this out a little and at the moment, the way FileLocator->locateFile and View->render work seems a bit off to me. Right now, we are relying on Windows systems to accept both '/' and '' as directory separators, which works fine on recent OS. Yet, as far as I know, '' is still the official directory separator on Windows. This results in file paths with mixed separators, e.g. calling view('folderName/fileName') results in the file path 'Xampp\htdocs\Views/folderName/fileName.php'. While this is not really an issue with current Windows I would be more happy with the default OS separators being used (e.g., by using DIRECTORY_SEPARATOR).
Apart from this, we should still promote development in 'Linux-Style', i.e. function parameters containing '' should intend to use namespaces, while parameters containing solely '/' should intent to load a file via path.

This would require the above change in the render function but also some changes to the FileLocator.
Let's see what the others think.

[michalsn]

To be honest I don't really see an issue here. Paths with / on Windows are resolved properly. Trying to always check the directory separator is not worth the hassle for me.

Mistakes... well, maybe they will happen - I can't argue with this. But the user guide clearly states how files in folders should be loaded and how from namespaces.

If people will follow the user guide they're safe.

[sfadschm]

Actually can't really argue against that.

The initial idea was to have CI consistently throw the same exceptions on both OS when people mess with the intended view loading options.

If you guys think this can stay the way it is, I'm happy with closing, too :)

[sfadschm]

Closing as behavior seems to be desired.