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.

Sign up for GitHub

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

Improve the way we append parent object if any #6171

Merged
merged 1 commit into from
Jul 13, 2020
Merged

Improve the way we append parent object if any #6171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions UPGRADE-3.x.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,21 @@
UPGRADE 3.x
===========

## Deprecated `SonataAdminBundle\Admin\AdminHelper::addNewInstance()`

Use
```
$instance = $fieldDescription->getAssociationAdmin()->getNewInstance();
SonataAdminBundle\Admin\AdminHelper::addInstance($object, $fieldDescription, $instance);
```
Instead of
```
$this->adminHelper->addNewInstance($object, $fieldDescription);
```

The static method `addInstance()` avoids the need to inject the admin helper dependency,
and adds more flexibility with the instance you're adding to the object.

UPGRADE FROM 3.68 to 3.69
=========================

Expand Down
56 changes: 36 additions & 20 deletions src/Admin/AbstractAdmin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
use Sonata\AdminBundle\Filter\Persister\FilterPersisterInterface;
use Sonata\AdminBundle\Form\FormMapper;
use Sonata\AdminBundle\Form\Type\ModelHiddenType;
use Sonata\AdminBundle\Manipulator\ObjectManipulator;
use Sonata\AdminBundle\Model\ModelManagerInterface;
use Sonata\AdminBundle\Object\Metadata;
use Sonata\AdminBundle\Route\RouteCollection;
Expand Down Expand Up @@ -1365,6 +1366,9 @@ public function getTemplate($name)
public function getNewInstance()
{
$object = $this->getModelManager()->getModelInstance($this->getClass());

core23 marked this conversation as resolved.
Show resolved Hide resolved
$this->appendParentObject($object);

foreach ($this->getExtensions() as $extension) {
$extension->alterNewInstance($this, $object);
}
Expand Down Expand Up @@ -3382,26 +3386,6 @@ protected function buildForm()

$this->loaded['form'] = true;

// append parent object if any
// todo : clean the way the Admin class can retrieve set the object
if ($this->isChild() && $this->getParentAssociationMapping()) {
Copy link

Choose a reason for hiding this comment

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

Hi, @VincentLanglet There is BC break here.
In case of a create form in a subadmin, the parent is not anymore associated.

In my case, to workaround it, I need to associate the parent entity in the subAdmin::preCreate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you create an issue with as much information as you have.
Or directly create the PR to call appendParentObject if needed and explain why ?

I called appendParentObject directly in getNewInstance which should be enough IMO, do you override this method ? @Kal74

Copy link

Choose a reason for hiding this comment

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

Right, I overrided the getNewInstance!
Thanks.

$parent = $this->getParent()->getObject($this->request->get($this->getParent()->getIdParameter()));

$propertyAccessor = $this->getConfigurationPool()->getPropertyAccessor();
$propertyPath = new PropertyPath($this->getParentAssociationMapping());

$object = $this->getSubject();

$value = $propertyAccessor->getValue($object, $propertyPath);

if (\is_array($value) || $value instanceof \ArrayAccess) {
$value[] = $parent;
$propertyAccessor->setValue($object, $propertyPath, $value);
} else {
$propertyAccessor->setValue($object, $propertyPath, $parent);
}
}

$formBuilder = $this->getFormBuilder();
$formBuilder->addEventListener(FormEvents::POST_SUBMIT, function (FormEvent $event) {
$this->preValidate($event->getData());
Expand Down Expand Up @@ -3531,6 +3515,38 @@ protected function configureDefaultSortValues(array &$sortValues)
{
}

/**
* Set the parent object, if any, to the provided object.
*/
final protected function appendParentObject(object $object): void
Copy link
Member

Choose a reason for hiding this comment

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

Do you need this method or can you make it private

Copy link
Member Author

@VincentLanglet VincentLanglet Jul 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't need it.
But since it's called in the getNewInstance and I know some users are overridden this method (maybe without calling the parent method), I was thinking it could be useful to expose the appendParentObject method.

{
if ($this->isChild() && $this->getParentAssociationMapping()) {
$parentAdmin = $this->getParent();
$parentObject = $parentAdmin->getObject($this->request->get($parentAdmin->getIdParameter()));

if (null !== $parentObject) {
$propertyAccessor = $this->getConfigurationPool()->getPropertyAccessor();
$propertyPath = new PropertyPath($this->getParentAssociationMapping());

$value = $propertyAccessor->getValue($object, $propertyPath);

if (\is_array($value) || $value instanceof \ArrayAccess) {
$value[] = $parentObject;
$propertyAccessor->setValue($object, $propertyPath, $value);
} else {
$propertyAccessor->setValue($object, $propertyPath, $parentObject);
}
}
} elseif ($this->hasParentFieldDescription()) {
$parentAdmin = $this->getParentFieldDescription()->getAdmin();
$parentObject = $parentAdmin->getObject($this->request->get($parentAdmin->getIdParameter()));
Copy link

Choose a reason for hiding this comment

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

@VincentLanglet

This fails with embedded documents (MongoDB) when adding new document (let's say it's called Level2NestedDoc) at 2nd level of nesting: Class "App\Document\Level1NestedDoc" does not have an identifier.

Maybe nesting such deep is not a good design (isn't it OK-ish with MongoDB's embedded documents?), but it worked on Sonata Admin v.3.

Now, after update to v.4, to work around the issue we have to override AbstractAdmin::createNewInstance() to omit this method's call. But maybe instead we could

  1. first check the mapping for embedded flag, and/or
  2. then verify that request's id parameter matches parent admin by looking at request's admin code?

What do you think? Should I open an issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Better open an issue, this PR is 2 years old.

But, we won't check embedded in SonataAdmin since it's not related to any persistence bundle.

getObject is doing

final public function getObject($id): ?object
    {
        if (null === $id) {
            return null;
        }

        $object = $this->getModelManager()->find($this->getClass(), $id);
        if (null === $object) {
            return null;
        }

        $this->alterObject($object);
        foreach ($this->getExtensions() as $extension) {
            $extension->alterObject($this, $object);
        }

        return $object;
    }

Where does come from the Exception ?

If it's in the

$this->getModelManager()->find($this->getClass(), $id);

call, we could add some check in https://github.com/sonata-project/SonataDoctrineMongoDBAdminBundle/blob/4.x/src/Model/ModelManager.php

But its weird to have an entity without identifier, since all the admin (and parent admin) are supposed to be related to an entity/document.


if (null !== $parentObject) {
ObjectManipulator::setObject($object, $parentObject, $this->getParentFieldDescription());
}
}
}

/**
* Build all the related urls to the current admin.
*/
Expand Down
66 changes: 17 additions & 49 deletions src/Admin/AdminHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,12 @@
namespace Sonata\AdminBundle\Admin;

use Doctrine\Common\Collections\Collection;
use Doctrine\Common\Util\ClassUtils;
use Doctrine\Inflector\Inflector;
use Doctrine\Inflector\InflectorFactory;
use Doctrine\ODM\MongoDB\PersistentCollection;
use Doctrine\ORM\PersistentCollection as DoctrinePersistentCollection;
use Sonata\AdminBundle\Exception\NoValueException;
use Sonata\AdminBundle\Manipulator\ObjectManipulator;
use Sonata\AdminBundle\Util\FormBuilderIterator;
use Sonata\AdminBundle\Util\FormViewIterator;
use Symfony\Component\Form\FormBuilderInterface;
Expand Down Expand Up @@ -180,16 +180,17 @@ public function appendFormFieldElement(AdminInterface $admin, $subject, $element
$objectCount = null === $value ? 0 : \count($value);
$postCount = \count($data[$childFormBuilder->getName()]);

$associationAdmin = $fieldDescription->getAssociationAdmin();

// add new elements to the subject
while ($objectCount < $postCount) {
// append a new instance into the object
$this->addNewInstance($form->getData(), $fieldDescription);
ObjectManipulator::addInstance($form->getData(), $associationAdmin->getNewInstance(), $fieldDescription);
++$objectCount;
}

$newInstance = $this->addNewInstance($form->getData(), $fieldDescription);
$newInstance = ObjectManipulator::addInstance($form->getData(), $associationAdmin->getNewInstance(), $fieldDescription);

$associationAdmin = $fieldDescription->getAssociationAdmin();
$associationAdmin->setSubject($newInstance);
}

Expand All @@ -214,6 +215,10 @@ public function appendFormFieldElement(AdminInterface $admin, $subject, $element
}

/**
* NEXT_MAJOR: remove this method.
*
* @deprecated since sonata-project/admin-bundle 3.x, use to be removed with 4.0.
*
* Add a new instance to the related FieldDescriptionInterface value.
*
* @param object $object
Expand All @@ -224,53 +229,16 @@ public function appendFormFieldElement(AdminInterface $admin, $subject, $element
*/
public function addNewInstance($object, FieldDescriptionInterface $fieldDescription)
{
$instance = $fieldDescription->getAssociationAdmin()->getNewInstance();
$mapping = $fieldDescription->getAssociationMapping();
$parentMappings = $fieldDescription->getParentAssociationMappings();

$inflector = InflectorFactory::create()->build();

foreach ($parentMappings as $parentMapping) {
$method = sprintf('get%s', $inflector->classify($parentMapping['fieldName']));

if (!(\is_callable([$object, $method]) && method_exists($object, $method))) {
/*
* NEXT_MAJOR: Use BadMethodCallException instead
*/
throw new \RuntimeException(sprintf(
'Method %s::%s() does not exist.',
ClassUtils::getClass($object),
$method
));
}

$object = $object->$method();
}

$method = sprintf('add%s', $inflector->classify($mapping['fieldName']));

if (!(\is_callable([$object, $method]) && method_exists($object, $method))) {
$method = rtrim($method, 's');

if (!(\is_callable([$object, $method]) && method_exists($object, $method))) {
$method = sprintf('add%s', $inflector->classify($inflector->singularize($mapping['fieldName'])));

if (!(\is_callable([$object, $method]) && method_exists($object, $method))) {
/*
* NEXT_MAJOR: Use BadMethodCallException instead
*/
throw new \RuntimeException(sprintf(
'Method %s::%s() does not exist.',
ClassUtils::getClass($object),
$method
));
}
}
}
@trigger_error(sprintf(
'Method %s() is deprecated since sonata-project/admin-bundle 3.x. It will be removed in version 4.0.'
.' Use %s::addInstance() instead.',
__METHOD__,
ObjectManipulator::class
), E_USER_DEPRECATED);
greg0ire marked this conversation as resolved.
Show resolved Hide resolved

$object->$method($instance);
$instance = $fieldDescription->getAssociationAdmin()->getNewInstance();

return $instance;
return ObjectManipulator::addInstance($object, $instance, $fieldDescription);
}

/**
Expand Down
22 changes: 10 additions & 12 deletions src/Form/Type/AdminType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use Sonata\AdminBundle\Admin\AdminInterface;
use Sonata\AdminBundle\Admin\FieldDescriptionInterface;
use Sonata\AdminBundle\Form\DataTransformer\ArrayToModelTransformer;
use Sonata\AdminBundle\Manipulator\ObjectManipulator;
use Symfony\Component\Form\AbstractType;
use Symfony\Component\Form\Extension\Core\Type\CheckboxType;
use Symfony\Component\Form\FormBuilderInterface;
Expand All @@ -36,22 +37,22 @@
class AdminType extends AbstractType
{
/**
* @var AdminHelper
* NEXT_MAJOR: Remove this property.
*
* @var AdminHelper|null
*/
private $adminHelper;

/**
* NEXT_MAJOR: Allow only `AdminHelper` for argument 1 and remove the default null value.
* NEXT_MAJOR: Remove the __construct method.
*/
public function __construct(?AdminHelper $adminHelper = null)
{
// NEXT_MAJOR: Remove this condition.
if (null === $adminHelper) {
if (null !== $adminHelper) {
@trigger_error(sprintf(
'Calling %s without passing an %s as argument is deprecated since sonata-project/admin-bundle 3.66'
.' and will throw an exception in 4.0.',
__METHOD__,
AdminHelper::class
'Passing argument 1 to %s() is deprecated since sonata-project/admin-bundle 3.x'
.' and will be ignored in version 4.0.',
__METHOD__
), E_USER_DEPRECATED);
}

Expand Down Expand Up @@ -107,10 +108,7 @@ static function (array $associationMapping): string {
$subject = $p->getValue($parentSubject, $parentPath.$path);
} catch (NoSuchIndexException $e) {
// no object here, we create a new one
// NEXT_MAJOR: Remove the null check.
if (null !== $this->adminHelper) {
$subject = $this->adminHelper->addNewInstance($parentSubject, $parentFieldDescription);
}
$subject = ObjectManipulator::setObject($admin->getNewInstance(), $parentSubject, $parentFieldDescription);
Copy link
Contributor

Choose a reason for hiding this comment

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

@VincentLanglet There is BC break here, parent object is set to new instance instead of adding new instance to parent collection, and this does not work for me

Copy link
Member Author

Choose a reason for hiding this comment

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

That's intended, to solve #6166
And it works well for me since month.

If you think a special case is missed create an issue with a precise description.
But without any information, I would say that your setter or mapping has something wrong.

}
}
}
Expand Down
Loading