FUEL::find_file + Namespaces

[axelitus]

I just find a bug in FUEL::find_file function on line 259: https://github.com/fuel/core/blob/develop/classes/fuel.php#L259

The class caches the found path so that it has it on it's records. But now we have modules, so the directory structure can 'repeat' itself. If I have two modules: mod1 and mod2 having this structure (I wont put all folders as they are not need to show the point):

- mod1
    |- config
         |- config.php

-mod2
    |- config
         |- config.php

and I run:

$mod1_config = \Config::load('mod1\\config', true);
$mod2_config = \Config::load('mod2\\config', true);

then both vars will have the same contents, because of caching not using the namespace. Let me clarify this (fuel.php L259):

$path = $directory.DS.strtolower($file).$ext;
var_dump($path);

will output in both cases:

string(17) "config\config.php"

if then, on line 261 we check for the cached path:

if (static::$path_cache !== null and array_key_exists($cache_id.$path, static::$path_cache)){
    return static::$path_cache[$cache_id.$path];
}

the array key we are checking ($cache_id.$path) would be in both cases something like this:

string(49) "d41d8cd98f00b204e9800998ecf8427econfig\config.php"

which is wrong because they are two different paths!!! I think the namespace should be somewhere involved in assigning a key for the cached path.

Reference: http://fuelphp.com/forums/posts/view_reply/1271

[axelitus]

I've found a solution for this issue which I think doesn't interfere with how things are supposed to work:

public static function find_file($directory, $file, $ext = '.php', $multiple = false, $cache = true)
{
    $cache_id = '';
    $ns = '';

    $paths = static::$_paths;

    // get extra information of the active request
    if (class_exists('Request', false) and $active = \Request::active())
    {
        $cache_id = md5($active->uri->uri);
        $paths = array_merge($active->paths, $paths);
    }

    // the file requested namespaced?
    if($pos = strripos(ltrim($file, '\\'), '\\'))
    {
        $file = ltrim($file, '\\');
        $ns = ucfirst(substr($file, 0, $pos));

        // get the namespace path
        if ($path = \Autoloader::namespace_path('\\'.$ns))
        {
            // and strip the classes directory as we need the module root
            $paths = array(substr($path,0, -8));

            // strip the namespace from the filename
            $file = substr($file, $pos+1);
        }
        $ns .= DS;
    }

    $path = $directory.DS.strtolower($file).$ext;

    if (static::$path_cache !== null and array_key_exists($cache_id.$ns.$path, static::$path_cache))
    {
        return static::$path_cache[$cache_id.$ns.$path];
    }

    $found = $multiple ? array() : false;
    foreach ($paths as $dir)
    {
        $file_path = $dir.$path;
        if (is_file($file_path))
        {
            if ( ! $multiple)
            {
                $found = $file_path;
                break;
            }

            $found[] = $file_path;
        }
    }

    if ( ! empty($found))
    {
        $cache and static::$path_cache[$cache_id.$ns.$path] = $found;
        static::$paths_changed = true;
    }

    return $found;
}

[WanWizard]

This was already on my todo list. I also want to add the possibility to use fully qualified filename. I'll have a look at it over the weekend.

[axelitus]

Fine! In the meantime I'll stick with my patch... it works for me now! :)

[axelitus]

Is this if supposed to be empty? or even there? https://github.com/fuel/core/blob/develop/classes/fuel.php#L275

Also this breaks everything if the file wasn't found: https://github.com/fuel/core/blob/develop/classes/fuel.php#L311

[axelitus]

I'm sorry I keep re-opening this issue but it's not completely fixed. Well we have namespaces but there's an issue which was introduced by this, which is posted in the forums:

I debugged it today and narrow it down to the find_file() function. As of now the master branch has still a line of code which was used for testing purposes I think in file fuel.php L311 which prevents from showing anything, not even an error. But what's happening is this:

When an exception/error occurs Fuel calls an error view, which is located in core/views/errors/ folder. When this is called then the function gets called liked this:

\Fuel::find_file('views', 'errors\php_fatal_error', '.php', false, false);

The first test that is made is if the targeted file was requested using absolute path; the second one (more interesting due to the nature of this issue) is to see if the file requested was namespaced L244:

// the file requested namespaced?
elseif($pos = strripos(ltrim($file, '\\'), '\\'))

running that command with the given arguments will get the file misinterpreted as being namespaced so everything will be messed-up and that's why the Error reporting is broken in the master and develop branches because it searches for a namespaced file which is interpreted as being in a module called 'errors' and not a directory into the views directory.

Don't know what's best to fix this... if leaving the function as it is and making the call like:

\Fuel::find_file('views\errors', 'php_fatal_error', '.php', false, false);

which will need to change the function call... or change the find_file() function's behavior.

[axelitus]

The error originates from this line of code in the error.php file in core/classes. This is what gets called:

exit(\View::factory('errors'.DS.'php_fatal_error', $data, false));

The DS constant is an '\' so this will effectively turn into 'errors\php_fatal_error' which worked in the previous version of the find_file() function... but as namespaces were introduced then this doesn't work anymore as it is confused for a namespace called 'errors'.

[axelitus]

I've come up with a really horrible workaround so that the function works as intended with namespaces and error views... Note beforehand: I really don't like this! Don't judge hehe... but this way let's me debug other code easier...

On line #245 change this:

elseif($pos = strripos(ltrim($file, '\\'), '\\'))

for this:

elseif($directory !== 'views' && $pos = strripos(ltrim($file, '\\'), '\\'))

Awful I know... but it's only a temporary workaround...

[jschreuder]

I'm having the same problem but as the current code still looks unfinished I'll wait for WanWizard to look into this for a real solution.

In the mean time I think my ugly fix is better than yours :P

Add after line 259 - https://github.com/fuel/core/blob/master/classes/fuel.php#L259

else
{
    $paths = static::$_paths;

    // get extra information of the active request
    if (class_exists('Request', false) and $active = \Request::active())
    {
        $cache_id = $active->uri->uri;
        $paths = array_merge($active->paths, $paths);
    }
}

In words: when the namespacing fails, just do what would happen otherwise.

[axelitus]

Hehe, yes it 'looks' nicer but it doesn't work, as the failure is because the path to the error files IS identified as a namespaced filename... so it really never reaches your ugly fix :P haha... sorry, I mislooked the line you meant... it indeed is a better fix, it's more generic as mine... I'll implement this fix into my repo...

And yes... let's see how WanWizard fixes this... but it involves not only programming I think... so it could take some time before this one is fixed sigh... in the meantime I'll stick with my horrible patch I'll use your ugly patch as I need to be able to debug my code, and this error screen helps A LOT!

[WanWizard]

Back online!

We need to think about the best way to approach this issue:

• have it fall through if no valid module namespace has been found
• use something else then a single backslash to identify the module name

Suggestions welcome

[axelitus]

As posted here I suggest a pipe to separate namespace names from file names...

Regarding the way that Config::save() handles the saving process, I enumerate some points here; that's what I have implemented (of course there's the issue still pending about knowing when it is a namespace, as of now I use the same find_file() method of looking for a backslash...)

[WanWizard]

Decided to choose for a fall-through. It means that on Windows platforms, some extra code is executed as paths will always contain a backslash. So be it.

I'm not in favor of alternative random characters, it will only confuse people.

[jschreuder]

Wouldn't it for consistancy sake be better if we got this to work the same on *nix & Windows?

This was originally my oversight as I proposed the "namespaced path" solution, so I feel a bit responsible. I think it might be better if we would handle the first segment of a path as a module identifier (to be determined by either forward or backward slash) and have the module path prefixed on any type of system to the full array of paths.

That way Fuel will behave the same on both *nix & windows platforms.

[WanWizard]

jschreuder: I can live with that, but it means a namespace check for every call to find_file(). Which due to the number of calls could be causing a noticable delay.

[jschreuder]

WanWizard: you have a point there, though the overhead should be very small as it's only a call to the \Autoloader::namespace_path() which shouldn't be noticeable as it only checks the array for a key?

Another operator we could use might be ::, though that'd only a little less random than using pipes.

[axelitus]

But wouldn't that way (take the first segment of a path as a module) interfere with the global APPATH? I mean that it would prevent of having sub-folders inside the APPATH, because it will always take the first segment as a module.

Unless maybe if we want to use subfolders in the APPATH folder then we could use a (to be defined: backslash or forwardslash) as the first character in the $file parameter... that way we are stating that all that comes next is a directory structure relative to the APPATH folder.

To sum up:

Referencing modules:

find_file('views', 'mymodule\subfolder1\subfolder2\filename', '.php', [...]);

Referencing the APPATH folder:

find_file('views', '\subfolder1\subfolder2\filename', '.php', [...]);

I agree with jschreuder that :: it's more natural than using a pipe... however, it still can confuse people...

[jschreuder]

Reread my proposal:

I think it might be better if we would handle the first segment of a path as a module identifier (to be determined by either forward or backward slash) and have the module path prefixed on any type of system to the full array of paths.

[WanWizard]

Just thought of another downside:

Suppose you have a module called 'test', and load something like 'test/this/that'. This means that find_file() will only check the module, and you can't have for example an app view called 'test/this/that.php'.

[axelitus]

That is what I meant, just not only specific to views...

That's why I proposed prefixing with a slash if one does not mean to use module names...

'test/this/that' will be interpreted as $module = 'test'; $file = 'this/that';

whereas

'/test/this/that' will be interpreted as-is inside the APPATH thus
$module = APPATH (or global); $file = 'test/this/that'

so you could have a view named 'test/this/that.php'

I think this could work, but I really don't know the core of Fuel that much and how 'every' call to files is made, so perhaps I'm missing something here...

[axelitus]

It would be also helpful to define either forward-slash or backward-slash to separate folder structure and then internally change the defined char to DS (as DS changes from *nix to Windows)...

This would insert an extra function call and it may not be needed in some cases as the selected char would match to the filesystem separator; but it would ensure that no matter where the code is running it would work as expected. There's a point where convenience supersedes performance...

[jschreuder]

I've repeated this already, but I will again - this time even shorter and to the point:

and have the module path prefixed [...] to the full array of paths.

[WanWizard]

Decided to ignore this... ;)

Using the first path segment as a namespace identifier causes the namespace to allways be selected if found, so you can never have a folder name equal to an existing namespace (for example, you can not load a view called 'auth/login' anymore if you have loaded the auth package).

Opted instead of using the double colon to identify the namespace: find_file('views', 'module::folder/file');