Skip to content
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

Template call #1414

Open
wants to merge 8 commits into
base: development
Choose a base branch
from
Open

Template call #1414

wants to merge 8 commits into from

Conversation

joshuaadickerson
Copy link
Contributor

This makes templates infinitely pluggable by adding in 3 events for every template: pre, execute, post. You can effectively add before/after or not even call any/every template which makes themes very extensible.

It changes all calls of every template to use template_call()
If a template doesn't use template_call() it doesn't break anything so there are no backwards compatibility issues.

Although it can be added at any time, I'd still recommend some testing and code review to make sure I didn't miss anything or break anything.

See http://www.elkarte.net/community/index.php?topic=1077 for the discussion

Resubmitted to development branch from: #1403

Every template called through loadSubTemplate() now has hooks. You should be able to change almost everything in a theme without overwriting a single file.
Signed-off-by:Joshua Dickerson <[email protected]>
3/4 of the way done with changing all of the template calls and I realized that there is no point in returning anything but it will break things when you do echo template_call('some_template'); because it call_integration_hook() returns an array.
Signed-off-by:Joshua Dickerson <[email protected]>
…alled, the array won't be empty.

Signed-off-by:Joshua Dickerson <[email protected]>
@emanuele45
Copy link
Contributor

Apparently travis complains.
Judging from the errors seems in the "web" tests (i.e. those accessing a web page), should it be already working or it is expected to be broken?

I brokeded it
Templates with references won't work when called with template_call()
@emanuele45 emanuele45 added this to the 1.1 Beta milestone Feb 26, 2014
@emanuele45
Copy link
Contributor

Is there a specific reason why you added both a __pre and a __execute?
Technically they do the same thing, I would merge __execute into __pre and save one call (it also means one less function for who writes a mod and this is always something nice to me).

@joshuaadickerson
Copy link
Contributor Author

Initially they had return values. So the __pre() would have a different return value. __execute's return value changes whether it will execute or not. I guess there is no reason to have a pre and an execute now, but call __execute() with a return value could stop the execution of the template, so be careful.

@emanuele45
Copy link
Contributor

AHA!
This morning I understood what was bothering me about this PR. There was something "odd to me", but I was not able to understand what it was, now I know.
Two things:

  1. the use of call_integration_hook, this probably gives much more freedom than what I would like to see for a template, for example it allows to include files, and I feel this is a terribly bad thing to do in the template, it allows to execute logic that again is a bad thing, etc.
  2. (that is probably intended, but that due to 1 can be badly misused) is that the hooks are generic and match anything anywhere.

TBH my idea of the templating in general was a bit different (i.e. build a list of templates to be executed in a certain order and let it run, something like the layers), but I have to admit the idea of call_template is quite good and should give a lot more flexibility than mine.

TL;DR I wouldn't use call_integration_hook in this case, at the very least a custom function call_template_hook (or similar name) that just takes care of execute a template_something function or a lambda, but definitely not call_integration_hook.

Yep, what I wanted to say with __pre/__execute was just let __pre return the blocking value. Dunno. You know I tend to reduce duplications... maybe too much. lol

@joshuaadickerson
Copy link
Contributor Author

The reason I wouldn't want it to be a different call is because the plugin/event system should handle it the same. The name of the event is what differentiates it. No matter how you do it, the template system has the power to run PHP. If that's what people want to do with the templates, it will happen.

Take a look at what I'm doing with the event system https://gist.github.com/joshuaadickerson/9347177 to see the direction I'm working it to.

@emanuele45
Copy link
Contributor

Of course the template is php and anyone using it has already a lot of freedom, but to me there is a difference between "listen, since you can already do almost anything, feel free to do it, I don't care" and "here is the thing: you could do this, we highly discourage this behaviour and we set this limit, if you want to kill yourself, feel free, but don't complain with us if tomorrow it doesn't work".

For example, to load a template file there is a special function loadTemplate that is called way before any template functions are called.
That function doesn't only require the file, it does several other things.
A hook in the template shouldn't (in my book, that may be badly written and completely wrong) be able to load any file, just because is not the correct place a file should be loaded. I even moved all the button strips definition into the controllers to avoid this situation.

As far as I have always understood the theory should be:

  • source => lots of things including define what to do, require files, load templates, define variables to be used in the template, etc.
  • theme => calls the templates and renders the output. Plain and simple.

As I said I may be badly wrong. ;)

@joshuaadickerson
Copy link
Contributor Author

You're totally right, that's all the template should do. However, since
templates call each other, there is no way to stop one or change one from
calling another so the system relies a lot on the other parts of the MVC
system to handle what should be displayed. Even if you called it
template_hook(), that doesn't stop people from doing some kind of logic in
there. No matter what, it still needs to call another function. That
function is going to be part of a plugin (usually). That plugin can do
whatever it wants and there is nothing any other function in the entire
architecture can do to change that.

On Tue, Mar 4, 2014 at 11:12 AM, emanuele45 [email protected]:

Of course the template is php and anyone using it has already a lot of
freedom, but to me there is a difference between "listen, since you can
already do almost anything, feel free to do it, I don't care" and "here is
the thing: you could do this, we highly discourage this behaviour and
we set this limit, if you want to kill yourself, feel free, but don't
complain with us if tomorrow it doesn't work".

For example, to load a template file there is a special function
loadTemplate that is called way before any template functions are called.
That function doesn't only require the file, it does several other things.
A hook in the template shouldn't (in my book, that may be badly written
and completely wrong) be able to load any file, just because is not the
correct place a file should be loaded. I even moved all the button strips
definition into the controllers to avoid this situation.

As far as I have always understood the theory should be:

  • source => lots of things including define what to do, require files,
    load templates, define variables to be used in the template, etc.
  • theme => calls the templates and renders the output. Plain and
    simple.

As I said I may be badly wrong. ;)

Reply to this email directly or view it on GitHubhttps://github.com//pull/1414#issuecomment-36641173
.

@joshuaadickerson
Copy link
Contributor Author

I just thought about something. Using the event system is wrong here. The listener needs to know what the theme is so you don't call it for every theme. Then again, as an admin or developer, you might want the event to be triggered on every theme. That seems like a lot of administrative work.

@emanuele45 emanuele45 modified the milestones: 2.0, 1.1 Beta 1 Nov 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants