theofidry / AliceDataFixtures

Nelmio Alice extension to persist the loaded fixtures.
MIT License
307 stars 70 forks source link

ObjectManagerPersister bad arguments #202

Closed Helo3615 closed 9 months ago

Helo3615 commented 2 years ago

Hello

Following this issue : https://github.com/theofidry/AliceBundle/issues/6

I just updated theofidry/alice-data-fixtures to 1.5.1 and got this error by launching fixtures :

  Fidry\AliceDataFixtures\Bridge\Doctrine\Persister\ObjectManagerPersister::g  
  etMetadata(): Argument #2 ($object) must be of type object, array given, ca  
  lled in /app/vendor/theofidry/alice-data-fixtures/src/Bridge/Doctrine/Persi  
  ster/ObjectManagerPersister.php on line 158  

I updated hautelook/alice-bundle to 2.10 as well.

Thanks you

fkizewski commented 2 years ago

Hello

I've the same issue since i've updated theofidry/alice-data-fixtures to 1.5.1.

I my case, it's appear because into my fixtures i'm using array:

App\Entity\User:
  user_2:
    roles: ['@role_*']

To testing and try to understand where is the problem, i've added this condition into the ObjectManagerPersister before the testing line with null, and all is fine now (i known it's a bad idea to do this) :

elseif (is_array($fieldValueOrFieldValues)) {
    foreach ($fieldValueOrFieldValues as $fieldValue) {
        $this->getMetadata($targetEntityClassName, $fieldValue);
    }
}

There is another possibility to replace an array by a collection into fixtures (because collection is managed)? Or it's necessary to update the library?

Thanks for your help.

theofidry commented 2 years ago

Would it be possible to have access to a reproducer or a more complete sample of the issue? i.e. the fixture, the model behind and the complete stack trace? Because with the current information I have no idea what is causing the issue neither where is it coming from

fkizewski commented 2 years ago

Thanks @theofidry for your reply. Please find a more complete sample :

Entities User.php

<?php

namespace App\Entity;

use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\ORM\Mapping as ORM;

/**
 * @ORM\Entity()
 */
class User
{
    /**
     * @ORM\Id()
     * @ORM\GeneratedValue()
     * @ORM\Column(type="integer")
     */
    private ?int $id;

    /**
     * @var ArrayCollection<int, Group>
     *
     * @ORM\ManyToMany(targetEntity="App\Entity\Group", inversedBy="users")
     */
    public $groups;

    public function __construct()
    {
        $this->groups = new ArrayCollection();
    }

    public function getId(): ?int
    {
        return $this->id;
    }
}

Group.php

<?php

namespace App\Entity;

use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\ORM\Mapping as ORM;

/**
 * @ORM\Entity
 * @ORM\Table(name="`group`")
 */
class Group
{
    /**
     * @ORM\Id
     * @ORM\GeneratedValue
     * @ORM\Column(type="integer")
     */
    private ?int $id;

    /**
     * @ORM\Column(type="string", length=20)
     */
    public string $name;

    /**
     * @var ArrayCollection<int, User>|User[]
     *
     * @ORM\ManyToMany(targetEntity="App\Entity\User", mappedBy="groups")
     */
    public $users;

    public function __construct()
    {
        $this->users = new ArrayCollection();
    }

    public function getId(): ?int
    {
        return $this->id;
    }
}

Fixtures User.yaml

App\Entity\User:
  user_1:
    groups: ['@group_*']

Group.yaml

App\Entity\Group:
  group_{1..3}:
    name: group_<current()>

And when i launch the command to generate fixtures : bin/console hautelook:fixtures:load, i've this stack trace :

