-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add test to check if a block exists #1831
Conversation
eec4ad2
to
780df1c
Compare
*/ | ||
public function existsBlock($name, array $context, array $blocks = array(), $useBlocks = true) | ||
{ | ||
$name = (string) $name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we really cast here ? IMO, we should expect a string here all the time. The casting could be done in the compiled code of the Twig_Node_Expression_Test_Block
. It would avoid casting again when calling the method on the parent
3c0d17d
to
b811fd8
Compare
@@ -0,0 +1,13 @@ | |||
``block`` | |||
======== |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing =
/** | ||
* Returns whether a block exists or not in the current context of the template. | ||
* | ||
* This method does return blocks defined in the current template |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it does not return blocks
public function compile(Twig_Compiler $compiler) | ||
{ | ||
$compiler | ||
->raw('$this->existsBlock((string) ') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string cast should be removed
Imho |
|
||
.. code-block:: jinja | ||
|
||
{% if 'title' is block %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I'm reading it in a template, using a test looks wrong to me: 'title'
is a string, not a block.
I was thinking about using a function instead like has_block('title')
:
{% if has_block('title') %}
<title>{{ block('title') }}<title>
{% endif %}
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer a test instead of a function. In #1821 I suggested {% if 'title' is block name %}
, for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally the has_block
should check the existence of a block in the current template as same as Twig_Template::hasBlock and not the existence of a block in the context.
The trouble with is block
is similar to is constant
. More correct would be:
{% if post.status is same as(constant('Post::PUBLISHED')) %}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hason Twig_Template::hasBlock
does not take inheritance into account, which precisely defeats the use case for this test: if it does not take inheritance into account, there is no reason to do this check at runtime; the developer can know it when writing the template and one of the branches of your if
will always be dead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stof Sure. I just don't like the confusing name of the function has_block
, that gives the impression: "if the template has a block". That's why I prefer rather the test is block
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer is block name
rather than is block
036b91f
to
dae8d81
Compare
529c9a3
to
8afe7ed
Compare
@fabpot What do you think? |
|
||
.. code-block:: jinja | ||
|
||
{% if 'title' is block name %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still find this very weird to read and not easy to understand. I don't have any better proposal for now.
Closes in favor of #2239 |
This PR was merged into the 1.x branch. Discussion ---------- Add "is defined" support for block() replaces #1831, fixes #1821 I think reusing the semantic of the `defined` test is more idiomatic and easily discoverable. /cc @hason Commits ------- 3ce06af added 'is defined' support for block() 4f013e0 Add test to check if a block exists
…ortable way (fabpot) This PR was merged into the 1.x branch. Discussion ---------- Expose a way to access template data and methods in a portable way `Twig_Template` is an internal class, and most of its methods are marked as being `@internal` as they are not "safe" to use by end users. But some of its features are interesting like `renderBlock()` which is very useful when rendering emails for instance (see #1873). This PR tries to address both issues by marking the whole `Twig_Template` class as being internal and by exposing a new way to safely interact with a template besides the obvious `render()`/`display()` methods via `$twig->load("...")`. If also add some convenient methods like `hasBlock()` or `getBlocks()` (see ~#1831~ and #1882): ```php $template = $twig->load('index'); echo $template->render($context); $template->display($context); if ($template->hasBlock('name') { $template->displayBlock('name', $context); } foreach ($template->getBlocks() as $block) { echo $template->render($block, $context); } ``` Commits ------- d7062f3 exposed a way to access template data and methods in a portable way
Fixes #1821