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

Feature/make collection configurable #5719

Closed
wants to merge 6 commits into from
Closed

Feature/make collection configurable #5719

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0f47f5e
Adds more config to FormCollection-ViewHelper
Nov 29, 2013
beddb3e
Adds a missing return value
Nov 29, 2013
86a2cf9
Changes return value in DocBlock to self
Nov 29, 2013
c3728a2
REmoves getter in favour of property
Nov 29, 2013
2fc73fd
Adds more tests to the FormCollection ViewHelper
Nov 29, 2013
b9f972b
Fixes a typo in the comment
Feb 21, 2014
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
140 changes: 127 additions & 13 deletions library/Zend/Form/View/Helper/FormCollection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,27 @@ class FormCollection extends AbstractHelper
*/
protected $shouldWrap = true;

/**
* This is the default wrapper that the collection is wrapped into
*
* @var string
*/
protected $wrapper = '<fieldset>%2$s%1$s%3$s</fieldset>';
Copy link
Member

Choose a reason for hiding this comment

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

Why 2 before 1 and not make it more predictable with 1 before 2 (and change sprintf params)

Copy link
Member Author

Choose a reason for hiding this comment

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

I've ordered the parameters by importance.

As the content of the fieldset seemed to me the most important one. A legend, which possibly might not be existent is placed before the content in HTML-output, but I couldn't see it as important as the content, so I placed it as second. And the third parameter would then be the template-span-tag that might also be empty.


/**
* This is the default label-wrapper
*
* @var string
*/
protected $labelWrapper = '<legend>%s</legend>';

/**
* Where shall the template-data be inserted into
*
* @var string
*/
protected $templateWrapper = '<span data-template="%s"></span>';

/**
* The name of the default view helper that is used to render sub elements.
*
Expand Down Expand Up @@ -98,11 +119,6 @@ public function render(ElementInterface $element)
}
}

// If $templateMarkup is not empty, use it for simplify adding new element in JavaScript
if (!empty($templateMarkup)) {
$markup .= $templateMarkup;
}

// Every collection is wrapped by a fieldset if needed
if ($this->shouldWrap) {
$label = $element->getLabel();
Expand All @@ -116,14 +132,19 @@ public function render(ElementInterface $element)
);
}

$label = $escapeHtmlHelper($label);

$markup = sprintf(
'<fieldset><legend>%s</legend>%s</fieldset>',
$label,
$markup
$label = sprintf(
$this->labelWrapper,
$escapeHtmlHelper($label)
);
}
$markup = sprintf(
Copy link
Member

Choose a reason for hiding this comment

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

@Ocramius This is a BC/Break. Before at this poing only markup.templatemarkup was returned. Now <fieldset> is returned too.

Copy link
Member Author

Choose a reason for hiding this comment

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

The point is, you couldn't get a fieldset without a label/legend before that. Now you always get a fieldset - either with or without legend depending on whether a label is set for the fieldset. So it's more predictable what you get. And when you set Zend\Form\View\Helper\FormCollection::$shouldWrap = true I'd expect it to wrap my content up into something and not only because I've also set a label for the Collection.

Agreed, If you see it as a BC-Break, I could add another property containing a default that wraps everything up into nothing (i.e. '%s') which is used when only $shouldWrap is set but no label is given. This property can then be set by the user to '<fieldset>%2$s%1$s%3$s</fieldset' to get the expected behaviour.

The further question is, whether it's a BC-Break or a fix of an unintended behaviour. As no tests are failing and no existing tests have been changed I can't say whether it was intended behaviour in the first place or not. For my use-case it was unintended behaviour so I tried to fix it best as I could.

$this->wrapper,
$markup,
$label,
$templateMarkup
);
} else {
$markup .= $templateMarkup;
}

return $markup;
Expand All @@ -139,18 +160,20 @@ public function renderTemplate(CollectionElement $collection)
{
$elementHelper = $this->getElementHelper();
$escapeHtmlAttribHelper = $this->getEscapeHtmlAttrHelper();
$fieldsetHelper = $this->getFieldsetHelper();

$templateMarkup = '';

$elementOrFieldset = $collection->getTemplateElement();

if ($elementOrFieldset instanceof FieldsetInterface) {
$templateMarkup .= $this->render($elementOrFieldset);
$templateMarkup .= $fieldsetHelper($elementOrFieldset);
} elseif ($elementOrFieldset instanceof ElementInterface) {
$templateMarkup .= $elementHelper($elementOrFieldset);
}

return sprintf(
'<span data-template="%s"></span>',
$this->templateWrapper,
$escapeHtmlAttribHelper($templateMarkup)
);
}
Expand Down Expand Up @@ -260,4 +283,95 @@ protected function getFieldsetHelper()

return $this;
}

/**
* Get the wrapper for the collection
*
* @return string
*/
public function getWrapper()
{
return $this->wrapper;
}

/**
* Set the wrapper for this collection
*
* The string given will be passed through sprintf with the following three
* replacements:
*
* 1. The content of the collection
* 2. The label of the collection. If no label is given this will be an empty
* string
* 3. The template span-tag. This might also be an empty string
*
* The preset default is <pre><fieldset>%2$s%1$s%3$s</fieldset></pre>
*
* @param string $wrapper
*
* @return self
*/
public function setWrapper($wrapper)
{
$this->wrapper = $wrapper;

return $this;
}

