New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix collection template for not numeric children keys #6280
base: 4.x
Are you sure you want to change the base?
Conversation
@@ -137,7 +137,7 @@ | |||
'data-entry-is-complex': form.vars.ea_vars.field and form.vars.ea_vars.field.customOptions.get('entryIsComplex') ? 'true' : 'false', | |||
'data-allow-add': allow_add ? 'true' : 'false', | |||
'data-allow-delete': allow_delete ? 'true' : 'false', | |||
'data-num-items': form.children is empty ? 0 : max(form.children|keys) + 1, | |||
'data-num-items': form.children is empty ? 0 : form.children|length, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work if the array has numeric keys not starting by 0.
And it's quite common to use the entity id as the index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And it's quite common to use the entity id as the index.
Collection field does not handle indexes at all. Even field named "num-items" (read as count).
Needed only to be "unique" in js.
And yes, it must be numeric, but indexes may not (check #6277)
EasyAdminBundle/assets/js/field-collection.js
Lines 33 to 34 in ca0e6c5
// Use a counter to avoid having the same index more than once | |
let numItems = parseInt(collection.dataset.numItems); |
A fictional problem brought problems
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a look on this thread to see how indexed collection using ids are used:
symfony/symfony#7828 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be clear, if I have 2 items in my collection with index not starting by 0:
1 => item 1,
2 => item 2,
With your solution, form.children|length
will return 2
and the new item added will have the same key 2
as the second item which is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know what you mean, but it is weird idea doing so here.
And yes, I know that this fix may bring collisions, but don't know how properly resolve it.
Probably need some refactoring - there is already many crutches
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe filtering only numeric key?
max(form.children|keys|filter(k => k matches '/^\\d+$/')) + 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another crutch :)
With string keys all new items will be 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably it resolve problem in moment, as after save it become normal keys and "unique" will be satisfied.
But I don't envy those who will support it later )
Fix #6277