Yapapaya / jquery-cloneya

A jquery plugin to clone DOM elements
MIT License
76 stars 28 forks source link

serializeIndex does not properly update nested array field names #42

Closed acoulton closed 8 years ago

acoulton commented 9 years ago

When cloning an element like this with serializeIndex: true:

<div class="toclone">
    <input name="first[0][second][0][field]">
    <input name="first[0][second][1][field]">
</div>

I would expect only the first-level array key to be updated to the new sequence - eg:

<div class="toclone">
    <input name="first[0][second][0][field]">
    <input name="first[0][second][1][field]">
</div>
<div class="toclone">
    <input name="first[1][second][0][field]">
    <input name="first[1][second][1][field]">
</div>

However, cloneya resets all the numeric keys in a field name in both the toClone and newClone elements - so the output is actually:

<div class="toclone">
    <input name="first[0][second][0][field]">
    <input name="first[0][second][0][field]"> <!-- note the second-level key changed from 1 to 0 -->
</div>
<div class="toclone">
    <input name="first[1][second][1][field]">
    <input name="first[1][second][1][field]">
</div>

This then produces duplicate field names in each block, so the submitted data is missing expected values.

Also reproduced in this jsFiddle.

I appreciate this probably isn't as simple as just forcing it to only change the first key, since potentially in a nested set of clones you might actually want to end up with fields like:

<input name="person[0][telephone][0]">
<input name="person[0][telephone][1]">
<input name="person[1][telephone][0]">
<input name="person[1][telephone][1]">
<input name="person[1][telephone][2]">
<!-- etc -->

where both the "person" block and the individual "telephone" rows could be cloned - so you'd have to track the depth of the clone and work out which index to change each time.

However, as-is the behaviour is very unexpected and doesn't really work for any usecase - so we should either try and fix it or at least strongly document that multi-level array field names are not supported.

acoulton commented 9 years ago

One possible solution for all usecases might be to define a nameIndexPattern or similar option to define the regular expression that will be used to find indexes to replace (in place of the hardcoded /\[([^}]+)\]/)[https://github.com/yapapaya/jquery-cloneya/blob/stable/dist/jquery-cloneya.js#L418].

We could then do things like:

$('.clone-wrapper').cloneya({serializeIndex: true, nameIndexPattern: /^first\[(\d+)\]/});
$('.person-blocks).cloneya({serializeIndex: true, nameIndexPattern: /^person\[(\d+)\]/});
$('.nested-telephone-rows).cloneya({serializeIndex: true, nameIndexPattern: /^person\[\d+\]\[telephone\]\[(\d+)\]/});

or basically whatever pattern was appropriate for the usecase. Cloneya would continue to replace the content of any numeric capture group found by the regex.

The default could either be the current pattern, or a slightly safer pattern that only matches the first numeric key in a name.

If you think something like this would be OK I can work up a PR with tests.

acoulton commented 9 years ago

@yapapaya bump - what do you think? Should I try to fix this with a namePattern option instead of the hardcoded one?

actual-saurabh commented 9 years ago

@acoulton I actually wanted to keep this open to plugging by the dev. So, the idea was to either to not support it at all and let the dev figure it out or handle just the first level.

The ideal thing to do is to set it to false and hook onto _afterappend.cloneya that is triggered just after the _redoIDs function is run to do things specific to one's use case. It'll be impossible to handle all possible use-cases. That is why I haven't documented this one and would possibly remove it altogether!

lagunawebdesign commented 9 years ago

You can add new param 'nthMatch' for options and add function like this one: http://stackoverflow.com/questions/36183/replacing-the-nth-instance-of-a-regex-match-in-javascript#answer-7958627 for param what have to be increment.

if($this.config.nthMatch){
                                    name = replaceNthMatch(name, /(\d+)/, $this.config.nthMatch, function(val){
                                      return i;
                                    });
                                }else{

                                    name = [].map.call(st, function (s, n) {
                                        return (!isNaN(+s) && st[n - 1] === '[' && st[n + 1] === ']') ? i : s;
                                    }).join('');

                                };
solokco commented 8 years ago

Was there any solutions for this?! I'm having the same problem that @acoulton

Any ideas?!

lagunawebdesign commented 8 years ago

I've just wrote own js/jquery script (no plugin) for cakephp+bootstrap structure - to dynamic change field ('name', 'id') and label ('for') - without cloneya.

actual-saurabh commented 8 years ago

See this related issue: https://github.com/yapapaya/jquery-cloneya/issues/53

I still think Indexing is not something that a generic plugin like cloneya can do. Each case is going to be different from the other. I'd rather have developers use the after_clone and before_append events to figure out how to do the indexing for form elements.

actual-saurabh commented 8 years ago

See event documentation: https://github.com/yapapaya/jquery-cloneya/wiki/Events#after_clone

at each clone event, one could easily hook in and manage the indexing on their own.