magento / magento2

Prior to making any Submission(s), you must sign an Adobe Contributor License Agreement, available here at: https://opensource.adobe.com/cla.html. All Submissions you make to Adobe Inc. and its affiliates, assigns and subsidiaries (collectively “Adobe”) are subject to the terms of the Adobe Contributor License Agreement.
http://www.magento.com
Open Software License 3.0
11.54k stars 9.32k forks source link

Inconsistency between Select and Multiselect form elements. #732

Closed tzyganu closed 8 years ago

tzyganu commented 10 years ago

Even if the differences between a select an multiselect should be only 'multiple="multiple"' and the way that the selected value is determined the classes that render these elements behave differently: I can define a select element like this:

 $fieldset->addField(
        'some_id',
        'select',
        array(
            'name'        => 'some_name',
            'label'       => __('Some Label'),
            'title'       => __('Some Label'),
            'options'     => array(
                   'val1' => 'Label 1',
                   'val2' => 'Label 2',
             )
        )
    );

but the multiselect element must be defined like this:

 $fieldset->addField(
        'some_id',
        'multiselect',
        array(
            'name'        => 'some_name',
            'label'       => __('Some Label'),
            'title'       => __('Some Label'),
            'values'     => array(
                   array(
                        'value' => 'val1',
                        'label' => 'label 1',
                   ),
                   array(
                        'value' => 'val2',
                        'label' => 'label 2',
                   )
             )
        )
    );

Notice that for the select I can pass 'options' (it works with values also) but for multiselect I have to pass 'values'. And the format for the options array is different from the values array.
I think that the Magento\Framework\Data\Form\Element\Multiselect class should extend Magento\Framework\Data\Form\Element\Select. This way it can use the _prepareOptions method and handle the 'options' setting.
I realize that this is not a critical issue, but consistency is good. :smile:

barbazul commented 9 years ago

+1 El 06/11/2014 10:13, "Marius Strajeru" notifications@github.com escribió:

Even if the differences between a select an multiselect should be only 'multiple="multiple"' and the way that the selected value is determined the classes that render these elements behave differently: I can define a select element like this:

$fieldset->addField( 'some_id', 'select', array( 'name' => 'some_name', 'label' => ('Some Label'), 'title' => ('Some Label'), 'options' => array( 'val1' => 'Label 1', 'val2' => 'Label 2', ) ) );

but the multiselect element must be defined like this:

$fieldset->addField( 'some_id', 'multiselect', array( 'name' => 'some_name', 'label' => ('Some Label'), 'title' => ('Some Label'), 'values' => array( array( 'value' => 'val1', 'label' => 'label 1', ), array( 'value' => 'val2', 'label' => 'label 2', ) ) ) );

Notice that for the select I can pass 'options' (it works with values also) but for multiselect I have to pass 'values'. And the format for the options array is different from the values array.

I think that the Magento\Framework\Data\Form\Element\Multiselect class should extend Magento\Framework\Data\Form\Element\Select. This way it can use the _prepareOptions method and handle the 'options' setting.

I realize that this is not a critical issue, but consistency is good. [image: :smile:]

— Reply to this email directly or view it on GitHub https://github.com/magento/magento2/issues/732.

ihor-sviziev commented 9 years ago

+1

vkorotun commented 9 years ago

Thank you for reporting this issue. Having in mind that we are going to use new UI component for all forms and grids (which is now in development), it is quite probable that this and other inconsistencies will be fixed only there, and not in Widgets. Any way, this will be added to the backlog, and hopefully will be fixed also for Form and Grid Widgets as well. And we'll ensure that new UI component won't be sucked into the same swamp.

Btw, so far, we are going to stick with this declaration as more flexible and extensible:

'values'     => array(
    array(
        'value' => 'val1',
        'label' => 'label 1',
    )
vkorotun commented 9 years ago

Linked to internal ticket: MAGETWO-31571.

tzyganu commented 8 years ago

@vkorotun I'm going through my old opened issues trying to solve what I can and close what's not useful anymore. I think this is not going to get solved very soon since it does not bring a big business value and there are already extensions that use this. Changing the way a multiselect field behaves would break backwards compatibility. So I will close this issue. In case a miracle happens and this gets on your TODO list feel free to reopen it.