New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New template engine for 3.1 #171

Merged
merged 87 commits into from Aug 14, 2011

Conversation

Projects
None yet
5 participants
@p
Contributor

p commented Apr 24, 2011

Marek A. Ruszczynski and others added some commits Mar 7, 2011

[feature/template-engine] Adjust path in includephp template.
Now that tests are run from top level the '..' is wrong.

PHPBB3-9726
[feature/template-engine] Fix recompilation logic.
Do not change $recompile from true to false - any recompilation
condition alone is sufficient to force recompilation.

Also uncomment the nonexistent file test which passes with this fix.

PHPBB3-9726
[feature/template-engine] Allow leading underscores in variable names.
Subsilver uses ._file in overall_header.

PHPBB3-9726
Show outdated Hide outdated phpBB/includes/template.php Outdated

igorw and others added some commits Nov 23, 2010

[ticket/9924] Pass template instance into $template->display hook
This is a cherry-pick of 053cf79
which appears to have been partially reverted here.

PHPBB3-9924
[feature/template-engine] Deleted $template from phpbb_template_compi…
…le class.

phpbb_template_compile is now much simpler. It takes complete file paths
as inputs, either source template path or source template path and output
compiled template path. The number of methods also went down to two -
compile template and returned compiled text or compile and write to file.

phpbb_compile class is responsible for determining source and compiled
paths. It already had all the data necessary for this, now the code is
in the same place as the data it uses.

PHPBB3-9726
[feature/template-engine] Deleted silencing of notices.
The code is now supposed to be notice-free, therefore there is no need
to have the notices silenced.

PHPBB3-9726
@p

This comment has been minimized.

Show comment
Hide comment
@p

p Apr 25, 2011

Contributor

There are some lines in template.php that do not appear in the diff that seem like they stand no chance of working given the changes in the diff:

$compile->compile_write($handle, $this->compiled_code[$handle]);
$compile->compile_write($row['template_filename'], $compile->compile(trim($row['template_data'])));

Contributor

p commented Apr 25, 2011

There are some lines in template.php that do not appear in the diff that seem like they stand no chance of working given the changes in the diff:

$compile->compile_write($handle, $this->compiled_code[$handle]);
$compile->compile_write($row['template_filename'], $compile->compile(trim($row['template_data'])));

[feature/template-engine] Added a test for multilevel references in l…
…oops.

This currently fails.

This test is a reduced version of permission_mask template in acp, which
is not correctly compiled by the current template engine code.

PHPBB3-9726
@p

This comment has been minimized.

Show comment
Hide comment
@p

p Apr 25, 2011

Contributor

Other questionable practices:

https://gist.github.com/939870 line 226: isset(foo) && !foo - what happens when foo is not set?

Line 6: - why is this going through php?

Contributor

p commented Apr 25, 2011

Other questionable practices:

https://gist.github.com/939870 line 226: isset(foo) && !foo - what happens when foo is not set?

Line 6: - why is this going through php?

p added some commits Apr 25, 2011

[feature/template-engine] Corrected an off-by-one error in nested nam…
…espaces.

This error resulted in a dot from the namespace being placed into
variable reference in compiled template code, thus creating bogus
compiled template code.

PHPBB3-9726
[feature/template-engine] Created a script to compile templates.
Script takes path to template as the only argument and outputs
the compiled template to standard output.

PHPBB3-9726
@p

This comment has been minimized.

Show comment
Hide comment
@p

p Apr 25, 2011

Contributor

The aliasing that is being done for nested loops may likely prohibit reuse of loop identifiers within a template.

Contributor

p commented Apr 25, 2011

The aliasing that is being done for nested loops may likely prohibit reuse of loop identifiers within a template.

@igorw igorw closed this Apr 25, 2011

@igorw igorw reopened this Apr 25, 2011

Show outdated Hide outdated phpBB/includes/template.php Outdated
@igorw

This comment has been minimized.

Show comment
Hide comment
@igorw

igorw Apr 25, 2011

Contributor

Line 6 is possibly because some server side scripting environments use script tags for server side stuff. Not sure if PHP ever did this or it is even relevant, though.

Line 226 looks bad though, should probably be if empty(...)

Do you have an example for the loop identifiers?

Testing now.

Contributor

igorw commented Apr 25, 2011

Line 6 is possibly because some server side scripting environments use script tags for server side stuff. Not sure if PHP ever did this or it is even relevant, though.

Line 226 looks bad though, should probably be if empty(...)

Do you have an example for the loop identifiers?

Testing now.

@igorw

This comment has been minimized.

Show comment
Hide comment
@igorw

igorw Apr 26, 2011

Contributor
Contributor

igorw commented Apr 26, 2011

p and others added some commits Apr 25, 2011

[feature/template-engine] Added a test for reuse of loop identifiers.
This currently does not pass, thus it is commented out.

The reuse appears implausible in the same file, however it may be
also done across template files where it is much harder to detect.