/**
* Set the label-wrapper
* The string will be passed through sprintf with the label as single
* parameter
* This defaults to '<legend>%s</legend>'
*
* @param string $labelWrapper
*
* @return self
*/
public function setLabelWrapper($labelWrapper)
{
$this->labelWrapper = $labelWrapper;

return $this;
}

/**
* Get the wrapper for the label
*
* @return string
*/
public function getLabelWrapper()
{
return $this->labelWrapper;
}

/**
* Ge the wrapper for the template
*
* @return string
*/
public function getTemplateWrapper()
{
return $this->templateWrapper;
}

/**
* Set the string where the template will be inserted into
*
* This string will be passed through sprintf and has the template as single
* parameter
*
* THis defaults to '<span data-template="%s"></span>'
*
* @param string $templateWrapper
*
* @return self
*/
public function setTemplateWrapper($templateWrapper)
{
$this->templateWrapper = $templateWrapper;

return $this;
}

}
127 changes: 127 additions & 0 deletions tests/ZendTest/Form/View/Helper/FormCollectionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,4 +176,131 @@ public function testCanTranslateLegend()

$this->assertContains('>translated legend<', $markup);
}

public function testCollectionIsWrappedByFieldsetWithoutLegend()
{
$form = $this->getForm();
$collection = $form->get('colors');
$this->helper->setShouldWrap(true);

$markup = $this->helper->render($collection);

$this->assertNotContains('<legend>', $markup);
$this->assertStringStartsWith('<fieldset>', $markup);
$this->assertStringEndsWith('</fieldset>', $markup);
}

public function testCollectionIsWrappedByFieldsetWithLabel()
{
$form = $this->getForm();
$collection = $form->get('colors');
$collection->setLabel('foo');
$this->helper->setShouldWrap(true);

$markup = $this->helper->render($collection);

$this->assertContains('<legend>foo</legend>', $markup);
$this->assertStringStartsWith('<fieldset>', $markup);
$this->assertStringEndsWith('</fieldset>', $markup);
}

public function testCollectionIsWrappedByCustomElement()
{
$form = $this->getForm();
$collection = $form->get('colors');
$this->helper->setShouldWrap(true);
$this->helper->setWrapper('<div>%2$s%1$s%3$s</div>');

$markup = $this->helper->render($collection);

$this->assertNotContains('<legend>', $markup);
$this->assertStringStartsWith('<div>', $markup);
$this->assertStringEndsWith('</div>', $markup);

}

public function testCollectionContainsTemplateAtPos3()
{
$form = $this->getForm();
$collection = $form->get('colors');
$collection->setShouldCreateTemplate(true);
$this->helper->setShouldWrap(true);
$this->helper->setWrapper('<div>%3$s%2$s%1$s</div>');

$markup = $this->helper->render($collection);

$this->assertNotContains('<legend>', $markup);
$this->assertStringStartsWith('<div><span', $markup);
$this->assertStringEndsWith('</div>', $markup);
}

public function testCollectionRendersLabelCorrectly()
{
$form = $this->getForm();
$collection = $form->get('colors');
$collection->setLabel('foo');
$this->helper->setShouldWrap(true);
$this->helper->setLabelWrapper('<h1>%s</h1>');

$markup = $this->helper->render($collection);

$this->assertContains('<h1>foo</h1>', $markup);
$this->assertStringStartsWith('<fieldset><h1>foo</h1>', $markup);
}

public function testCollectionCollectionRendersTemplateCorrectly()
{
$form = $this->getForm();
$collection = $form->get('colors');
$collection->setShouldCreateTemplate(true);
$this->helper->setTemplateWrapper('<div class="foo">%s</div>');

$markup = $this->helper->render($collection);

$this->assertNotContains('<legend>', $markup);
$this->assertRegExp('/\<div class="foo">.*?<\/div>/', $markup);

}

public function testCollectionRendersTemplateWithoutWrapper()
{
$form = $this->getForm();
$collection = $form->get('colors');
$collection->setShouldCreateTemplate(true);
$this->helper->setShouldWrap(false);
$this->helper->setTemplateWrapper('<div class="foo">%s</div>');

$markup = $this->helper->render($collection);

$this->assertNotContains('<fieldset>', $markup);
$this->assertRegExp('/\<div class="foo">.*?<\/div>/', $markup);
}

public function testCollectionRendersFieldsetCorrectly()
{
$form = $this->getForm();
$collection = $form->get('fieldsets');
$collection->setShouldCreateTemplate(true);
$this->helper->setShouldWrap(true);
$this->helper->setWrapper('<div>%2$s%1$s%3$s</div>');
$this->helper->setTemplateWrapper('<div class="foo">%s</div>');

$markup = $this->helper->render($collection);

$this->assertNotContains('<fieldset>', $markup);
$this->assertRegExp('/\<div class="foo">.*?<\/div>/', $markup);
}

public function testGetterAndSetter()
{
$this->assertSame($this->helper, $this->helper->setWrapper('foo'));
$this->assertAttributeEquals('foo', 'wrapper', $this->helper);
$this->assertEquals('foo', $this->helper->getWrapper());
$this->assertSame($this->helper, $this->helper->setLabelWrapper('foo'));
$this->assertAttributeEquals('foo', 'labelWrapper', $this->helper);
$this->assertEquals('foo', $this->helper->getLabelWrapper());
$this->assertSame($this->helper, $this->helper->setTemplateWrapper('foo'));
$this->assertAttributeEquals('foo', 'templateWrapper', $this->helper);
$this->assertEquals('foo', $this->helper->getTemplateWrapper());
}
}