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

Nested CollectionTypes #7538

Closed
TheFox opened this issue Oct 14, 2021 · 3 comments · Fixed by #7624
Closed

Nested CollectionTypes #7538

TheFox opened this issue Oct 14, 2021 · 3 comments · Fixed by #7624

Comments

@TheFox
Copy link

TheFox commented Oct 14, 2021

I have a similar problem to #6777. In the latest version 3.105.3 in AdminHelper, the line

$childFormBuilder = $this->getChildFormBuilder($formBuilder, $elementId);

in is returning null when I am using nested Sonata\Form\Type\CollectionType.

In my Entity I have to expose the Collection

public function getAnswers(): Collection
{
    return $this->answers;
}

instead of only the array

/**
 * @return array<Answer>
 */
public function getAnswers(): array
{
    return $this->answers->toArray();
}

Otherwise it will not work because the else block is reached in the AdminHelper class.

In the loop of getChildFormBuilder the value for $elementId is s616839720b0e2_questions_0_answers. But there is only a s616839720b0e2_questions in the array (iterator). Not sure if this is the case of the problem.

@VincentLanglet
Copy link
Member

It seems the right way to me to always expose the Collection. Especially for something called a CollectionType.
Never used an array instead of Collection for my entities.

@TheFox
Copy link
Author

TheFox commented Oct 14, 2021

@VincentLanglet Depends on who you ask. If you ask the SensioLabs guys the collection should not be exposed. In my opinion it shouldn't matter. But who am I, right?

It shouldn't matter if you want to use array or Collection or any other array-like object. If Sonata would use ArrayAccess it shouldn't matter at all.

Instead of calling

$collection->add(new $modelClassName());

you could easily change it to

$collection[] = new $modelClassName();

This would be compatible and easy to change. Doctrine\Common\Collections\Collection already extends from ArrayAccess. And the code would be compatible with other array like objects/interfaces.

You could even remove the check

if (!($collection instanceof Collection))

or just change it to

if (!($collection instanceof \ArrayAccess))

Edit: you have to remove

if (!($collection instanceof Collection))

Native array is not an instanceof \ArrayAccess.

TheFox added a commit to FOX21/SonataAdminBundle that referenced this issue Oct 15, 2021
Fixes sonata-project#7538.

Instead of using the Collection interface an easy switch to ArrayAccess interface can be used.
TheFox added a commit to FOX21/SonataAdminBundle that referenced this issue Oct 15, 2021
Check if it's ArrayAccess or array.
Fixes sonata-project#7538.
@TheFox
Copy link
Author

TheFox commented Mar 9, 2022

@jordisala1991 @VincentLanglet Thank you for the support for ArrayAccess.

I just wondered if all the locations in the code where Collection is used is now supposed to be supported by ArrayAccess, or only this one case in AdminHelper. For example here it is still only Collection, no ArrayAccess:

\assert(null === $collection || $collection instanceof Collection);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants