themeum / kirki

Extending the customizer
https://kirki.org
MIT License
1.26k stars 328 forks source link

Modifying new fields of repeater field in existing rows doesn't work #1540

Open ArekZw opened 7 years ago

ArekZw commented 7 years ago

Issue description:

  1. In Kirki: Create repeater with one field
  2. In customizer: create one row using repeater
  3. In Kirki: Add second field to repeater
  4. In customizer: modifying second field in the existing row doesn't work (after save and reloading the value disappears). Value is not saved in DB.

But if I create second row, second field in second row works.

Version used:

3.0.9 and develop branch

Using theme_mods or options?

options

Code to reproduce the issue (config + field(s))

Step 1:

    Kirki::add_panel('modules_global', array(
        'priority' => 1000,
        'title' => __('Modules - global', 'textdomain') ,
        'description' => __('', 'textdomain') ,
    ));
    Kirki::add_config('theme_is_styles_global', array(
        'option_type' => 'option',
        'option_name' => 'is_global_modules_styles',
    ));
    Kirki::add_section('modules_section_global', array(
        'title' => __('Test - Global') ,
        'description' => __('') ,
        'panel' => 'modules_global'
    ));
    Kirki::add_field('theme_is_styles_global', array(
        'type' => 'repeater',
        'label' => esc_attr__(' ', 'my_textdomain') ,
        'section' => 'modules_section_global',
        'settings' => 'isb_modules_section_global',
        'option_type' => 'option',
        'fields' => array(
            'id' => array(
                'type' => 'text',
                'label' => esc_attr__('ID', 'my_textdomain') ,
                'default' => 'gl_',
                'description' => 'Pamiętaj o profiksie gl_',
            ) ,
        );
        ,
    ));

Step 3:

    Kirki::add_panel('modules_global', array(
        'priority' => 1000,
        'title' => __('Modules - global', 'textdomain') ,
        'description' => __('', 'textdomain') ,
    ));
    Kirki::add_config('theme_is_styles_global', array(
        'option_type' => 'option',
        'option_name' => 'is_global_modules_styles',
    ));
    Kirki::add_section('modules_section_global', array(
        'title' => __('Test - Global') ,
        'description' => __('') ,
        'panel' => 'modules_global'
    ));
    Kirki::add_field('theme_is_styles_global', array(
        'type' => 'repeater',
        'label' => esc_attr__(' ', 'my_textdomain') ,
        'section' => 'modules_section_global',
        'settings' => 'isb_modules_section_global',
        'option_type' => 'option',
        'fields' => array(
            'id' => array(
                'type' => 'text',
                'label' => esc_attr__('ID', 'my_textdomain') ,
                'default' => 'gl_',
                'description' => '',
            ) ,
            'second_field' => array(
                'type' => 'text',
                'label' => esc_attr__('Here is problem', 'my_textdomain') ,
                'default' => '',
                'description' => '',
            ) ,
        );
    ));
mapsteps commented 6 years ago

Confirmed. Just came across the same issue.

aristath commented 6 years ago

Yes, this is a known issue. Unfortunately this will have to wait for the 3.1 refactor where repeaters will be rebuilt from scratch

BenceSzalai commented 5 years ago

I've faced the same issue. It looks like it is because of the check from row 2938 in script.js:

if ( _.isUndefined( currentSettings[ row.rowIndex ][ fieldId ] ) ) {
            return;
}

This check aborts the updateField method (row 2916) if the field being updated has no previous value.

I don't know why this check is necessary. Is there any situations, when we should expect someone trying to add a setting that is not allowed? Shouldn't even in that case sanitization happen on the server side?

I've simply commented out that JS check as a workaround, but please let me know if you know any specific cases when that check is important! If not, I think a quick fix could be easily done by removing that check even before the 3.1 refactor release mentioned above.

mapsteps commented 5 years ago

Is this a possible solution @aristath?

Thanks @Grapestain!

jeradshepherd commented 4 years ago

Has anyone figured out how to fix this yet, by chance? I see the last comment here was almost 2 years ago and I am still having this issue. Commenting out the code snippet that @BenceSzalai posted above did not work for me.

Any help would be greatly appreciated!

BenceSzalai commented 4 years ago

Probably, because you client side uses the minified JS. Look for script.min.js and try to make the same change on that too.

BenceSzalai commented 4 years ago

Looks like it has moved since here: https://github.com/kirki-framework/kirki/blob/develop/packages/kirki-framework/control-repeater/src/assets/scripts/control.js#L736