Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Improvements Zend\Form\View\Helper\FormElement #5252

Merged
merged 8 commits into from Oct 21, 2013

Conversation

Projects
None yet
6 participants
Contributor

snapshotpl commented Oct 9, 2013

Remove duplicate code, add easy way to add new elements

@samsonasik samsonasik commented on an outdated diff Oct 9, 2013

library/Zend/Form/View/Helper/FormElement.php
- if ('week' == $type) {
- $helper = $renderer->plugin('form_week');
- return $helper($element);
- }
+ /**
+ * Add instance map to plugin
+ *
+ * @param string $instance
+ * @param string $plugin
+ * @return FormElement

@samsonasik samsonasik commented on an outdated diff Oct 9, 2013

library/Zend/Form/View/Helper/FormElement.php
- if ('time' == $type) {
- $helper = $renderer->plugin('form_time');
- return $helper($element);
- }
+ /**
+ * Add type map to plugin
+ *
+ * @param string $type
+ * @param string $plugin
+ * @return FormElement
Contributor

bakura10 commented Oct 10, 2013

I like this!

Contributor

stefanotorresi commented Oct 10, 2013

hmm what about keeping plugin naming consistent with Zend\Form\View\HelperConfig ? maybe even getting the keys right from that class with a getter.

Contributor

bakura10 commented Oct 10, 2013

@stefanotorresi you're right. You should reuse the ekys fro HelperConfig :).

@bakura10 bakura10 commented on an outdated diff Oct 10, 2013

library/Zend/Form/View/Helper/FormElement.php
use Zend\Form\ElementInterface;
use Zend\View\Helper\AbstractHelper as BaseAbstractHelper;
class FormElement extends BaseAbstractHelper
{
+ const DEFAULT_HELPER = 'form_input';
+
+ /**
+ * Instance map to view helper
+ *
+ * @var array
+ */
+ protected $instanceMap = array(
+ 'Zend\Form\Element\Button' => 'form_button',
+ 'Zend\Form\Element\Captcha' => 'form_captcha',
@bakura10

bakura10 Oct 10, 2013

Contributor

Alignmenet with the "=" please.

@bakura10 bakura10 commented on an outdated diff Oct 10, 2013

library/Zend/Form/View/Helper/FormElement.php
+ 'Zend\Form\Element\DateTimeSelect' => 'form_date_time_select',
+ 'Zend\Form\Element\DateSelect' => 'form_date_select',
+ 'Zend\Form\Element\MonthSelect' => 'form_month_select',
+ );
+
+ /**
+ * Type map to view helper
+ *
+ * @var array
+ */
+ protected $typeMap = array(
+ 'checkbox' => 'form_checkbox',
+ 'color' => 'form_color',
+ 'date' => 'form_date',
+ 'datetime' => 'form_date_time',
+ 'datetime-local' => 'form_date_time_local',
@bakura10

bakura10 Oct 10, 2013

Contributor

Sma,e please align the "=" :).

@bakura10 bakura10 and 2 others commented on an outdated diff Oct 10, 2013

library/Zend/Form/View/Helper/FormElement.php
- if ('time' == $type) {
- $helper = $renderer->plugin('form_time');
- return $helper($element);
+ /**
+ * Render element by instance map
+ *
+ * @param ElementInterface $element
+ * @return string|null
+ */
+ protected function renderInstance(ElementInterface $element)
+ {
+ foreach ($this->instanceMap as $class => $pluginName) {
@bakura10

bakura10 Oct 10, 2013

Contributor

I think you can do something more efficient:

$class = get_class($element);

if (isset($this->instanceMap[$class]) {
   // ... do something
}
@snapshotpl

snapshotpl Oct 10, 2013

Contributor

Ok, but what if you extends some form elements? Class name will be different.

@bakura10

bakura10 Oct 10, 2013

Contributor

True also. So keep your solution then :)

@stefanotorresi

stefanotorresi Oct 10, 2013

Contributor

if you use an extended element, you'll probably want to use the addInstance() method you added to make the helper aware of the releation between the element and the helper.

Speaking of that, I'd also rename addInstance() to something more self-explaining. The word 'instance' may be misleading, since your not actually passing any instance, but rather an element class name and a plugin alias. ;)

@snapshotpl

snapshotpl Oct 10, 2013

Contributor

But by default you can use view helper from parent.

Rename 👍

Contributor

snapshotpl commented Oct 10, 2013

@stefanotorresi @bakura10 I'm not sure how to reuse HelperConfig it this case

Contributor

stefanotorresi commented Oct 10, 2013

@snapshotpl i think the cleanest way would be to use a factory to create the FormElement helper.

you can add the factory as a callable in the HelperConfig class itself, so you can immediately access to the array you need, and change the configureServiceManager() method accordingly, to make sure it also adds the factory.

what do you think @bakura10 ?

Contributor

bakura10 commented Oct 10, 2013

Lets just keep it easy. Let's just use the same keys that what we use. (So for instance, "form_date_time_select" becomes "formdatetimeselect"). Anyway, this does not matter a lot as those are normalized by the SM.

Contributor

snapshotpl commented Oct 10, 2013

I think you don't understand what FormElement does. It's map between form elements and form view helpers. In zf1 view helper is define in form elements. In zf2 Zend\Form\View\Helper\FormElement gets responsibility for that. I don't know how HelperConfig can help with that.

Contributor

stefanotorresi commented Oct 10, 2013

I understand what it does, I probably didn't explain myself ;)

HelperConfig is used by ViewHelperManagerFactory to add each of default form view helpers to ViewHelperManager. By default they're all simple invokables services.

I was suggesting a more complex solution that involved a peculiar configuration for the FormElement view helper service. The aim was basically to map HelperConfig view helper aliases to FormElementManager form element aliases. That would need access to both, which can be achieved in a service factory, which would probably take place in HelperConfig. That would avoid repeating the same array keys and values all over the place.

Anyway it doesn't matter much since @bakura10 suggested to keep the current simpler approach, which to be honest I don't like, but I trust his wisdom nevertheless. ;)

Contributor

bakura10 commented Oct 10, 2013

@stefanotorresi. I just want him to rename to elements in the typemap array to be written the way they are written in HelperConfig for coherency. Once again does not change anything as everything is normalized at the end.

Contributor

stefanotorresi commented Oct 10, 2013

Yep, got it.

@samsonasik samsonasik commented on an outdated diff Oct 10, 2013

library/Zend/Form/View/Helper/FormElement.php
@@ -1,4 +1,5 @@
<?php
+
/**
@samsonasik

samsonasik Oct 10, 2013

Contributor

no new line after <?php and the docblocks for consistency with other files

@Maks3w Maks3w commented on an outdated diff Oct 21, 2013

library/Zend/Form/View/Helper/FormElement.php
- if ('week' == $type) {
- $helper = $renderer->plugin('form_week');
- return $helper($element);
+ foreach ($this->typeMap as $typeName => $pluginName) {
@Maks3w

Maks3w Oct 21, 2013

Member

Rather try to access directky to the array index isset($array[$key]]

@Maks3w Maks3w commented on an outdated diff Oct 21, 2013

library/Zend/Form/View/Helper/FormElement.php
- if ('time' == $type) {
- $helper = $renderer->plugin('form_time');
- return $helper($element);
+ /**
+ * Render element by instance map
+ *
+ * @param ElementInterface $element
+ * @return string|null
+ */
+ protected function renderInstance(ElementInterface $element)
+ {
+ foreach ($this->classMap as $class => $pluginName) {
+ if (is_a($element, $class)) {
@Maks3w

Maks3w Oct 21, 2013

Member

I'm worried about the performance of this. Could you show some benchmark?

@Maks3w Maks3w commented on the diff Oct 21, 2013

library/Zend/Form/View/HelperConfig.php
@@ -61,9 +61,7 @@ class HelperConfig implements ConfigInterface
'formradio' => 'Zend\Form\View\Helper\FormRadio',
'formrange' => 'Zend\Form\View\Helper\FormRange',
'formreset' => 'Zend\Form\View\Helper\FormReset',
- 'form_reset' => 'Zend\Form\View\Helper\FormReset',
@Maks3w

Maks3w Oct 21, 2013

Member

Why did you choose to remove this?

@snapshotpl

snapshotpl Oct 21, 2013

Contributor

Becouse names in ServiceManager does format by Zend\ServiceManager\ServiceManager::canonicalizeName function. So formreset === form_reset

@ghost ghost assigned Maks3w Oct 21, 2013

Maks3w added a commit that referenced this pull request Oct 21, 2013

@Maks3w Maks3w merged commit aebe19f into zendframework:develop Oct 21, 2013

1 check passed

default The Travis CI build passed
Details
Contributor

psfpro commented Nov 5, 2013

I updated the Zend Framework 2.2.5, but I don't see this pull request in release.
This pull request in changelog:
https://github.com/zendframework/zf2/releases/tag/release-2.2.5
but if download tag, this has no changes

Contributor

stefanotorresi commented Nov 5, 2013

this has been merged into develop branch, so you'll have to wait for 2.3.0 release.

Contributor

psfpro commented Nov 5, 2013

Very bad, it is very useful for me. I have a lot of additional form elements.

Contributor

samsonasik commented Nov 5, 2013

@psfpro I think you can just use :

"require" : {
     "zendframework/zendframework" : "dev-develop"
}

if you need it for now.

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