Extend Parser\View rather than \Fuel\Core\View

[tgriesser]

Each of the individual parser classes should be extending the \Parser\View class rather than the \View class, in the case that the Parser package is not loaded/called until after the first \View is called.

[jschreuder]

They all extend \View because that allows you to extend the View class in the App which will in turn be used by the package. It doesn't refer to Fuel\Core\View as you say in the commit message, once the package is loaded \View refers to Parser\View.

[WanWizard]

According to the extra information given in the forum topic (http://fuelphp.com/forums/topics/view/6995#6996) about this, there is an issue when you use the View class from the core first, and then load the parser package.

Might this be the issue that once the autoloader has an alias for \View, it will no longer accept a new one, so Parser\View will no longer be called once \Fuel\Core\View is used (and aliased to global)?

[jschreuder]

I read it, but it doesn't change what I said and why I closed it. I responded in the topic.

[tgriesser]

Got your message about loading it in the bootstrap, which makes sense... but if using extended \View is the end goal, wouldn't it make more sense then to have the view.php with

namespace Parser;
class View extends \View

and then in mustache.php (etc.)

namespace Parser;
class View_Mustache extends View

which offers the ability to lazy-load the view, still use to the overridden \View and still refer to the correct namespace from within parser related classes?

Something like this: https://github.com/tgriesser/parser/commit/b43812feafc4ade5407be197d52da74adcb293dd

[jschreuder]

Got your message about loading it in the bootstrap, which makes sense... but if using extended \View is the end goal, wouldn't it make more sense then to have the view.php with

No because that code would crash as it essentially means:

namespace Parser;
class View extends \Parser\View {}

Parser\View is aliased to View but you do have to load the Parser package before the View class is ever used. Otherwise Fuel\Core\View is aliased before it gets the chance. That's how it's intended to work, the Parser\View class is meant to "overwrite" the Fuel\Core\View class. Which is why it says in the docs to add the Parser package to the always_load config.

[tgriesser]

Again, that makes sense. I'll just stick to putting something it in the bootstrap then.

One final question... would it make any sense to separate

public static function _init()
{
    \Config::load('parser', true);

    // Get class name
    $class = \Inflector::denamespace(get_called_class());

    if ($class !== __CLASS__)
    {
        // Include necessary files
        foreach ((array) \Config::get('parser.'.$class.'.include', array()) as $include)
        {
            if ( ! array_key_exists($include, static::$loaded_files))
            {
                require $include;
                static::$loaded_files[$include] = true;
            }
        }
    }
}

into the respective classes, or put that into an intermediate class which couldn't potentially be overridden, in the case that you wanted to use the parsers individually rather than as the \View override... The reason I didn't see the above commit crashing is because I was using it to just call View_Mustache::parser() directly...

I understand that the accepted convention is to just override the \View class but that actually shouldn't be necessary in the way that I'm trying to use it.

Thanks.

[jschreuder]

If you don't load the Parser package early enough, View::forge() won't even work as expected. I see little point in resolving some problems but not others (all of which are caused by undocumented usage) by sacrificing DRY code, that's just bad practice all around.

[WanWizard]

This was the only remark I tried to make earlier:

Otherwise Fuel\Core\View is aliased before it gets the chance. That's how it's intended to work, 
the Parser\View class is meant to "overwrite" the Fuel\Core\View class.

If we require all packages to be used by an application to be loaded in the config, we need to clearly document that. And add a warning to the documenation of the Package class, because that provides the methods to load packages in your code (which as I read your comments is something we advice against).

[tgriesser]

@WanWizard right, that might need to be made a little clearer, I was under the impression that by having Package::load it would be good idea to have the ability to conditionally load packages within packages, which should work fine unless the package it is intentionally aliasing to the base namespace (like in this case with the View class) and thus any dependent classes for the package are not inheriting from the intended class if called out of order

@jschreuder I see why it wouldn't make sense to do that, just wanted to see if there was some way that I could do it other than in the bootstrap.

I wouldn't say it's an entirely undocumented use case, I was just trying to conditionally grab the static parser object from the driver as per the docs: http://docs.fuelphp.com/packages/parser/intro.html

All drivers have a parser() method that will allow you to access the current Parser object. This is a convention and not a requirement and might be different on 3rd party drivers.

// Example static usage
View_SimpleTags::parser()->set_delimiters('{', '}');

It would make some sense if you didn't want the parser class to be loaded on every request as would be the case if it were loaded in the bootstrap, especially if I don't even need the parser's version of forge, but I'll just do it that way for now.

I guess another option would be to check if the config and \Mustache are loaded after I load the class with Package::load("parser") and if not, require those it in my package's code.

Not trying to be a pain in the ass, just trying to figure out a few things as I'm working my way though the core, etc.

[jschreuder]

Agreed it must be put clearer in the docs, though it already says that to install it you must put it in the always-load config it doesn't underline that you have to do that or deal with the consequences.

This is meant as a View class replacement, not as something additional. There's no real cause for optimization if you only use it some of the time as it only adds 1 fileload when the class isn't used (the Parser package's bootstrap) or 2 if the View class is used (the Parser\View extension). You could of course load it with Package::load() as long as you do it before the View class is used, but it's better to keep that undocumented as that'll just invite unnecessary invalid bug-reports.

[WanWizard]

I'll put updating the docs on my todo list...