[#34027] Add ability to override the renderer [New feature]

[dgrammatiko]

In Joomla! if you want to override something you have to put it in the active template directory (e.g.: the layouts goes in (active template)/html/layouts).
This PR introduces the ability to override the whole rendering process in the template by copying (and modifying per your preferences) the /libraries/joomla/document directory.

It shouldn't rise any backward compatibility issues (except that the directory [active template]/document might be in use?).

What this PR might provide to Joomla! devs/users?
Flexibility on the way that the template renders the data (the inner logic). e.g. someone may use echo $this->getBuffer('modules', 'menu', array('menu', 'html5')); and remove the <jdoc.. > parser.

Why?
Because this is one area that if you want to override and use your own logic you have to do it through a plugin (this is way easier and cleaner)

Feature tracker: http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=34027

[Bakual]

Please don't start the property name with an underscore. According to our coding standards we only use a starting underscore for private properties (yes I know there are wrong named properties already there).

[dgrammatiko]

I think I have to study harder the coding standards... :)

[dgrammatiko]

If I declare public $_file = ‘’; tests pass but cs fail. The other way around seems failing the tests so I am little bit confused here. Also all the other variables in the same array have similar key names $_profile. What am I missing here?

[Bakual]

You need to make the change to the property name also in the later checks to $this->_file

[Bakual]

You need to change $this->_file to $this->file 😄

[Bakual]

The error was that you changed the property declaration correctly from public $_file = ''; to public $_file = '';. But you didn't apply this change in the rest of your code where you still used $this->_file instead of $this->file.
That created the "undefined variable" notices which broke Travis.

[dgrammatiko]

In the logic the var is $this->_file from the array is fine. If I don’t declare it travis fail. If I declare is as $file I still have to get the content from $this->_file. I think I will remove the declaration and do it in the unit test file 😃

[dgrammatiko]

Test istructions:

Apply the patch
assuming selected template: protostar
create directory named document in /templates/protostar
copy the folder html from /libraries/joomla/document to protostar/document
So you have /templates/document/html/html.php
edit html.php (in protostar foolder...)
In the last fuction _fetchTemplate()
replace the line before return so it will be like
$this->_template = $this->_loadTemplate($directory . '/' . $template, $file). '';
(Add a comment after the document)
Refresh the page, see the source at the bottom you should have
You just overrided the render!

What you can do if this gets accepted?

Because renderers are the place where the code gets from arrays and strings to actual html, xml, raw, feed or json, you can do pretty much whatever you want.
Some examples of creative code that can be put in the renderers:
Put custom code in renderer html (eg minifier, remove <JDoc )
Create custom code for head (e.g.. scripts and css concatenation, custom metas)
Mesh around with the error page and feed in creative ways
Or go all the way and get mustache or twig in the template

[Bakual]

In the logic the var is $this->_file from the array is fine. If I don’t declare it travis fail. If I declare is as $file I still have to get the content from $this->_file. I think I will remove the declaration and do it in the unit test file

Declaring the property in the unit test certainly is the wrong approach to fix it. That will never be accepted.

Currently I'm wondering where you intend to set the $this->_file property. Before your PR it doesn't seem to be used anywhere. So I assume you introduced it and meant to set it directly somehow.
But then it should be $this->file according to our coding rules as I stated. Properties starting with underlines are meant to be private properties.
And of course it should also be initialised before using it by adding the public $file = ''; as you did originally.

[Bakual]

Also you're restricting this feature now only to the frontend. Why?

[dgrammatiko]

Ok, I’ll give it another try…
As for the restriction only in front end: there are not that many templates for admin…
I’ll revert that as well
@Bakual About $this->_file: All is needed here is the directory of the current template, dirname($this->_file) provides this very inexpensive. Getting it through $template = $app->getTemplate(); is a lot more intense work (many calls on many files, even db query). So what I really meant was the code to be really efficient.

[Bakual]

Ah, so $this->_file is set somewhere from outside the class? Do you know where this is set?

Then of course we can't rename it, sorry for that then. But imho it should still be initialised.

[Bakual]

Found it. It's used in JDocumentError (https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/document/error/error.php#L131) and JDocumentHtml (https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/document/html/html.php#L572).
But it's not a property which is always set in JDocument. So you can't count on that one being there.
However it may be that you only want to override it for HTML documents anyway, Then you should look at making that change in JDocumentHtml instead.

[dgrammatiko]

Maybe something like this will make a good fit: if ($type == ‘html’ || $type == ‘error’) {$this->file… } else {$template = $app->getTemplate(); … } ?

[Bakual]

No, that looks like messy code to me.
Just override the loadRenderer method in JDocumentHtml (and JDocumentError if that's needed) if you think it's important enough.

[dgrammatiko]

Actually the code as i tested will be like

            $path = __DIR__ . '/' . $this->_type . '/renderer/' . $type . '.php';

            if (!empty($this->_file) && file_exists(dirname($this->_file) . '/document/' . $this->_type . '/renderer/' . $type . '.php'))
            {
                $path = dirname($this->_file) . '/document/' . $this->_type . '/renderer/' . $type . '.php';
            }
            else
            {
                $template = $app->getTemplate();
                if (file_exists( JPATH_THEMES .'/' . $template . '/document/' . $this->_type . '/renderer/' . $type . '.php'))
                {
                    $path =  JPATH_THEMES .'/' . $template . '/document/' . $this->_type . '/renderer/' . $type . '.php';
                }
            }

            if (file_exists($path))
            {
                require_once $path;
            }
            else
            {
                throw new RuntimeException('Unable to load renderer class', 500);
            }

[Bakual]

That looks fine, yes.

[dgrammatiko]

On the other hand additional renderers function loadRenderer($type) are available only in html and feed and makes me wonder if this is the best approach here. I’ll give it some thought...

[dgrammatiko]

This PR got really meshy, therefore I am closing it and opening a new one.

I forgot: Big thanx to Thomas @Bakual for his help!