PHPBB3-9726
[feature/template-engine] Fix negative variable expressions
compile_tag_if had the flawed approach of adding an isset statement for
all variables to the beginning of the if. This fails for negative
expressions, and checking those takes a considerable effort.

The easier solution is to make the variable expression itself
conditional, defaulting to null if it is not set.

Thanks to naderman for the solution.

PHPBB3-9726
@igorw

This comment has been minimized.

Show comment
Hide comment
@igorw

igorw Apr 26, 2011

Contributor

No more issues encountered when testing. It's pretty much ready to go. If no other major issues arise this can be merged tonight.

Contributor

igorw commented Apr 26, 2011

No more issues encountered when testing. It's pretty much ready to go. If no other major issues arise this can be merged tonight.

Show outdated Hide outdated phpBB/includes/template.php Outdated
@bantu

This comment has been minimized.

Show comment
Hide comment
@bantu

bantu Apr 26, 2011

Member

At least public functions need docblocks, better all functions.

Member

bantu commented Apr 26, 2011

At least public functions need docblocks, better all functions.

[feature/template-engine] Removed $this from new phpbb_template_compi…
…le calls.

The compile class no longer takes template as a parameter.

PHPBB3-9726

igorw and others added some commits Jul 16, 2011

[feature/template-engine] Docblocks, no more constructor for filter
Add docblocks for phpbb_template_filter, remove the useless constructor.

PHPBB3-9726
[feature/template-engine] Delete $files_template property.
This seems to have been used for db storage of templates.
We no longer offer db storage of templates, and thus currenty
$files_template is only written to but not read anywhere.

PHPBB3-9726
[feature/template-engine] Factor template locator out of template class.
Template locator is responsible for maintaining mapping from template
handles to filenames and paths, and provides resolution services
using these mappings.

Template locator is aware of template inheritance and is capable of
checking template file existence on the filesystem.

PHPBB3-9726
[feature/template-engine] Delete $style_name param from locator's set…
…_custom_template.

This parameter was unused, it was only used by template's set_custom_template
to determine cache file prefix.

PHPBB3-9726
[feature/template-engine] Get rid of orig_tpl_* in template engine.
The origins of orig_tpl_* are not pretty. Please see the following commits
and associated tickets: r9823, r9839, r9847, r10150, r10460.

In short, multiple hacks were required due to template engine reading
inheritance/storedb flags from $user (global) even when the template that
was being looked up or rendered was not the "active style of the
current user".

We no longer store templates in the database, removing half of the problem.
This commit fixes the second half of the problem by deleting
set_template_path function from template locator, and moving that logic
back into the template class' set_template. set_template now calls
set_custom_template, the latter only taking the template path and the
fallback paths as parameters. With this change template locator no longer
uses $user and does not use phpbb root path either.

All logic involving setting the user's "active" template is now
encapsulated in a single template class's function, set_template.
Setting other templates is done via set_custom_template and the caller
is responsible for determining and passing in fallback/inheritance path,
if any.

PHPBB3-9726
Show outdated Hide outdated phpBB/includes/bbcode.php Outdated
@p

This comment has been minimized.

Show comment
Hide comment
@p

p Aug 10, 2011

Contributor

All fixed.

Looks like we have no tests exercising the set_template code path, as p@fb8a1d9 demonstrates. I discovered that after installing the board manually for testing (and the fact that installer was broken was uncovered essentially by Nils' request to use DI, and fixed in p@60372b4).

Contributor

p commented Aug 10, 2011

All fixed.

Looks like we have no tests exercising the set_template code path, as p@fb8a1d9 demonstrates. I discovered that after installing the board manually for testing (and the fact that installer was broken was uncovered essentially by Nils' request to use DI, and fixed in p@60372b4).

Show outdated Hide outdated phpBB/includes/template/template.php Outdated
Show outdated Hide outdated phpBB/includes/bbcode.php Outdated

p added some commits Aug 14, 2011

[feature/template-engine] Delete _get_locator function.
It is no longer needed as locator is injected into template.

PHPBB3-9726
@p

This comment has been minimized.

Show comment
Hide comment
@p

p Aug 14, 2011

Contributor

Fixed and fixed.

I'd really like to work on something else now rather than debating cosmetics here. I'm sufficiently satisfied with the current version of the template engine.

If you really feel strongly about something cosmetic please propose your changes via a patch.

Contributor

p commented Aug 14, 2011

Fixed and fixed.

I'd really like to work on something else now rather than debating cosmetics here. I'm sufficiently satisfied with the current version of the template engine.

If you really feel strongly about something cosmetic please propose your changes via a patch.

@naderman

This comment has been minimized.

Show comment
Hide comment
@naderman

naderman Aug 14, 2011

Member

Those were the last 2 things I found, so going to merge this now :)

Member

naderman commented Aug 14, 2011

Those were the last 2 things I found, so going to merge this now :)

@naderman naderman merged commit 41de09e into phpbb:develop Aug 14, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment