This PR was merged into the master branch.
Commits
-------
f4f88a5 added an include function
Discussion
----------
added an include function
---------------------------------------------------------------------------
by stof at 2012-12-09T14:12:25Z
I see an issue with this naming: it will confuse Symfony users because of the ``render`` tag doing something different than the ``render`` function. And it will force to break BC for Silex users where the subrequest logic is using a ``render`` function right now
---------------------------------------------------------------------------
by fabpot at 2012-12-10T12:49:57Z
@stof: I know about the possible confusion. For Silex, as it is not stable yet, we don't need to keep BC. And I don't have any other better name. What I want to introduce next is a `render_request` function in both Silex and Symfony (which will do the same as the current render tag in Symfony and the render function in Silex).
---------------------------------------------------------------------------
by Taluu at 2012-12-10T14:13:31Z
Hi,
I may be absent-minded, but I don't really understand the difference between `render` function and the `include` tag... ?
---------------------------------------------------------------------------
by fabpot at 2012-12-10T14:16:09Z
@Taluu Using a function allows you to do whatever you want with the output (which is not possible with a tag), like a simple:
```jinja
{{ set content = render('some_template') }}
```
Also, semantically, using a tag to output some content is less correct.
---------------------------------------------------------------------------
by vicb at 2012-12-10T14:18:17Z
could it be named `include` ?
---------------------------------------------------------------------------
by tcz at 2012-12-10T14:18:40Z
I agree that it's confusing for Symfony users. It is the same as raw filter and raw tag. I would prefer execute as name or something like that
---------------------------------------------------------------------------
by Taluu at 2012-12-10T14:19:29Z
@fabpot so, does it means that it could replace the include tag ? Then, we could could name it `include` as @vicb suggested... ?
---------------------------------------------------------------------------
by titomiguelcosta at 2012-12-10T14:21:51Z
I agree with @stof, naming it render will be confusing, parse or execute or compile are other names that come to my mind for this purpose.
---------------------------------------------------------------------------
by silentworks at 2012-12-10T14:24:36Z
How about calling it `partial`? even though you are rendering a full template, it is being included in another so this could be seeing as a partial of that template.
---------------------------------------------------------------------------
by stof at 2012-12-10T14:26:05Z
@titomiguelcosta parse is totally wrong. The function is not parsing the string you give it. you don't pass the content of the template but its name. and ``compile`` is also wrong. It is not compiling a template but rendering it.
---------------------------------------------------------------------------
by jmather at 2012-12-10T14:30:53Z
I think `render` is probably the most appropriate name, but also may cause confusion, however I don't have another suggestion that is nearly as clear as to context and meaning.
---------------------------------------------------------------------------
by emgiezet at 2012-12-10T14:31:26Z
Can you pass the array of params as an argument to the `render` function?
---------------------------------------------------------------------------
by titomiguelcosta at 2012-12-10T14:31:59Z
@stof true that we pass the filename as argument, but we end up parsing the file.
---------------------------------------------------------------------------
by stof at 2012-12-10T14:32:48Z
@emgiezet you mean passing some variables available in the included template ? Sure you can. See the doc. the argument is named ``variables``
---------------------------------------------------------------------------
by stof at 2012-12-10T14:34:36Z
@titomiguelcosta But the goal of the function is not to parse the file (btw, it may not parse it if the template was already compiled in the cache). The goal is to render the template.
Btw, I agree with @jmather. As far as Twig is concerned, ``render`` is the best name. The only issue comes from the fact that Symfony already use a ``{% render %}`` tag with a different meaning
---------------------------------------------------------------------------
by emgiezet at 2012-12-10T14:39:02Z
@stof thx a lot didn't noticed the doc link. Maybe `render_partial` will be good in this case?
---------------------------------------------------------------------------
by tcz at 2012-12-10T14:44:13Z
In an ideal world the Symfony2 `render` tag would be renamed to `subrequest` or something like that and the new function in Twig would get the `render` name. The question is how many people use Twig because of Symfony2 and how many of them use it in itself (in my company we use standalone Twig for example).
I think it's important to avoid `raw`-style confusions for the former group.
---------------------------------------------------------------------------
by stof at 2012-12-10T15:16:28Z
@tcz The issue with renaming the tag in Symfony is that it would be a BC break. So we cannot simply drop the tag (we can provide a new name and deprecate the current one, but we would still keep it).
---------------------------------------------------------------------------
by Taluu at 2012-12-10T15:18:56Z
I think `partial` would be a good idea... But still, does it mean that the `include` tag would be deprecated in favor of `render` (or whatever the name will be) function ?
---------------------------------------------------------------------------
by tcz at 2012-12-10T15:19:49Z
@stof Yes :( hence my remark "in an ideal world"
---------------------------------------------------------------------------
by stof at 2012-12-10T15:27:52Z
@Taluu It depends if @fabpot wants to keep the compatibility with Jinja where possible (as it is where the ``include`` tag comes from)
---------------------------------------------------------------------------
by WouterJ at 2012-12-10T15:48:26Z
I like the function, but I don't like the name. I like the name `partial`.
I am a front-end developer from origin and all files you include in the base CSS stylesheet are called 'partials' in SASS or other preprosessors. Twig is used to make live easier for front-end developers and I think a function called `partial` is more familiair for them than `render`.
---------------------------------------------------------------------------
by silentworks at 2012-12-10T16:17:24Z
@stof I guess based on @fabpot wanting to keep Twig compatibility with Jinja, calling the tag `partial` rather than `render` shouldn't be a problem since Jinja doesn't have any of these.
---------------------------------------------------------------------------
by Taluu at 2012-12-10T16:19:25Z
@silentworks @stof was talking about my question, about why keep the `include` tag if the `render` (or whatever the name will be) is indeed inserted into Twig. :)
---------------------------------------------------------------------------
by stof at 2012-12-10T16:19:38Z
@silentworks I was not talking about the name of the function, but about the removal of the ``include`` tag
---------------------------------------------------------------------------
by silentworks at 2012-12-10T16:22:37Z
Thanks for clarity from both.
On the basis of keeping to conformity to Jinja, should this function not be a part of twig extensions rather than in the core?
---------------------------------------------------------------------------
by mvrhov at 2012-12-10T17:20:03Z
Well search and replace for symfony users when upgrading shouldn't be that hard so I'd say that we rename the symfony block into subrequest or sth similar.
---------------------------------------------------------------------------
by Taluu at 2012-12-10T17:26:22Z
If we were to follow your recommandations, we could say the same for all and every changes inducing a BC break huh...
---------------------------------------------------------------------------
by markstory at 2012-12-10T17:49:36Z
What about naming the function `include`? It would share a name with the tag, but it also does the same type of thing as the tag. The function has the advantage of having its return captured. I think `{{ include(template) }}` semantically does what it looks like it does when operating on the template object as well.
Other names that I think could fit well are `partial` and `display`. Render works well, but I understand people not wanting to overlap with existing symfony features that do very different things.
---------------------------------------------------------------------------
by Baachi at 2012-12-10T18:13:25Z
I think "partial" is a better name for this function and it would keep BC.
This PR was squashed before being merged into the master branch (closes#894).
Commits
-------
7c5854b Fix twig error lineno off by 2
Discussion
----------
Fix twig error lineno off by 2
It seems to me that the lineno reported by Twig_Error is off by 2 from the real lineno in the generated PHP source
For example:
```
{% if true %}
Yes
{% endif %}
```
```php
<?php
/* */
class __TwigTemplate_d41d8cd98f00b204e9800998ecf8427e extends Twig_Template
{
public function __construct(Twig_Environment $env)
{
parent::__construct($env);
$this->parent = false;
$this->blocks = array(
);
}
protected function doDisplay(array $context, array $blocks = array())
{
// line 1
if (true) { // <- this is line 19
// line 2
echo "Yes // <- this is line 21
";
}
}
public function getTemplateName()
{
return null;
}
public function isTraitable()
{
return false;
}
public function getDebugInfo()
{
return array ( 19 => 2, 17 => 1,); // <- should be return array ( 21 => 2, 19 => 1,);
}
}
```
---------------------------------------------------------------------------
by char101 at 2012-11-08T08:23:57Z
Since `$compiler->addDebugInfo()` is called first before writing the node content, usually the number of lines in the PHP code is less than 2 (1 for the // lineno comment, and 1 for the generated content) than the line where the node content is written.
This PR was merged into the master branch.
Commits
-------
f99504a changed the date filter behavior to always apply the default timezone, except if false is passed as the timezone
d3b0cbd merged branch jmikola/patch-1 (PR #845)
a6a1ef0 Avoid setting timezones on DateIntervals
Discussion
----------
changed the date filter behavior to always apply the default timezone, except if false is passed as the timezone
This PR is based on #845, and addresses issue #778.
Right now, the timezone management when using the date filter is counter-intuitive. When you pass a DateTime object, the default timezone is not applied, but in all other cases, it is.
This PR makes the behavior more consistent by always applying the default timezone (which is probably what users want most of the time), and this can be disabled by explicitly passing ``false`` as the timezone value.
This is a BC break but I think that not many consciously rely on the current behavior.
---------------------------------------------------------------------------
by fabpot at 2012-10-30T15:49:13Z
@jmikola Can you have a look at this PR and tell me your opinion about this change?
---------------------------------------------------------------------------
by jmikola at 2012-11-02T20:17:30Z
@fabpot: Will take a look at this tonight. I missed the notifications as I was offline for a few days.
---------------------------------------------------------------------------
by jmikola at 2012-11-03T04:09:54Z
I read through all of the issues and PR's and this looks reasonable. Are we missing a test case for passing in a DateTime object with `false` for the timezone? I do see one example where we render the current date using the `'e'` format string and provide `false` for the timezone, but that's it.
---------------------------------------------------------------------------
by fabpot at 2012-11-03T07:09:08Z
ok, I've added an additional test just to be sure.
This PR was merged into the master branch.
Commits
-------
a6a1ef0 Avoid setting timezones on DateIntervals
Discussion
----------
Avoid setting timezones on DateIntervals
Edit: originally, I opened this to ensure the default Twig timezone was always set on incoming DateTime objects, but I came to realize why that behavior would be undesirable. The idea was prompted by this line from: http://twig.sensiolabs.org/doc/filters/date.html
> The default timezone can also be set globally by calling `setTimezone()`.
Anyway, I did notice an edge case where the code might call `DateInterval::setTimezone()`, so this PR attempts to correct that. Additionally, I added some tests for passing a DateTimeZone object to the `date` filter.
---------------------------------------------------------------------------
by jmikola at 2012-09-22T17:18:56Z
@fabpot: One question came up as I was looking through this a second time: if `twig_date_converter()` receives a DateTime argument, it will override its timezone regardless of the logic path within (either from the `$timezone` argument, Twig default, or the system default).
At first, I thought that it was redundant for `twig_date_format_filter()` not to utilize `twig_date_converter()`, but it looks like that was a careful decision; otherwise, there'd be no way to display DateTimes with their internal timezone. This PR would remove that functionality, so I need to reconsider.
Having said that, there does appear to be a bug in `twig_date_format_filter()` in that it could end up calling a nonexistent `setTimezone()` method on a DateInterval object. Would it be preferable to throw an exception or simply ignore the `$timezone` argument if the `$date` argument is a DateInterval?
---------------------------------------------------------------------------
by fabpot at 2012-10-30T08:56:31Z
I don't know if not changing the timezone on DateTime object was a conscious/careful decision or not. The documentation is not so clear about that, and there is even a bug report about this behavior: #778.
For consistency, I think we can always set the timezone and allow disabling that by passing `false` for the timezone.
Does it sounds good?
This PR was merged into the master branch.
Commits
-------
d5df200 * bug fixes. * str_split on empty delimeter. * tests and documentation updated.
e3c201a Added "split" filter aka explode.
Discussion
----------
Split filter fix
Modification of request #621:
str_split, bug fixes
---------------------------------------------------------------------------
by senz at 2012-08-21T05:56:51Z
@fabpot this is squashed and fixed (hopefully) #621
- Fix extension memory leak
- Fix gcc compile warning (cast to const char *)
- Add TWIG_HAS_DYNAMIC_PROPERTY
- Remove the previous translation of 'array_key_exists($item, $object)'
into TWIG_ARRAY_KEY_EXISTS when checking object property because what
it really means is property_exists which has already been handled by
TWIG_HAS_PROPERTY and TWIG_HAS_DYNAMIC_PROPERTY
- Fix native extension spacing
Commits
-------
36372c6 Made node tests reusable
Discussion
----------
Reusable tests and fixtures
This PR makes Twig integration testing reusable for 3rd party extensions.
I have implemented resuable functional tests.
The [test runner](https://github.com/zikula/core/blob/master/src/Zikula/Bundle/CoreBundle/Tests/Twig/IntegrationTest.php) and the [fixtures](https://github.com/zikula/core/tree/master/src/Zikula/Bundle/CoreBundle/Tests/Twig/Fixtures) are the only part that an extension needs to implement itself.
I've included documentation in the PR.
---------------------------------------------------------------------------
by jusuff at 2012-08-11T20:41:55Z
This PR would make things little easier. Right now tests are organized in confusing way and are hard to implement for Twig extensions
---------------------------------------------------------------------------
by drak at 2012-08-14T09:09:45Z
@fabpot - This is ready for merging now. When it's merged I'll update the documentation as this PR significantly eases our ability to test extensions.
---------------------------------------------------------------------------
by fabpot at 2012-09-16T14:28:16Z
@drak Do you plan to update this PR with the code you have in Zikula?
---------------------------------------------------------------------------
by drak at 2012-09-16T14:37:30Z
@fabpot - Yes, I just need a couple of days (or less).
---------------------------------------------------------------------------
by drak at 2012-09-17T12:00:34Z
@fabpot - I have ported over the code.
---------------------------------------------------------------------------
by drak at 2012-09-18T08:23:18Z
@fabpot - I updated Zikula to us this particular branch so there is a practical test. I've updated the links in the top description part of this PR with the relevant links. The PR also refactors Twig's core tests to use the re-usable test base classes.
---------------------------------------------------------------------------
by fabpot at 2012-09-18T09:01:10Z
@drak Can you squash your commits before I merge? Thanks.
---------------------------------------------------------------------------
by drak at 2012-09-18T09:14:08Z
@fabpot - I've squashed them now. Thanks!
Commits
-------
7e5acd1 fixed some possible warnings
f0d0d6d fixed typo
b7076fe added Twig_Loader_Filesystem::getNamespaces()
8f7ccd1 moved an exception
9426072 tweaked documentation
b9afa84 renamed the default filesystem namespace to __main__
c34541d changed notation of namespaced templates to @namespace/template_path
0eb4d01 added namespaced templates support in Twig_Loader_Filesystem
c24ea1b added Twig_Loader_Filesystem::prependPath()
Discussion
----------
added namespaced templates support in Twig_Loader_Filesystem
Everything is explained in the updated documentation.
Basically, it gives more flexibility when it comes to manage many templates that are not necessarily related (think frontent vs backend for instance, or templates from different Symfony bundles, ...). It is useful if, for each namespace, you need to look for templates in different directories. This implementation would probably be enough to replace the current Symfony bundle template paths, with more flexibility and less restrictions.
Feedback is more than welcome. One thing I'm not sure about is the `#` separator between the namespace and the template path. Is it the best choice? We need a character that is not used in filesystem paths, so good candidates are: `@`, `!`, `%`, `$`, `*`, `;`, `?`.
Some immediate benefits: give the same template flexibility that we have in Symfony to other frameworks using Twig like Silex. Another benefit would be the ability to share template between Symfony and other frameworks like Twig (more on a proof of concept later).
---------------------------------------------------------------------------
by alessandro1997 at 2012-07-14T16:25:42Z
This is very nice, and I think # is a great choice for the separator character. We could also use @.
---------------------------------------------------------------------------
by jjbohn at 2012-07-14T16:25:59Z
👍 for #
---------------------------------------------------------------------------
by Symfomany at 2012-07-14T16:27:45Z
Pipe character isn't candidate?
---------------------------------------------------------------------------
by inspiran at 2012-07-14T16:32:52Z
I remember from the wetter.com talk at sf live that they actually implemented a similar functionality: allow twig templates to be defined different from the default path. So they had something like:
{% extends "cms://Home/weather/..." ... %}
If we could make the "cms" part a configurable setting then one could use it to point to a custom folder or even to a content provider (symfony cmf?)
Maybe that would be another approach?
---------------------------------------------------------------------------
by fabpot at 2012-07-14T16:33:21Z
The other syntax possibility is something more like we have in Symfony: `@namespace/template_path`.
---------------------------------------------------------------------------
by fabpot at 2012-07-14T16:34:37Z
@inspiran: that's another possibility: use the PHP stream notation.
---------------------------------------------------------------------------
by mvrhov at 2012-07-14T16:36:18Z
I'd also prefer uri/php streams syntax...
---------------------------------------------------------------------------
by Tobion at 2012-07-14T16:41:16Z
On Windows I can create a file with `#` in it. How does it distinguish that? I don't think we will find a single seperation character that works on all file systems, do we? And `@` (e.g. namespace@file) would be semantically strange (unless you read it from right-to-left).
So I'd also prefer uri/php stream syntax.
---------------------------------------------------------------------------
by Symfomany at 2012-07-14T16:43:17Z
@fabpot Pointer Syntax? or greater than character?
---------------------------------------------------------------------------
by markstory at 2012-07-14T16:43:48Z
I'm in favour of the `foo://` style syntax. It is similar to other parts of PHP, and avoids overlap with filesystem characters, and doesn't introduce new unique syntax.
---------------------------------------------------------------------------
by stof at 2012-07-14T16:44:25Z
@fabpot ``@namespace/template_path`` would make it confusing in Symfony. Because locating a resource with the kernel would use ``@AcmeDemoBundle/Resources/views/layout.html.twig`` whereas Twig would find the same file as ```@AcmeDemoBundle/layout.html.twig``.
And none of ``#``, ``@``, ``!``, ``%``, ``$`` or ``;`` is forbidden in filenames on Windows.
So I think the URI/stream notation is fine
---------------------------------------------------------------------------
by maryo at 2012-07-14T16:46:44Z
On Windows @!%$; are valid. Invalid characters are \/:*?"<>|
---------------------------------------------------------------------------
by maryo at 2012-07-14T16:47:55Z
Is > allowed on Linux?
---------------------------------------------------------------------------
by jschreuder at 2012-07-14T16:49:17Z
Some thoughts from a non-Symfony user:
* Not sure about the `#`, on the one hand it is used as the comment character on the other hand I'm also familiar with it as a membership operator (which supports its usage)
* The streams/PHP syntax: I don't really like this, the syntax signifies some type of protocol to be used. Even stretched to the limits of its definition you wouldn't be able to define different namespaces as different protocols (same protocol, different location).
* `@namespace/file/path` - kind of like this one, the `@` signifies that it is followed by a special symbol (being the namespace name) and other than that a normal path.
* Another option we've used in Fuel is the double colon `::` as in `namespace::file/path` (like the `#` it signifies membership, but doesn't have another significance in Twig)
---------------------------------------------------------------------------
by gunnarlium at 2012-07-14T17:10:15Z
Does it matter what symbols are valid in filenames? Won't the separator just be used for exploding the string into namespace and path, and thus never be used to directly access a file?
---------------------------------------------------------------------------
by alessandro1997 at 2012-07-14T17:15:27Z
As @gunnarlium said, what's the problem with filenames?
```php
$path = 'namespace#new#page.html';
$parts = explode('#', $path, 2);
list($namespace, $filename) = $parts;
// "namespace and "new#page.html"
var_dump($namespace, $filename);
```
---------------------------------------------------------------------------
by stof at 2012-07-14T17:16:19Z
@gunnarlium the issue is that if you use this symbols in the filename of your template without namespace, Twig will consider it as a namespaced template for another file name
---------------------------------------------------------------------------
by stof at 2012-07-14T17:16:45Z
@alessandro1997 the issue is that namespaces are optional
---------------------------------------------------------------------------
by alessandro1997 at 2012-07-14T17:17:07Z
@stof Oh, right. I didn't get that :-)
---------------------------------------------------------------------------
by gunnarlium at 2012-07-14T17:20:32Z
@stof Ok, I see. So what we want is a symbol which is not allowed as a filename? Or (suboptimally) just add a requirement that template filenames can't contain certain reserved symbols?
---------------------------------------------------------------------------
by fabpot at 2012-07-14T17:23:37Z
But then, who is using `#` in a filename?
Anyway, I'm going to implement a version based on PHP streams to stick to the PHP way. Using a PHP stream might also allow us to get rid of the array loader.
---------------------------------------------------------------------------
by alessandro1997 at 2012-07-14T17:24:01Z
@gunnarlium But how would we check if the user is using reserved symbols in template names? Twig isn't aware of the available templates' names until the user requests them. And when he/she does it's too late because Twig doesn't know if the separator is used as namespace or if it's part of the filename.
---------------------------------------------------------------------------
by stof at 2012-07-14T17:33:38Z
@fabpot even if no sane guy would probably use ``#`` in a filename, it would mean that Twig assumes that all its userbase is sane :)
---------------------------------------------------------------------------
by lolautruche at 2012-07-14T18:51:53Z
-1 for #
+1 for php stream, though I agree with @jschreuder's arguments against it
+2 for @ syntax like in Symfony. I find it more consistent.
---------------------------------------------------------------------------
by fabpot at 2012-07-14T19:50:31Z
One problem with PHP streams is that it makes using template names quite ugly and verbose:
{% include 'twig://namespace/index.html' %}
And for non-namespaced templates:
{% include 'twig:///index.html' %}
for which we can provide a shortcut:
{% include 'index.html' %}
---------------------------------------------------------------------------
by fabpot at 2012-07-14T19:54:35Z
If we are using `@` like in Symfony, it will be:
{% include '@namespace/index.html' %}
And for non-namespaced templates:
{% include 'index.html' %}
And for Symfony users, the notation would be the same as for resources. If we register each bundle path as a namespace, the path would even be exactly the same:
{% include '@AcmeDemoBundle/Resources/views/layout.html.twig' %}
EDIT: that's not so simple because of the templates stored under `app/`.
---------------------------------------------------------------------------
by mahono at 2012-07-14T19:56:54Z
might not be the best idea, but what about using Backslash as namespace char?
---------------------------------------------------------------------------
by fabpot at 2012-07-14T19:57:50Z
@mahono: I have not dared to propose this alternative ;)
---------------------------------------------------------------------------
by fabpot at 2012-07-14T20:06:17Z
I've just made an additional commit that implements the `@namespace/template_path` notation.
---------------------------------------------------------------------------
by Baachi at 2012-07-14T20:32:41Z
The `@` syntax is the most consistent solution.
But the integration in symfony2 is really hard and a BC break.
The stream syntax is not a good solution.
`twig` might already be already registered as a stream.
I would suggest the `::` syntax.
Fabien Potencier
Charles
Jeremy Mikola
Drak
Martin Hasoň
Fran Moreno