[RFC|Merged] Template locator and path finder

[Arty]

Introduction

I think template locator and path finder require some changes.

Sample situation: 2 templates, 3 extensions

Templates: progreen, which inherits from prosilver.
Extensions: ext1, ext2 and ext3. ext1 has prosilver templates, ext2 has prosilver and progreen templates, ext3 has "all" templates (see events)

Currently locator stores paths to templates in one dimensional array. With template inheritance and extensions, order of search looks like this:
1. styles/progreen/template/
2. styles/prosilver/template/
3. ext/ext1/styles/prosilver/template
4. ext/ext2/styles/progreen/template
5. ext/ext2/styles/prosilver/template
6. ext/ext3/styles/all/template <-- currently missing, but I think it should be added

That works great when including templates.

However, it doesn't work for events because events must include templates from all extensions.

In current version of events PR, template filter is responsible for locating templates, it uses finder for it without accounting for template inheritance. For example above, it means templates will be searched for in locations #1, #4 and #6. So it isn't searching half of directories that it should search in.

If locator is used to locate template instead of finder, it doesn't distinguish between extension and style, so sample template located within several styles in same extension, like #4 and #5, both will be included.

If all of those styles in example have template for event, correct way would be to include only templates from #1, #3, #4 and #6. #2 should not be included because #1 overrides it with template inheritance. #5 should not be included because #4 overrides it with template inheritance.


Suggestion

I suggest to solve it by changing from one dimensional list of paths to 2 dimensions: extension and paths:
['style'][0] = styles/progreen/template/
['style'][1] = styles/prosilver/template/
['ext1'][0] ext/ext1/styles/prosilver/template
['ext2'][0] = ext/ext2/styles/progreen/template
['ext2'][1] = ext/ext2/styles/prosilver/template
['ext3'][0] = ext/ext3/styles/all/template

When searching for templates for events, locator should look for first entry in each top level array. As soon as entry in second level array is found, locator should go to next top level array. Example: searching in ['style'][0] => if found, go to ['ext1'][0]. if not found, check ['style'][1]

This way only one template per extension will be included in event.

Also with this change main template path is gone. It wasn't functioning properly anyway.

Patch

Ticket: http://tracker.phpbb.com/browse/PHPBB3-10735
Patch: https://github.com/phpbb/phpbb3/pull/677

[MichaelC]

This looks very good to me.

[brunoais]

I suggest to solve it by changing from one dimensional list of paths to 2 dimensions: extension and paths:
[0] = styles/
[0][0] = styles/progreen/template/
[0][1] = styles/prosilver/template/
[1] = ext/ext1/styles/
[1][0] ext/ext1/styles/prosilver/template
[2] = ext/ext2/styles/
[2][0] = ext/ext2/styles/progreen/template
[2][1] = ext/ext2/styles/prosilver/template
[3] = ext/ext3/styles/
[3][0] = ext/ext3/styles/all/template

When searching for templates for events, locator should look for first entry in each top level array. As soon as entry in second level array is found, locator should go to next top level array. Example: searching in [0][0] => if found, go to [1][0]. if not found, check [0][1]

This way only one template per extension will be included in event. I'm not sure how much code changes it would require, but without it I don't see how template events could be implemented.

Hum... shouldn't it be something different?

Code: Select all

['phpbb_core'][0] = styles/ 
['phpbb_core']['progreen'] = styles/progreen/template/
['phpbb_core']['prosilver'] = styles/prosilver/template/
['ext1'][0] = ext/ext1/styles/
['ext1']['prosilver'] ext/ext1/styles/prosilver/template
['ext2'][0] = ext/ext2/styles/
['ext2']['progreen'] = ext/ext2/styles/progreen/template
['ext2']['prosilver'] = ext/ext2/styles/prosilver/template
['ext3'][0] = ext/ext3/styles/
['ext3']['all'] = ext/ext3/styles/all/template

?

If I understood correctly, what you have written is not even a valid php array.

[Arty]

That would work too, except for [whatever][0] - that item was for illustrative purposes only, it doesn't exist in array.

[MichaelC]

Does this need an RFC or as part of events/extensions?

[Arty]

I don't know. Without it or another similar solution events won't work properly, so I guess it should be a part of events.

[Arty]

I've changed topic from discussion to RFC and moved it to 3.2 RFCs forum.

After testing it, I've implemented these changes on my forum, using associative arrays that brunoais suggested.

Implementation of "all" directory requires changing template initialization function to add "all" as third fallback template path. Such change already exists in PR 625 because it can handle unlimited styles tree. Therefore, implementation of this RFC requires merge of PR #625

[MichaelC]

This is required for 3.1 events so it shouldn't it be in the 3.1 RFC's & Patches Forum?

[Arty]

Patch: https://github.com/phpbb/phpbb3/pull/677

[igorw]

Merged.