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

Conversation

heiglandreas
Copy link
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 5 commits January 14, 2014 12:28
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 aims to a better CodeCoverage
@froschdesign
Copy link
Member

Looks good to me. 👍

@Ocramius
Copy link
Member

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

* 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>
Copy link
Member

Choose a reason for hiding this comment

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

This don't match default value.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@weierophinney weierophinney added this to the 2.3.0 milestone Mar 3, 2014
@weierophinney weierophinney self-assigned this Mar 3, 2014
@weierophinney
Copy link
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
Copy link
Member

I tried changing the $wrapper to read '<fieldset%s>%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 added a commit that referenced this pull request Mar 4, 2014
…igurable

Feature/make collection configurable

Conflicts:
	library/Zend/Form/View/Helper/FormCollection.php
	tests/ZendTest/Form/View/Helper/FormCollectionTest.php
weierophinney added a commit that referenced this pull request Mar 4, 2014
@weierophinney
Copy link
Member

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

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants