Skip to content
This repository has been archived by the owner on Jan 1, 2020. It is now read-only.

[WIP] removal of duplicate about attribute in collections rendering #46

Merged
merged 10 commits into from
Feb 14, 2013
Merged

[WIP] removal of duplicate about attribute in collections rendering #46

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
73049ee
WIP removal of duplicate about attribute in collections
adou600 Jan 11, 2013
5dc9560
fix various things according to comments
adou600 Jan 15, 2013
e3f032f
twig template test update and refactoring
adou600 Jan 15, 2013
280c10e
README update with information about noautotag
adou600 Jan 15, 2013
8d549c6
new test for collections with the duplicate about attribute case
adou600 Jan 16, 2013
d146e4d
->parentMapping is now a string again
adou600 Jan 17, 2013
e3ea138
doc comments
adou600 Jan 18, 2013
caa8488
getTitle needs a value in determineEntityTitle
adou600 Feb 1, 2013
a885ae8
adjustments according to comments
dbu Feb 14, 2013
3395fd6
fix stupid bug
dbu Feb 14, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,14 +224,15 @@ name is generated by appending "_rdf" to your model name.
Inside the createphp tag, you can render the entity with the default structure
(see above), or render single fields as in the example below:

{% createphp mymodel %}
<div {{ createphp_attributes(mymodel_rdf) }}>
{% createphp mymodel noautotag %}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the first example should be using the autotag feature, and then we add a section how to have more control over the html from the template where we explain the noautotag

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually if you look at https://github.com/symfony-cmf/createphp/blob/d146e4d71dd0797ffc0c4ccc859c4cf9a8e2ab1a/README.md#twig-extension, the first example is using the autotag:
{% createphp mymodel %}{{ mymodel_rdf|raw }}{% endcreatephp %}
Is it ok like this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah sorry, just saw that this replaced something existing and thought there is no more example without noautotag. all fine then.

<div class="my-section" {{ createphp_attributes(mymodel_rdf) }}>
<h1 class="my-title" {{ createphp_attributes( mymodel_rdf.title ) }}>{{ createphp_content( mymodel_rdf.title ) }}</h1>
{{ mymodel_rdf.content|raw }}
</div>
{% endcreatephp %}

You need to render the attributes of the entity in a tag that encloses all of
With ``noautotag`` the enclosing elements are not rendered automatically. Therefore,
you need to render the attributes of the entity in a tag that encloses all of
your entity. Inside that tag, you can place arbitrary HTML and render single
fields or render attributes and content of the fields separately for closer
control over the generated HTML.
Expand Down
13 changes: 13 additions & 0 deletions src/Midgard/CreatePHP/Entity/Collection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,19 @@ public function renderStart($tag_name = false)
return parent::renderStart($tag_name);
}

/**
* {@inheritDoc}
*
* Overwrite to not output about attribute again if parent is rendering
*/
public function renderAttributes($attributesToSkip = array())
{
if ($this->_parent && $this->_parent->isRendering()) {
$attributesToSkip[] = 'about';
}
return parent::renderAttributes($attributesToSkip);
}

/**
* {@inheritDoc}
*
Expand Down
32 changes: 30 additions & 2 deletions src/Midgard/CreatePHP/Extension/Twig/CreatephpNode.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,17 +29,22 @@ class CreatephpNode extends Twig_Node
* @param Twig_NodeInterface $body The body node
* @param string $modelname The name of the model class to make an rdfa entity
* @param string $varname The name for the rdfa entity to expose
* @param boolean $autotag Automatically render start and end part of the node?
* @param integer $lineno The line number
* @param string $tag The tag name
*/
public function __construct(Twig_NodeInterface $body, $modelname, $varname, $lineno = 0, $tag = null)
public function __construct(Twig_NodeInterface $body, $modelname, $varname, $autotag, $lineno = 0, $tag = null)
{
$nodes = array('body' => $body);
if (empty($varname)) {
$varname = $modelname . '_rdf';
}

$attributes = array('varname' => $varname, 'modelname' => $modelname);
$attributes = array(
'varname' => $varname,
'modelname' => $modelname,
'autotag' => $autotag
);

parent::__construct($nodes, $attributes, $lineno, $tag);
}
Expand All @@ -59,9 +64,32 @@ public function compile(Twig_Compiler $compiler)

$compiler
->raw(";\n")
;

$autotag = $this->getAttribute('autotag');
if ($autotag) {
$compiler->write('echo ');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we start rendering even when we do not want to output, to set the node into the correct mode. it is up to the user to be intelligent enough to understand what he has to do if he is using noautotag.

}

$compiler
->write('$context[')
->repr($this->getAttribute('varname'))
->write(']->renderStart()')
->raw(";\n");

$compiler
->subcompile($this->getNode('body'))
;

if ($autotag) {
$compiler->write('echo ');
}
$compiler
->write('$context[')
->repr($this->getAttribute('varname'))
->write(']->renderEnd()')
->raw(";\n");

$compiler
->write('unset($context[')
->repr($this->getAttribute('varname'))
Expand Down
9 changes: 7 additions & 2 deletions src/Midgard/CreatePHP/Extension/Twig/CreatephpTokenParser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,20 @@ public function parse(Twig_Token $token)
$var = $stream->expect(Twig_Token::STRING_TYPE)->getValue();
}

