[RFC|Merged] Template locator and path finder

[Blitze]

ext/ext3/styles/all/templates currently does not work because $names[] = 'all'; (includes/style/style.php, line 101). Is this by design our an oversight?

[brunoais]

Welcome to area51. The place where development is made .
Please format your post better so that I can understand better what you are writing. (use more paragraphs)

[imkingdavid]

note that "templates" in your path above should not have an s. Other than that, please be more specific about what doesn't work and why that line you showed is the problem.

[Blitze]

Sorry folks, I was at work, and posted using my cell phone.

Say I have the following extension structure:

Code: Select all

root
   - ext
       - test
            - mypage
                - controller
                - ---
                - styles
                    - all
                        - template
                            - page_body.html
                            - main_body_before.html (*custom template event added as <!-- EVENT main_body_before --> in overall_header.html)
                            - my_content.html

and say the content of main_body_before.html is:

Code: Select all

<!-- INCLUDE my_content.html -->

The first issue I noticed is that the template event works just fine but the my_content.html cannot be located:

Code: Select all

style resource locator: File for handle my_content.html does not exist. Could not find: ./styles/prosilver/template/my_content.html, ./styles/prosilver/template/my_content.html

The second issue I noticed is that if I call the controller like so mysite/app.php?controller=mypage/test which uses the page_body.html for display I get:

Code: Select all

style resource locator: File for handle body does not exist. Could not find: ./styles/prosilver/template/page_body.html, ./styles/prosilver/template/page_body.html

Now if I change ext/test/mypage/styles/all/ to ext/test/mypage/styles/prosilver/, the controller renders the template but the INCLUDE statement in the template event still doesn't work.

What I found out is that the base template path does not include "all". This is actually commented out in root/includes/style/style.php on line 101

Code: Select all

	/**
	* Set style location based on (current) user's chosen style.
	*/
	public function set_style()
	{
		$style_path = $this->user->style['style_path'];
		$style_dirs = ($this->user->style['style_parent_id']) ? array_reverse(explode('/', $this->user->style['style_parent_tree'])) : array();

		$names = array($style_path);
		foreach ($style_dirs as $dir)
		{
			$names[] = $dir;
		}
		// Add 'all' path, used as last fallback path by events and extensions
		//$names[] = 'all';

		$paths = array();
		foreach ($names as $name)
		{
			$paths[] = $this->get_style_path($name);
		}

		return $this->set_custom_style($style_path, $paths, $names);
	}

If I uncomment //$names[] = 'all', all the above problems seem to be resolved but there is a new problem, which I think is the reason why this was commented out.

I am able to login just fine but If I try to go to the ACP, I get the following, where I normally would get the second level login

Code: Select all

style resource locator: File for handle my_content.html does not exist. Could not find: ./../styles/prosilver/template/my_content.html, ./../styles/prosilver/template/my_content.html, ./../styles/all/template/my_content.html

BACKTRACE

FILE: (not given by php)
LINE: (not given by php)
CALL: msg_handler()

FILE: [ROOT]/includes/style/resource_locator.php
LINE: 227
CALL: trigger_error()

FILE: [ROOT]/includes/template/template.php
LINE: 302
CALL: phpbb_style_resource_locator->get_source_file_for_handle()

FILE: [ROOT]/includes/template/template.php
LINE: 186
CALL: phpbb_template->_tpl_load()

FILE: [ROOT]/includes/template/template.php
LINE: 445
CALL: phpbb_template->load_and_render()

FILE: [ROOT]/cache/tpl_prosilver_overall_header.html.php
LINE: 335
CALL: phpbb_template->_tpl_include()

FILE: [ROOT]/includes/template/renderer_include.php
LINE: 58
CALL: include('[ROOT]/cache/tpl_prosilver_overall_header.html.php')

FILE: [ROOT]/includes/template/template.php
LINE: 190
CALL: phpbb_template_renderer_include->render()

FILE: [ROOT]/includes/template/template.php
LINE: 445
CALL: phpbb_template->load_and_render()

FILE: [ROOT]/cache/tpl_prosilver_login_body.html.php
LINE: 1
CALL: phpbb_template->_tpl_include()

FILE: [ROOT]/includes/template/renderer_include.php
LINE: 58
CALL: include('[ROOT]/cache/tpl_prosilver_login_body.html.php')

FILE: [ROOT]/includes/template/template.php
LINE: 190
CALL: phpbb_template_renderer_include->render()

FILE: [ROOT]/includes/template/template.php
LINE: 174
CALL: phpbb_template->load_and_render()

FILE: [ROOT]/includes/functions.php
LINE: 5436
CALL: phpbb_template->display()

FILE: [ROOT]/includes/functions.php
LINE: 3322
CALL: page_footer()

FILE: [ROOT]/adm/index.php
LINE: 33
CALL: login_box()

I hope this all makes sense. If I could I would provide a patch but I'm not sure where to even begin

[Oleg]

This is a tricky issue. Thank you for the detailed report.

The first important point probably is that "all" style is, currently, not a fallback. It is as a matter of fact not a full fledged style at all. It is simply a place where extensions can put non-style-specific event implementations.

