[WIP] Added option to ensure form element will be rendered inside label tag ev... #5533

Merged
merged 9 commits into from Mar 4, 2014

Projects

None yet

4 participants

@whatthemack
Contributor

...en if element has got an id attribute, fixes #4275

@whatthemack whatthemack Added option to ensure form element will be rendered inside label tag…
… even if element has got an id attribute, fixes ZF-4275
64e44f2
@grizzm0
Contributor
grizzm0 commented Nov 24, 2013

Isn't this more of a custom view helper or the job of formRows()'s setPartial()? I don't see why we should clutter up the framework to support edge cases.

@whatthemack
Contributor

Maybe you're right. Actually this and one other pull request are my first contributions to an open source project and I just wanted to find an entry. Don't get me wrong, this should not be an excuse. I looked up open issues for things that I may be able to fix or to contribute and found issue #4275 where @weierophinney suggested to hack on it as a feature.

@stefanotorresi
Contributor

@grizzm0 honestly I don't think it's an edge case. IMHO it's a trivial variant that FormRow should handle. It's a pita when you have to customize the whole form rendering just for a small detail.

@whatthemack may I suggest to use label_options instead of adding a new element option? Add it as a always_wrap and set it to false by default. See #4677.

@whatthemack
Contributor

@stefanotorresi sure! But I'm facing a simple question: with the current handling of label options it's impossible just to push a new option to existing ones. If I update the default element property as

protected $labelOptions = array(
    'disable_html_escape' => false,
    'always_wrap' => false
);

anyone will be able to set one of those options to false without affecting the other one. Wouldn't it be more intuitive to add methods for label options equally to element options or label attributes, like (add|get|set|add|remove)LabelOption()? Correct me if I'm wrong but I don't see a reason why it's important to know if there are label options defined and it would be way more easy to access them if those methods were existing. Codes like the following could be shortened and clarified as well

if (empty($labelOptions) || $labelOptions['disable_html_escape'] == false) {
    $label = $escapeHtmlHelper($label);
}

// could be changed to
if ($element->getLabelOption('disable_html_escape') == false) {
    $label = $escapeHtmlHelper($label);
}

I'm just worrying that there occur testing notices Undefined index: disable_html_escape when I want to set my new option via

$element->setLabelOptions(array('always_wrap' => true));
@stefanotorresi
Contributor

You're absolutely right, but I didn't want to clutter Element API too much so i'm not so sure about adding more methods. We could just merge the defaults with the new values in setLabelOptions() to ensure the array sanity.

@whatthemack
Contributor

@stefanotorresi this is true, but with this you are not able to unset a default option completely, right? You would have to set an existing key to a known other valid value and basically I think you don't want to have people to know all default label options.
I don't know but wouldn't be adding those methods the logical consequence and make the API of Element more generic? I'm fairly new to Zend\Form and at least I was confused that $labelOptions are treated differently than the $options, $attributes and $labelAttributes properties.
I'd like to contribute this improvement if it's really the right way to handle this, but if there's an official limitation to API-changes like this I'd also implement the merge-solution ;).

@stefanotorresi
Contributor

with this you are not able to unset a default option completely

You could set it to null, but why would you want to do that in the first place? ;)

The other Element properties are not that different: the accessors still operate on arrays and there are no dedicated methods for each of the available keys. The only difference with labelOptions is that to preserve BC we have to supply defaults.

There is no official limitation whatsoever, but since the whole labelOptions idea is meant to handle edge (still common) cases, the changes to Element class have to be as unobtrusive and clean as possible.

Bottom line is that I'd go with

diff --git a/library/Zend/Form/Element.php b/library/Zend/Form/Element.php
index 27ae8a4..2e35763 100644
--- a/library/Zend/Form/Element.php
+++ b/library/Zend/Form/Element.php
@@ -367,13 +367,14 @@ class Element implements

     /**
      * Set label specific options
+     * New values are are merged with sane defaults
      *
      * @param array $labelOptions
      * @return Element|ElementInterface
      */
     public function setLabelOptions(array $labelOptions)
     {
-        $this->labelOptions = $labelOptions;
+        $this->labelOptions = ArrayUtils::merge($this->labelOptions, $labelOptions);
         return $this;
     }
@whatthemack
Contributor

... but why would you want to do that in the first place?

There is no specific reason why the possibility should exist - as long as it's ensured that there's no code that only checks for an existing option key and treats this as default... but as for now there is only the option disable_html_escape as a default and the code...

if (empty($labelOptions) || $labelOptions['disable_html_escape'] == false) {
    $label = $escapeHtmlHelper($label);
}

... treats an empty $labelOptions equally as setting the disable_html_escape to false. So introducing a new option key beside disable_html_escape affects empty($labelOptions). Actually an empty label options set is something different than disable_html_escape = false expresses.
It's impossible right now to test what happens if $labelOptions are not empty but also don't include disable_html_escape. So being able to remove one specific option just enables you to test more precise.

... there are no dedicated methods for each of the available keys

It's not about having separate methods for each key, but to make each key accessible without working an a reference or copy of $labelOptions. I think the access of plain array keys is a bit painfull because you may have to ensure if it really exists and than check against its value.
As long as there's no merging with default attributes the access of $labelOptions['disable_html_escape'] will always trigger notices when overriding the options set, especially when subclassing Zend\Form\Element to introduce custom default properties. Accessing the key/value map like it's implemented for accessing element options or attributes is way more flexible and way more transparent (at least for me, but I think for others too), as you don't have to enforce those arrays to include certain keys to let the application or a part of it work like expected per default: just ask for $element->getLabelOption('disable_html_escape') and go ahead.

Furthermore implementing those accessors won't affect the API of Zend\Element negatively. You will be able to call getLabelOptions() and setLabelOptions() like now and there will be no magic within those methods. Automatically merge a setter argument with a default may be some kind of confusing if you are not familiar with the Element class itself, at least because it's named set* and not merge* or add* ;).

Sorry for writing such a confusing text... I'm not so experienced in explaining my point of view in english :).

@stefanotorresi
Contributor

oh I see your point clearly now. yes, the empty($labelOptions) is a possible fault when dealing with more than one array key, so that definitely has to change!

scrap the merge solution and go ahead, i'm sold with your proposal of the singular getter ;)
could you please also update LabelOptionsAware(Interface|Trait) accordingly.

at that point we can also get rid of the defaults value from the array. I'll take care of providing docs for them.

also mark the PR as wip because we'll probably have to update some view helpers accordingly, so i'll probably submit a PR against your branch once you're done with the Element API ;)

@whatthemack
Contributor

Yay! I'll work on that asap! :)

@whatthemack
Contributor

Sorry, I obviously forgot to squash my committs.
I tried to fix all occurrences of the old accessors in the affected view helpers. Furthermore I decided to implement the setLabelOptions() based on setAttributes() which merges the passed argument with the existing options per default, even though I think it's a bit fuzzy to make merges in setters ;).
The updated tests are running smoothly. I'll hand in some more tests for the new methods on Element later.

Let me know if there's something wrong.

@stefanotorresi
Contributor

Are you sure we really need clear remove and has methods? Apart from that, PR is looking fine, though we need to use the instanceof LabelOptionsAwareInterface check to avoid ducktyping on ElementInterface, which may not implement these new methods.

There are leftovers of that in the view helpers.

@whatthemack
Contributor

Are you sure we really need clear remove and has methods?

It's actually just for the completeness, I think. I'd basically be able to work with options like I'm able to work with attributes.

There are leftovers of that in the view helpers.

As I'm not very familiar with all the view helpers, I'd apologize if you point me in the right direction :). I'll see what I can do then.

@whatthemack
Contributor

[...] we need to use the instanceof LabelOptionsAwareInterface check to avoid ducktyping on ElementInterface
[...] There are leftovers of that in the view helpers.

@stefanotorresi does my last commit (whatthemack@b56a053) contain those fixes you suggested?

@stefanotorresi
Contributor

sorry mate, been a bit busy lately.
at first glance it looks fine, I will check on that ASAP and then pass the ball to the review team ;)

@weierophinney weierophinney added this to the 2.3.0 milestone Mar 3, 2014
@weierophinney weierophinney commented on the diff Mar 4, 2014
library/Zend/Form/LabelOptionsAwareTrait.php
@@ -23,9 +23,7 @@ class LabelOptionsAwareTrait
*
* @var array
*/
- protected $labelOptions = array(
- 'disable_html_escape' => false
- );
+ protected $labelOptions = array();
@weierophinney
weierophinney Mar 4, 2014 Member

The default behavior should be retained; users will overwrite this when desired.

@weierophinney
weierophinney Mar 4, 2014 Member

Never mind; default will always evaluate to false anyways.

@weierophinney weierophinney commented on the diff Mar 4, 2014
tests/ZendTest/Form/ElementTest.php
@@ -161,17 +161,6 @@ public function testSpecificOptionsSetLabelAttributes()
$this->assertEquals(array('bar' => 'baz'), $option);
}
- public function testLabelOptionsAccessors()
- {
- $element = new Element('foo');
- $element->setOptions(array(
- 'label_options' => array('moar' => 'foo')
- ));
-
- $labelOptions = $element->getLabelOptions();
- $this->assertEquals(array('moar' => 'foo'), $labelOptions);
- }
-
@weierophinney
weierophinney Mar 4, 2014 Member

We shouldn't remove tests.

@weierophinney weierophinney self-assigned this Mar 4, 2014
@weierophinney weierophinney added a commit that referenced this pull request Mar 4, 2014
@weierophinney weierophinney Merge branch 'feature/5533' into develop
Close #5533
Fixes #4275
4b9b9a3
@weierophinney weierophinney merged commit b56a053 into zendframework:develop Mar 4, 2014

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment