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

fix nested admin (one-to-one-to-many relation) that opens inline (fixes #6777) #6778

Merged
merged 1 commit into from
Jan 21, 2021

Conversation

vv12131415
Copy link
Contributor

@vv12131415 vv12131415 commented Jan 17, 2021

Subject

Fix form rendering of nested Admin properties.

I am targeting this branch, because everything is backwards compatible.

Closes #6777.

Changelog

### Fixed
How `AdminHelper::appendFormFieldElement` renders nested forms.

@vv12131415
Copy link
Contributor Author

tests will fail, because I have no idea how do you mock methods that does not exist on interface (talking about hasFormFieldDescription). yes, I've google it

@VincentLanglet
Copy link
Member

tests will fail, because I have no idea how do you mock methods that does not exist on interface (talking about hasFormFieldDescription). yes, I've google it

$this->getMockBuilder(Interface::class)
            ->addMethods(['method'])
            ->getMockForAbstractClass();

?

src/Admin/AdminHelper.php Outdated Show resolved Hide resolved
src/Admin/AdminHelper.php Outdated Show resolved Hide resolved
@vv12131415 vv12131415 force-pushed the fix-nested-admin branch 2 times, most recently from 890548e to 48eec3b Compare January 18, 2021 00:32
Copy link
Member

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test where hasFormFieldDescription return false ?

@vv12131415
Copy link
Contributor Author

There isnone case that I dont know how to handle.
There are nested if

            if ($collection instanceof DoctrinePersistentCollection || $collection instanceof PersistentCollection) {
                //since doctrine 2.4
                $modelClassName = $collection->getTypeClass()->getName();
            } elseif ($collection instanceof Collection) {
                $modelClassName = $this->getEntityClassName($admin, explode('.', preg_replace('#\[\d*?\]#', '', $path)));
            } else {
                throw new \Exception('unknown collection class');
            }

I can check for exception. But how should I handle case with PersistentCollection?
The short answer is - no, just don't check

@VincentLanglet
Copy link
Member

I find the code

if ($collection instanceof DoctrinePersistentCollection || $collection instanceof PersistentCollection) {
                //since doctrine 2.4
                $modelClassName = $collection->getTypeClass()->getName();
            } elseif ($collection instanceof Collection) {
                $modelClassName = $this->getEntityClassName($admin, explode('.', preg_replace('#\[\d*?\]#', '', $path)));
            } else {
                throw new \Exception('unknown collection class');
            }

weird, since a PersistentCollection is a Collection, we should be able to remove the if part.

the collection is

$collection = $this->propertyAccessor->getValue($subject, $path);

So you could mock the propertyAccessor.

@vv12131415 vv12131415 force-pushed the fix-nested-admin branch 2 times, most recently from f33a01a to 06715e9 Compare January 18, 2021 23:26
@VincentLanglet
Copy link
Member

Rebasing 3.x should fix the build

VincentLanglet
VincentLanglet previously approved these changes Jan 19, 2021
@VincentLanglet VincentLanglet requested a review from a team January 19, 2021 19:57
src/Admin/AdminHelper.php Outdated Show resolved Hide resolved
src/Admin/AdminHelper.php Outdated Show resolved Hide resolved
VincentLanglet
VincentLanglet previously approved these changes Jan 20, 2021
@vv12131415 vv12131415 requested a review from phansys January 20, 2021 12:28
@vv12131415
Copy link
Contributor Author

some why on my local machine php-cs-fixer was trying to fix E_USER_DEPRECATED in opposite way

-        ), \E_USER_DEPRECATED);
+        ), E_USER_DEPRECATED);

I don't really understand why

@VincentLanglet
Copy link
Member

Can you try composer update ? or a rebase ?

@VincentLanglet VincentLanglet requested a review from a team January 20, 2021 13:12
@vv12131415
Copy link
Contributor Author

Can you try composer update ? or a rebase ?

  1. deleted cache
  2. rebased to latest commit on 3.x
  3. composer update
    still the same.

Any way, I hope that it's been final fixes of this branch, other way I'll fix it some how

src/Admin/AdminHelper.php Outdated Show resolved Hide resolved
@phansys
Copy link
Member

phansys commented Jan 20, 2021

Could you please use the PR template in order to expose the motivation for these changes in the "Changelog" section?
Thank you.

- remove useless if branch (and some lines in phpstan baseline because of this)
-  cover all cases of this method
- early fail if no collection. Also add better description to the exception
Copy link
Member

@phansys phansys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this PR is fixing something, the "Changelog" section should include the "Fixed" item.

src/Admin/AdminHelper.php Show resolved Hide resolved
@OskarStark OskarStark merged commit c422007 into sonata-project:3.x Jan 21, 2021
@OskarStark
Copy link
Member

Thank you

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

Successfully merging this pull request may close these issues.

Relative admin form is broken after 3.84.0
4 participants