The use case for "all" is, for example, google analytics javascript.

Because "all" is not a style you cannot directly render a template which only exists under "all". As all pages are supposed to have a header and a footer I am not sure what it would mean to render an "all" template at top level. Therefore,

The second issue I noticed is that if I call the controller like so mysite/app.php?controller=mypage/test which uses the page_body.html for display I get:

Code: Select all

style resource locator: File for handle body does not exist. Could not find: ./styles/prosilver/template/page_body.html, ./styles/prosilver/template/page_body.html

What do you expect this to produce?

The first issue I noticed is that the template event works just fine but the my_content.html cannot be located:

Code: Select all

style resource locator: File for handle my_content.html does not exist. Could not find: ./styles/prosilver/template/my_content.html, ./styles/prosilver/template/my_content.html

This sounds like a use case we should be supporting.

[Blitze]

This is a tricky issue. Thank you for the detailed report.

The first important point probably is that "all" style is, currently, not a fallback. It is as a matter of fact not a full fledged style at all. It is simply a place where extensions can put non-style-specific event implementations.

The use case for "all" is, for example, google analytics javascript.

Because "all" is not a style you cannot directly render a template which only exists under "all". As all pages are supposed to have a header and a footer I am not sure what it would mean to render an "all" template at top level. Therefore,

The second issue I noticed is that if I call the controller like so mysite/app.php?controller=mypage/test which uses the page_body.html for display I get:

Code: Select all

style resource locator: File for handle body does not exist. Could not find: ./styles/prosilver/template/page_body.html, ./styles/prosilver/template/page_body.html

What do you expect this to produce?

The author of an extension cannot possibly be expected to account for all possible styles that the end user might install on their board. Therefore, I think of the "all" fallback as a very important implementation for extensions to work properly.
Staying with the same example as above, I have an extension that users can use to create pages (using the controller feature). The page is then displayed using the page_body.html template file. I expect that the extension will place this template file in an "all" folder, that way, regardless of what template the end-user is using, the page_body.html will still be rendered. Now, if there is need to further customize the page_body.html template to a particular style, we can place the customized version in a 'style-name' folder. I'm I looking at this wrong?

BTW: There is another problem that I did not mention above. When 'all' is enabled, the my_content.html is included twice for some reason. I should also mention this was working fine imkingdavid's repo branch (feature/controller) but when it was split to feature/controller-new there were some changes made that caused this. I reverted some of those changes to includes/style/extension_path_provider.php (provided phpbb_root_path to class and used it in the 'find' method) and now I am able to login to the ACP just fine. Only problem left is this duplication. I suspect it has something to do with the changes to includes/style/resource_locator.php

[Oleg]

Now, if there is need to further customize the page_body.html template to a particular style, we can place the customized version in a 'style-name' folder.

The only template I can think of that we have which is not style-specific is bbcode.html. All actual UI templates are specific to their styles. Based on this I am inclined to say that:

1. You should place page_body.html in one of the actual styles during development of your extension, because when your extension is finished the file will be style-specific.

2. We may want to support having a single bbcode.html template in the tree, because it is indeed not style-specific.

BTW: There is another problem that I did not mention above. When 'all' is enabled, the my_content.html is included twice for some reason. I should also mention this was working fine imkingdavid's repo branch (feature/controller) but when it was split to feature/controller-new there were some changes made that caused this. I reverted some of those changes to includes/style/extension_path_provider.php (provided phpbb_root_path to class and used it in the 'find' method) and now I am able to login to the ACP just fine. Only problem left is this duplication. I suspect it has something to do with the changes to includes/style/resource_locator.php

Can you create a git repo with an extension demonstrating this problem?

[Blitze]

May be I don't understand how this is meant to work but say I have the following controller:

Code: Select all

public function foo()
{
    $this->template->assign_var('BAR', 'baz');

    if (true === false)
    {
        $this->helper->error('True is somehow identical to false. The world is over.', 500);
    }

    return $this->helper->render('foo_body.html', 'Page Title');
}

If I put foo_body.html in ext/blitze/test/style/prosilver/template/foo_body.html and the end-user is using some style that does not inherit from prosilver, how would foo_body.html be rendered. My thinking is that for this very situation, it would be best to put foo_body.html in ext/blitze/test/style/all/

You can see the proposed changes here: https://github.com/blitze/phpbb3/commit ... d_all_path

[imkingdavid]

Currently, if a template file is required in a normal phpBB file, and it is not found in the inheritance tree, there is an error. That is how it is supposed to happen.

The 'all' folder is meant as a template event catchall, not an include catchall. Any files in 'all' should be non-style specific; in other words, the display of the snippet will not change much regardless of the style used. If there is any template-specific code in the file, it should go in that specific style folder.

If the user is going to use a style that is not prosilver or subsilver2 based, the template file you make and put in the all folder is not likely to be properly formatted anyway (it will have different tags and/or css names and such), so it wouldn't make sense to show the folder. It is the user's responsibility to modify the file; if they are using a completely custom format they are likely to know how to modify the extension files anyway.

[Blitze]

Well, I'm either not expressing myself correctly or I'm just not understanding how this is meant to work. So I'll stand back and watch how this turns out