$noautotag = false;
if ($stream->test(Twig_Token::NAME_TYPE, 'noautotag')) {
$noautotag = true;
$stream->next();
}

$stream->expect(Twig_Token::BLOCK_END_TYPE);

$endtag = 'end'.$this->getTag();
$test = function(Twig_Token $token) use($endtag) { return $token->test($endtag); };
$body = $this->parser->subparse($test, true);

$stream->expect(Twig_Token::BLOCK_END_TYPE);

return new CreatephpNode($body, $modelname, $var, $token->getLine(), $this->getTag());
return new CreatephpNode($body, $modelname, $var, !$noautotag, $token->getLine(), $this->getTag());
}

public function getTag()
Expand Down
23 changes: 13 additions & 10 deletions src/Midgard/CreatePHP/Node.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* attributes, parent/children relations and rendering. The latter is split into three
* different functions for maximum flexibility. So you can call render() to output the
* complete node HTML, or you can call render_start() for the opening tag, render_content()
* for the node's content (or children) and render_end() for the colsing tag.
* for the node's content (or children) and render_end() for the closing tag.
*
* @package Midgard.CreatePHP
*/
Expand Down Expand Up @@ -245,12 +245,13 @@ public function renderStart($tag_name = false)
$this->_tag_name = $tag_name;
}

$attributesToSkip = array();

if ( $this->_parent
&& $this->_parent->isRendering())
{
//remove about to work around a VIE bug with nested identical about attributes
$about_backup = $this->getAttribute('about');
$this->unsetAttribute('about');
$attributesToSkip[] = 'about';
}

$template = explode('__CONTENT__', $this->_template);
Expand All @@ -259,14 +260,9 @@ public function renderStart($tag_name = false)
$replace = array
(
"__TAG_NAME__" => $this->_tag_name,
" __ATTRIBUTES__" => $this->renderAttributes(),
" __ATTRIBUTES__" => $this->renderAttributes($attributesToSkip),
);

if (!empty($about_backup))
{
$this->setAttribute('about', $about_backup);
}

$this->_is_rendering = true;
return strtr($template, $replace);
}
Expand All @@ -291,11 +287,15 @@ public function render($tag_name = false)
*
* @api
*/
public function renderAttributes()
public function renderAttributes($attributesToSkip = array())
{
// add additional attributes
$attributes = '';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you should array_swap and then check with isset instead of in_array. in_array needs to loop through the whole array for each lookup - once swapped the values are the keys and isset is a fast hash lookup.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In $attributesToRemove, we have only the name of the attributes to skip. So in this case, the array is either empty or having one single element: 'about'. So $attributesToRemove is not an associative array. The key and values here are for $this->_attributes. So I guess it doesn't make senses to change it the way you propose? Or did I miss something?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well as its an array other uses could skip many attributes. if you swap this array you will have array('about' => 0) which makes the lookup the same speed regardless of the array length. though swapping the array also costs something.

maybe we could say the array must already be passed with the keys saying what to skip. it does not make more effort in calling and would definitely be faster in all cases than in_array.

$swappedAttributes = array_flip($attributesToSkip);
foreach ($this->_attributes as $key => $value) {
if (isset($swappedAttributes[$key])) {
continue;
}
$attributes .= ' ' . $key . '="' . $value . '"';
}
if ($attributes === ' ') {
Expand Down Expand Up @@ -329,6 +329,9 @@ public function renderEnd()
*/
public function __toString()
{
if ($this->isRendering()) {
return $this->renderContent();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do not render start and end again if we are already rendering. this is helpful in twig to say

{% createphp model as rdf %} {{ rdf|raw }} {{% endcreatephp %}}

}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it wouldn't be better to move this logic to the render() method, because right now the last two lines would produce different outputs:

echo $entity->renderStart();

echo $entity; //only renders content
echo $entity->render(); //renders enclosing tags as well.

You could of course say that render() is the "I know what I'm doing" option, but then this should be clearly stated in the phpdoc. But intuitively, I would say that the __toString method should be only an alias to render() so that they always behave in the same way. Of course, I might still be convinced otherwise :-)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you are right. and i see no good usecase where i would want to renderStart and before renderEnd call render and get the enclosing tag again.

return $this->render();
}
}
4 changes: 3 additions & 1 deletion src/Midgard/CreatePHP/NodeInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,9 +142,11 @@ public function renderEnd();
* Render just the attributes. This is not needed if you use
* renderStart()
*
* @param array $attributesToSkip attributes names that will not be printed
*
* @return string the rendered attributes
*/
public function renderAttributes();
public function renderAttributes($attributesToSkip = array());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you feel about using a typehint here? i.e.

public function renderAttributes(array $attributesToSkip = array());

just to make it a bit more robust

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

willdo


/**
* Has to return the same as self::render()
Expand Down
25 changes: 25 additions & 0 deletions tests/Test/Midgard/CreatePHP/Collection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<?php

namespace Test\Midgard\CreatePHP;

class Collection
{
public function getTitle()
{
return 'the collection title';
}

public function getSubject()
{
return '/the/subject/collection';
}

public function getChildren()
{
return array(
new Model('title 1', 'content 1', '/the/subject/model/1'),
new Model('title 2', 'content 2', '/the/subject/model/2')
);
}

}
Loading