TypeError : Fidry\AliceDataFixtures\Bridge\Doctrine\Persister\ObjectManagerPersister::getMetadata(): Argument #2 ($object) must be of type object, array given, called in /var/www/vendor/theofidry/alice-data-fixtures/src/Bridge/Doctrine/Persister/ObjectManagerPersister.php on line 158
 /var/www/vendor/theofidry/alice-data-fixtures/src/Bridge/Doctrine/Persister/ObjectManagerPersister.php:111
 /var/www/vendor/theofidry/alice-data-fixtures/src/Bridge/Doctrine/Persister/ObjectManagerPersister.php:158
 /var/www/vendor/theofidry/alice-data-fixtures/src/Bridge/Doctrine/Persister/ObjectManagerPersister.php:119
 /var/www/vendor/theofidry/alice-data-fixtures/src/Bridge/Doctrine/Persister/ObjectManagerPersister.php:158
 /var/www/vendor/theofidry/alice-data-fixtures/src/Bridge/Doctrine/Persister/ObjectManagerPersister.php:119
 /var/www/vendor/theofidry/alice-data-fixtures/src/Bridge/Doctrine/Persister/ObjectManagerPersister.php:158
 /var/www/vendor/theofidry/alice-data-fixtures/src/Bridge/Doctrine/Persister/ObjectManagerPersister.php:119
 /var/www/vendor/theofidry/alice-data-fixtures/src/Bridge/Doctrine/Persister/ObjectManagerPersister.php:158
 /var/www/vendor/theofidry/alice-data-fixtures/src/Bridge/Doctrine/Persister/ObjectManagerPersister.php:119
 /var/www/vendor/theofidry/alice-data-fixtures/src/Bridge/Doctrine/Persister/ObjectManagerPersister.php:158
 /var/www/vendor/theofidry/alice-data-fixtures/src/Bridge/Doctrine/Persister/ObjectManagerPersister.php:119
 /var/www/vendor/theofidry/alice-data-fixtures/src/Bridge/Doctrine/Persister/ObjectManagerPersister.php:79
 /var/www/vendor/theofidry/alice-data-fixtures/src/Loader/PersisterLoader.php:88
 /var/www/vendor/theofidry/alice-data-fixtures/src/Loader/PurgerLoader.php:119
 /var/www/vendor/theofidry/alice-data-fixtures/src/Loader/FileResolverLoader.php:72
 /var/www/vendor/hautelook/alice-bundle/src/Loader/DoctrineOrmLoader.php:157
 /var/www/vendor/hautelook/alice-bundle/src/Loader/DoctrineOrmLoader.php:123
 /var/www/vendor/hautelook/alice-bundle/src/PhpUnit/BaseDatabaseTrait.php:76
 /var/www/vendor/hautelook/alice-bundle/src/PhpUnit/RefreshDatabaseTrait.php:36

I hope this sample can help you to reproduce this bug and you could be able to fix it.

Thanks you

theofidry commented 2 years ago

From the code perspective, this does not look correct to me.

A small nitpick:

    /**
     * @var ArrayCollection<int, Group>
     *
     * @ORM\ManyToMany(targetEntity="App\Entity\Group", inversedBy="users")
     */
    public $groups;

Should probably be:

    /**
     * @var Collection<int, Group>
     *
     * @ORM\ManyToMany(targetEntity="App\Entity\Group", inversedBy="users")
     */
    public $groups;

As that it is an ArrayCollection is probably of no importance, and in practice once fetched I think it can be a different instance.

But more to your problem, you have User#groups which is a Collection or ArrayCollection instance and you try to assign an array to it... This cannot work and if you were using strict types the failure would fail up much easier.

If you want to do something like this, i think it would be more appropriate to make the property private and expose a setter which does $this->groups = new ArrayCollection($groups);

fkizewski commented 2 years ago

Hi @theofidry

Thanks for your help.

I can confirm by changing ArrayCollection to Collection, set my properties to private and adding getter and setter now all is fine.

    /**
     * @var Collection<int, Group>
     *
     * @ORM\ManyToMany(targetEntity="App\Entity\Group", inversedBy="users")
     */
    private Collection $groups;

    public function getGroups(): Collection
    {
        return $this->groups;
    }

    public function setGroups(array $groups): void
    {
        $this->groups = new ArrayCollection($groups);
    }

Last question (after that you can close this issue, of course if it's ok for @TuxxyDOS)

you try to assign an array to it... This cannot work and if you were using strict types the failure would fail up much easier.

Yes you're right, i'm using strict types. It's possible to set a collection into a fixtures writen in yaml than an array, could you please complete my next example (i've make some search on different documentation, and i've not found any example) ?

App\Entity\User:
  user_1:
    groups: ['@group_1', '@group_5']

Thanks you

theofidry commented 2 years ago

It's possible to set a collection into a fixtures writen in yaml than an array

Note that I'm aware unless you are using a custom provider. That said I don't recommend trying to pass an ArrayCollection instead of a Collection: Doctrine collections are state-full, depends on the DB state/connection...

Helo3615 commented 2 years ago

I think I have a different context than fkizewski. However there's probably a type related solution.

Domain\Model\ZmrList

    /**
     * @var iterable|array|Collection
     */
    private iterable $listColumns;

ORM\Maping\ZmrList.orm.yml

Domain\Model\ZmrList:
  type: entity
  oneToMany:
    listColumns:
      targetEntity: ListColumn
      mappedBy: zmrList

fixtures\zmrLists.yaml

Domain\Model\ZmrList:
  zmrList1:
    name: 'BehatTestList1'
    label: 'Behat Test List 1'
    type: 'Entity'
    form: '@form1'
    description: 'A list for automated testing'

I have these informations... :

Doctrine\Persistence\Reflection\TypedNoDefaultReflectionProperty {#132
  +name: "listColumns"
  +class: "Domain\Model\ZmrList"
  modifiers: "private"
  extra: {
    docComment: """
      /**\n
           * @var iterable|array|Collection\n
           */
      """
  }
}
[]
"listColumns"

... by doing this in ObjectManagerPersister :

 if ($fieldValueOrFieldValues instanceof Collection) {
      foreach ($fieldValueOrFieldValues->getValues() as $fieldValue) {
          $this->getMetadata($targetEntityClassName, $fieldValue);
      }
  } elseif (is_array($fieldValueOrFieldValues)) {
      dump($metadata->getReflectionProperty('listColumns'));
      dump($metadata->getReflectionProperty('listColumns')->getValue($object));
      dd($fieldName);
}

Do you have an idea on that @theofidry ?

Thanks you very much for your help

theofidry commented 2 years ago

I think it’s the exact same error: a collection (for relationships) in Doctrine must be Collection instances AFAIK so having an array/iterable there is wrong in that regard

On Mon 17 Jan 2022 at 15:49, Helo @.***> wrote:

I think I have a different context than fkizewski. However there's probably a type related solution.

Domain\Model\ZmrList

/**     * @var iterable|array|Collection     */
private iterable $listColumns;

ORM\Maping\ZmrList.orm.yml

Domain\Model\ZmrList: type: entity oneToMany: listColumns: targetEntity: ListColumn mappedBy: zmrList

fixtures\zmrLists.yaml

Domain\Model\ZmrList: zmrList1: name: 'BehatTestList1' label: 'Behat Test List 1' type: 'Entity' form: @.***' description: 'A list for automated testing'

I have these informations... :

Doctrine\Persistence\Reflection\TypedNoDefaultReflectionProperty {#132 +name: "listColumns" +class: "Domain\Model\ZmrList" modifiers: "private" extra: { docComment: """ /*\n @var iterable|array|Collection\n */ """ } } []"listColumns"

... by doing this in ObjectManagerPersister :

if ($fieldValueOrFieldValues instanceof Collection) { foreach ($fieldValueOrFieldValues->getValues() as $fieldValue) { $this->getMetadata($targetEntityClassName, $fieldValue); } } elseif (is_array($fieldValueOrFieldValues)) { dump($metadata->getReflectionProperty('listColumns')); dump($metadata->getReflectionProperty('listColumns')->getValue($object)); dd($fieldName); }

Do you have an idea on that @theofidry https://github.com/theofidry ?

Thanks you very much for your help

— Reply to this email directly, view it on GitHub https://github.com/theofidry/AliceDataFixtures/issues/202#issuecomment-1014622290, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABHPVANJUVZCHYJ5MP3YEM3UWQUAFANCNFSM5LFUAZSQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

fkizewski commented 2 years ago

I think it’s the exact same error: a collection (for relationships) in Doctrine must be Collection instances AFAIK so having an array/iterable there is wrong in that regard

The only comment i've with your remarks, that's before your last update of the library all is fine with this syntax, i think because we have now a strict types component enabled by default.

Thanks for your time.

tomme87 commented 2 years ago

I have the same issue. It worked just fine before this update.

Please consider reverting the change as I think this is a breaking change. Even if this was not intended to work with array instead of Collection, it did, and it was not clear that it should not work.

theofidry commented 2 years ago

I think this may have been introduced by https://github.com/theofidry/AliceDataFixtures/pull/198.

If it is a problem I'll happily accept a PR reverting this behaviour without reverting the bug fix included in the PR.

Even if this was not intended to work with array instead of Collection, it did, and it was not clear that it should not work.

For what it's worth: this is purely on Doctrine side (to not warn about the incorrect array strict type) but it is clear from the documentation and I'm frankly surprised if your code works at all with it. For this reason, if it is not possible to reconcile the two I would prefer to stick with the current patch rather than reverting it.

dcatz commented 2 years ago

Hi @theofidry,

I am facing the same issue. I have added a dump to see which entity is causing problems.

} elseif ($fieldValueOrFieldValues !== null) {
   if (is_array($fieldValueOrFieldValues)) {
     dump($targetEntityClassName, $fieldValueOrFieldValues); die;
   } else {
     $this->getMetadata($targetEntityClassName, $fieldValueOrFieldValues);
}

Dump result:

\^\ "CoreBundle\Entity\Order\CollectionMethod"
\^\ array:1 [
  0 => CoreBundle\Entity\Order\CollectionMethod\^\ {#31376
    -id: 1
    -name: "Drive-Thru"
  }
]

So it looks some issue with the Order/Collection method relationship.

Here are the entities configurations and fixtures:

Order Entity:

/**
  * @var CollectionMethodEntity $collectionMethod
  *
  * @ORM\ManyToOne(targetEntity="CoreBundle\Entity\Order\CollectionMethod")
  * @ORM\JoinColumn(name="collection_method_id", referencedColumnName="id")
  *
  */
  protected $collectionMethod;

Order Fixture:

CoreBundle\Entity\Order:

  order1:
      collectionMethod: '@collectionMethodDriveThru'

Collection Method Fixture:

CoreBundle\Entity\Order\CollectionMethod:

  collectionMethodDriveThru:
    id: 1
    name: 'Drive-Thru'

If I edit the checkAssociationsMetadata method by adding this seems all to be working as expected:

} elseif ($fieldValueOrFieldValues !== null) {
    if (is_array($fieldValueOrFieldValues)) {
        foreach ($fieldValueOrFieldValues as $fieldValue) {
            $this->getMetadata($targetEntityClassName, $fieldValue);
        }
    } else {
        $this->getMetadata($targetEntityClassName, $fieldValueOrFieldValues);
    }
}

composer.json

"require": {
  "php": "^7.4",
  "ext-ctype": "*",
  "ext-curl": "*",
  "ext-fileinfo": "*",
  "ext-gd": "*",
  "ext-geoip": "*",
  "ext-iconv": "*",
  "ext-json": "*",
  "ext-mongodb": "*",
  "ext-openssl": "*",
  "ext-pdo": "*",
  "ext-zip": "*",
  "composer-runtime-api": "^2.0",
  "alcaeus/mongo-php-adapter": "^1.2",
  "alexpechkarev/geometry-library": "^1.0",
  "algolia/search-bundle": "^5.1",
  "almasaeed2010/adminlte": "^3.1",
  "beberlei/doctrineextensions": "^1.3",
  "doctrine/annotations": "^1.13",
  "doctrine/doctrine-fixtures-bundle": "^3.4",
  "doctrine/doctrine-migrations-bundle": "^3.2",
  "doctrine/inflector": "^2.0",
  "doctrine/mongodb-odm-bundle": "^4.4",
  "fakerphp/faker": "^1.17",
  "friendsofsymfony/oauth-server-bundle": "dev-master",
  "friendsofsymfony/rest-bundle": "^3.2",
  "friendsofsymfony/user-bundle": "dev-master",
  "geocoder-php/google-maps-provider": "^4.6",
  "giggsey/libphonenumber-for-php": "^8.12",
  "google/apiclient": "^2.12",
  "gumlet/php-image-resize": "^2.0",
  "guzzlehttp/guzzle": "^7.4",
  "jean85/pretty-package-versions": "^2.0",
  "jms/serializer": "^3.17",
  "jms/serializer-bundle": "^4.0",
  "kreait/firebase-bundle": "^3.1",
  "lcobucci/jwt": "^4.1",
  "manasbala/doctrine-log-bundle": "^1.0",
  "nelmio/api-doc-bundle": "^4.8",
  "nesbot/carbon": "^2.55",
  "norkunas/onesignal-php-api": "^2.8",
  "nyholm/psr7": "^1.4",
  "omines/datatables-bundle": "^0.5.5",
  "payum/payum-bundle": "^2.4",
  "phpseclib/phpseclib": "^3.0",
  "respect/validation": "^2.2",
  "sensio/framework-extra-bundle": "^6.2",
  "sentry/sdk": "^3.1",
  "sentry/sentry-symfony": "^4.2",
  "sg/datatablesbundle": "^1.2",
  "sonata-project/admin-bundle": "^4.6",
  "sonata-project/block-bundle": "^4.9",
  "sonata-project/doctrine-orm-admin-bundle": "^4.2",
  "sonata-project/media-bundle": "4.0.0-alpha1",
  "sonata-project/translation-bundle": "3.x-dev",
  "sonata-project/user-bundle": "5.x-dev",
  "spatie/opening-hours": "^2.11",
  "stof/doctrine-extensions-bundle": "^1.7",
  "symfony/console": "5.4.*",
  "symfony/dotenv": "5.4.*",
  "symfony/flex": "^1.17|^2",
  "symfony/framework-bundle": "5.4.*",
  "symfony/google-mailer": "5.4.*",
  "symfony/mailer": "5.4.*",
  "symfony/messenger": "5.4.*",
  "symfony/runtime": "5.4.*",
  "symfony/templating": "5.4.*",
  "symfony/webpack-encore-bundle": "^1.13",
  "symfony/yaml": "5.4.*",
  "symfonycasts/reset-password-bundle": "^1.12",
  "twig/string-extra": "^3.3",
  "twig/twig": "^3.3",
  "twilio/sdk": "^6.32",
  "willdurand/geocoder-bundle": "^5.16"
},
"require-dev": {
  "hautelook/alice-bundle": "2.9.0",
  "symfony/debug-bundle": "5.4.*",
  "symfony/maker-bundle": "^1.36",
  "symfony/monolog-bundle": "^3.0",
  "symfony/stopwatch": "5.4.*",
  "symfony/web-profiler-bundle": "5.4.*"
},

Can you please tell me if I am doing something wrong ?

BTW: I have temporarily fixed using the change suggested by overloading the original class with my class via composer.

"autoload-dev": {
  "psr-4": {
    "Fidry\\AliceDataFixtures\\Bridge\\Doctrine\\Persister\\": "app/resources/compat/AliceDataFixtures/Persister"
  }
},
theofidry commented 2 years ago

@dcatz see my reply

theofidry commented 2 years ago

An attempt to fix it in #207. Please try it

dcatz commented 2 years ago

Hi @theofidry,

I have tested and I can confirm #207 is solving the issue for me.

Thx @titiyoyo

theofidry commented 9 months ago

Closed via #207.