Feature/make collection configurable #5719

Closed
wants to merge 6 commits into
from

4 participants

@heiglandreas
Zend Framework member

This commit adds more configuration options to the
FormCollection-ViewHelper.

The hard-coded fieldset-wrapper is now configurable as is the hard-coded
legend as well as the hard-coded position and template of the
template-provider. The defaults have been set to the currently
hard-coded values, so that no BC-break should happen.

Also the behaviour of the template-renderer has been changed in so far
as the template is now rendered using the collection-object and not the
content of the collection-object

THis might be a case of BC-Break in edge-cases but the current
default-behaviour as provided by the unit-tests is not broken.

No tests have been changed, there have only been additions

This PR is a follow-up to #5565

Andreas Heigl added some commits Nov 29, 2013
Andreas Heigl Adds more config to FormCollection-ViewHelper
This commit adds more configuration options to the
FormCollection-ViewHelper.

The hard-coded fieldset-wrapper is now configurable as is the hard-coded
legend as well as the hard-coded position and template of the
template-provider. The defaults have been set to the currently
hard-coded values, so that no BC-break should happen.

Also the behaviour of the template-renderer has been changed in so far
as the template is now rendered using the collection-object and not the
content of the collection-object

THis might be a case of BC-Break in edge-cases but the current
default-behaviour as provided by the unit-tests is not broken.

No tests have been changed, there have only been additions
0f47f5e
Andreas Heigl Adds a missing return value beddb3e
Andreas Heigl Changes return value in DocBlock to self 86a2cf9
Andreas Heigl REmoves getter in favour of property
see @weierophinney comment on #4436
c3728a2
Andreas Heigl Adds more tests to the FormCollection ViewHelper
THis aims to a better CodeCoverage
2fc73fd
@froschdesign
Zend Framework member

Looks good to me. 👍

@Ocramius
Zend Framework member

@Maks3w doesn't seem like there's any BC break here - can be thrown at develop IMO :D

@Maks3w Maks3w and 1 other commented on an outdated diff Feb 20, 2014
library/Zend/Form/View/Helper/FormCollection.php
+ {
+ 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%2$s%3$s</fieldset></pre>
@Maks3w
Zend Framework member
Maks3w added a line comment Feb 20, 2014

This don't match default value.

@heiglandreas
Zend Framework member
heiglandreas added a line comment Feb 21, 2014

Thanks for spotting this typo. I'll just change that one to <fieldset>%2$s%1$s%3$s</fieldset>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Maks3w Maks3w commented on the diff Feb 20, 2014
library/Zend/Form/View/Helper/FormCollection.php
@@ -26,6 +26,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>';
@Maks3w
Zend Framework member
Maks3w added a line comment Feb 20, 2014

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

@heiglandreas
Zend Framework member
heiglandreas added a line comment Feb 21, 2014

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Maks3w Maks3w commented on the diff Feb 20, 2014
library/Zend/Form/View/Helper/FormCollection.php
);
}
+ $markup = sprintf(
@Maks3w
Zend Framework member
Maks3w added a line comment Feb 20, 2014

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

@heiglandreas
Zend Framework member
heiglandreas added a line comment Feb 21, 2014

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney added this to the 2.3.0 milestone Mar 3, 2014
@weierophinney weierophinney self-assigned this Mar 3, 2014
@weierophinney
Zend Framework member

@heiglandreas #5623 provides some logic to ensure that

  1. legends will only be rendered if the fieldset has a label defined
  2. fieldsets will render attributes as set on the collection

On merging your PR to the develop branch, I had a merge conflict due to those changes. I tried changing the $wrapper to read '<fieldset%s>%2$s%1$s%3$s</fieldset>', and altering the render() logic to inject the attributes, if any. However, this results in practically all of your new tests failing.

Any chance you can take a look at it?

@weierophinney
Zend Framework member

I tried changing the $wrapper to read '%2$s%1$s%3$s',

This was wrong, once I paid more attention to the sprintf() invocation. I altered it as follows:

    '<fieldset%1$s>%3$s%2$s%4$s</fieldset>'

And the sprintf() invocation reads:

$markup = sprintf(
    $this->wrapper,
    $attributeString,
    $markup,
    $legend,
    $templateMarkup
);

This gets me closer - now only 2 failing tests. Will let you know if I get any closer.

@weierophinney weierophinney added a commit that referenced this pull request Mar 4, 2014
@weierophinney weierophinney Merge branch 'feature/5719' into develop
Close #5719
d9e3a2b
@weierophinney
Zend Framework member

@heiglandreas Got it! Merged to develop for release with 2.3.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment