Open mallorydxw opened 4 years ago
templates/views
{% import '../../assets/images/foo.svg' %}
doesn't work because it doesn't allow directory traversal - a function may work$autoescape
to 'html'
Our experience of Timber on Wilton Park has left me uncertain as to whether or not it's a good thing. In theory, it still delivers real benefits, but the way it's structured seems particularly unfriendly to testing. Particular issues I encountered were:
.twig
templates to have an accompanying .php
file containing the logic encourages the accompanying .php
files to be big unstructured pieces of code that can be hard to read and debug (see https://github.com/dxw/wilton-park/blob/main/wp-content/themes/theme/templates/page-events.php or https://github.com/dxw/wilton-park/blob/main/wp-content/themes/theme/templates/page-application-form-process.php for a couple of examples)It might be that we can deal with the first couple of issues via Kahlan once we're a bit more familiar with it, and the third could potentially be remedied via team convention/code review. But I feel like we need more experience of it before we can really know if it's the right tool. In particular, I think we need to be sure it doesn't hinder our ability to write testable code.
If we do end up using Timber, we should use a linter for the twig templates, e.g. https://github.com/sserbin/twig-linter
I think the release of Timber 2.0 might help with getting the logic into testable classes. e.g. there's a timber/calling_php_file
action that we could potentially use to hook templates to classes of the same name, and automatically run all the logic there: https://timber.github.io/docs/v2/hooks/actions/#timber/calling_php_file
Any of the classes that are being hooked in like that could extend a parent class that automates that hooking in process maybe? We could make that parent class part of Iguana if needs be.
You can set your own locations for your twig files with:
Timber::$locations = '/Users/jared/Sandbox/templates';
Use the full file path to make sure Timber knows what you're trying to draw from.
For the svg inline, there is https://github.com/manuelodelain/svg-twig-extension
OR
if your SVG lies in a folder called assets/icons/ in the main folder of your theme, you add that folder to the list:
functions.php
Timber::$dirname = array( 'views', 'assets' );
Be sure to also include the name of the folder for your Twig templates in the array. Otherwise these won’t work anymore.
You can then do
{% include 'icons/my.svg' ignore missing %}
To display the SVG.
If you have only one file to include, it’s probably overkill to make Timber look in another directory. You could also use file_get_contents() and get_template_directory().
{{ fn('file_get_contents', fn('get_template_directory') ~ '/assets/icons/my.svg') }}
OR
simply rename the .svg as .twig (it's text right?) and include it.
I'm not sure what was the problem, because you can actually do that:
if ( class_exists('Timber') ) { Timber::$autoescape = 'html';}
But honestly I would prefer escaping one by one e.g.
{{ post.get_field('custom_link')|e('esc_url') }}
considering that Timber has
wp_kses_post
esc_url
esc_html
esc_js
1) its frequent use of static methods
could you please make an example of this?
2) the requirement to new up instances of classes on demand (e.g. when handling menus or pagination)
Half true: you can declare the menus on the global context, and you have to use a query (and its object) for the pagination only if it's a very specific one, otherwise usually you can use an easy get_pagination.
3) the approach of expecting .twig templates to have an accompanying .php file containing the logic
Half true again: you can declare twig files separated from a specific php, and the best way to set up the logic is not what we did into Wilton Park, but the general idea of Timber is to extend the basic classes to move the logic away from the .php theme files.
See: https://timber.github.io/docs/guides/extending-timber/#an-example-that-extends-timberpost
These can get pretty complex. And that's the beauty. The complexity lives inside the context of the object, but very simple when it comes to your templates.
In that case, I think we could easily use Kahlan too
- its frequent use of static methods
could you please make an example of this?
e.g. Timber::get_posts()
, Timber::query_post()
, Timber::fetch()
, Timber::get_terms()
etc. Generally, these are harder to mock (and therefore test) than instance methods, where you can mock and inject an instance of the class.
- the requirement to new up instances of classes on demand (e.g. when handling menus or pagination)
Half true: you can declare the menus on the global context, and you have to use a query (and its object) for the pagination only if it's a very specific one, otherwise usually you can use an easy get_pagination.
Looking at the documentation for declaring menus on the global context (https://timber.github.io/docs/guides/menus/#setting-up-a-menu-globally), it looks like this still requires a new \Timber\Menu()
call.
- the approach of expecting .twig templates to have an accompanying .php file containing the logic
Half true again: you can declare twig files separated from a specific php, and the best way to set up the logic is not what we did into Wilton Park, but the general idea of Timber is to extend the basic classes to move the logic away from the .php theme files.
See: https://timber.github.io/docs/guides/extending-timber/#an-example-that-extends-timberpost
These can get pretty complex. And that's the beauty. The complexity lives inside the context of the object, but very simple when it comes to your templates.
In that case, I think we could easily use Kahlan too
I do like the look of the way you can extend the post object. That's something I've wanted to do with standard WP_Post
objects in the past, and always been frustrated when you can't!
I think my main concern on this point was where there are e.g. parameters in the query string that should be used to determine what post query is run. The Timber docs don't really offer any pointers as how you should deal with a situation like that, so the obvious solution is to put all that logic into the .php file that lives with the .twig template (which as a result won't be testable, and possibly hard to maintain).
I guess maybe a couple of alternatives in a case like that might be to:
Timber\PostQuery()
set up based on the query string params, and just call that instance method in the .php file that lives with the template. That means all the logic would still be a in a testable class. pre_get_posts
in the appropriate contexts, and sets the query up that way, e.g. like https://git.govpress.com/ukri-website/app/-/blob/develop/wp-content/themes/ukri/app/PostTypes/Opportunity/Filters.php. In which case, just calling Timber::get_posts()
should retrieve the correct set of posts.
- Write a class that includes a method that can return a Timber\PostQuery() set up based on the query string params, and just call that instance method in the .php file that lives with the template. That means all the logic would still be a in a testable class.
I think we should consider to try and set up a new starter theme with this logic, and see how it works. Of course I would not spend too much time on that.
I don't like the second alternative because hooking on pre_get_posts
can result in unexpected behaviours, even with a set of tests.
Advantage: clean separation of templates and other code
Disadvantage: new syntax to learn
https://timber.github.io/docs/