Skip to content
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

Use the index of the last child of a collection to construct the element's form #13666

Open
wants to merge 3 commits into
base: 1.8
Choose a base branch
from

Conversation

delyriand
Copy link
Contributor

Q A
Branch? 1.8
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Related tickets -
License MIT

When we have a CollectionType with children that have a numerical index, the "add" button can have a strange behaviour when deleting elements and adding new items.
For example, we have an array of items like this:

  • 1 => [...]
  • 3 => [...]
  • 4 => [...]
  • 5 => [...]

Previously, the JS code count the number of items, so 4, and add a new form with inputs name my_beautiful_collection[items][4][XXXX]. And when we submit the form, my "4" item is replace by the new "4"…

Like explained in Symfony doc, I use the last collection item to calculate the index of the new element: https://symfony.com/doc/5.4/form/form_collections.html#allowing-new-tags-with-the-prototype

This patch should be applied on all still maintained branches: 1.9, 1.10, 1.11 and master

@delyriand delyriand requested a review from a team as a code owner February 18, 2022 15:06
@AdamKasp AdamKasp added the Potential Bug Potential bugs or bugfixes, that needs to be reproduced. label Feb 21, 2022
@AdamKasp
Copy link
Contributor

hello, @delyriand I am not able to reproduce it, can you provide more detailed descriptions (maybe some screenshots?)
any tests also will be appreciated :)

@maximehuran
Copy link
Contributor

Hi, we have the case with the https://github.com/monsieurbiz/SyliusRichEditorPlugin/
When we have collection elements, sometimes some data is removed due to the incorrect index.

I have also other issues with this javascript I think I will try to correct it.

Example :

I have a collection in a collection

image

The second element of the second collection is split in the first collection :

image
image

If I correct the form after the first sumbit, it seems to be ok.

@maximehuran
Copy link
Contributor

It seems the post data are not correct :

image

And the second time when it's ok :

image

I search a little and found it's a configuration to add in my form :

            ->add('openings', CollectionType::class, [
                'entry_type' => OpeningType::class,
                'prototype_name' => '__opening__',
                'label' => false,

I added prototype_name in all my collection forms to have __name__ by default this will cause issue while replace.
So there is no issue with the embed collection in Sylius' javascript.

Put this pull request still important to avoid problems with the elements numerotation.

@jacquesbh
Copy link
Member

We've got an issue with this PR when we edit an attribute: An exception has been thrown during the rendering of a template ("Warning: A non-numeric value encountered"). in vendor/sylius/sylius/src/Sylius/Bundle/UiBundle/Resources/views/Form/theme.html.twig:90

The issue is because form.children|last.vars.name has a non numeric valud (45fee6f4-1e37-11ed-8248-827b9ec727bb).

A change could be:

+               {% if form.children|last.vars.name matches '/^\\d+$/' %}
                 data-index='{{ form.children|length > 0 ? form.children|last.vars.name + 1 : 0 }}'
+               {% endif %}

but I don't know if it keep fixing your own issue :/

@maximehuran
Copy link
Contributor

Hi @AdamKasp

We have this issue in form collections in AJAX, like in Rich Editor Plugin.
For now, we apply the patch on our projects but some people have the same issue and they would like to have the fix in Sylius directly.

@maximehuran
Copy link
Contributor

Hi @GSadee can you have a look to this PR ?

1 similar comment
@maximehuran
Copy link
Contributor

Hi @GSadee can you have a look to this PR ?

@welcoMattic
Copy link

Hi @GSadee can you have a look to this PR, I've met this bug several times in the past weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Potential Bug Potential bugs or bugfixes, that needs to be reproduced.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants