analogueorm / analogue

Analogue ORM : Data Mapper ORM for Laravel/PHP
MIT License
632 stars 51 forks source link

Error in EntityCache if identity is VO #154

Closed Sufir closed 7 years ago

Sufir commented 7 years ago

I have error with cache if using VO for identity property (PK):

2) UserRepositoryCest: Save
 Test  tests\integration\Infrastructure\UserRepositoryCest.php:save

  [PHPUnit_Framework_Exception] array_key_exists(): The first argument should be either a string or an integer

Scenario Steps:

 1. $I->takeContainer() at tests\integration\Infrastructure\UserRepositoryCest.php:30

#1  Codeception\Subscriber\ErrorHandler->errorHandler
#2  \vendor\analogue\orm\src\System\EntityCache.php:95
#3  \vendor\analogue\orm\src\System\Aggregate.php:1010
#4  \vendor\analogue\orm\src\Commands\Store.php:39
#5  \vendor\analogue\orm\src\System\Mapper.php:171
#6  \vendor\analogue\orm\src\System\Mapper.php:132
#7  \Infrastructure\Repository\AnalogueUserRepository.php:164
#8  \tests\integration\Infrastructure\UserRepositoryCest.php:71
#9  Infrastructure\UserRepositoryCest->save
/**
     * Check if a record for this id exists.
     *
     * @param  string  $id
     * @return boolean
     */
    public function has($id)
    {
        return array_key_exists($id, $this->cache);
    }
RemiCollin commented 7 years ago

Using a ValueObject as a primary key isn't supported indeed.

What is your use case ?

Sufir commented 7 years ago

My case something like this https://github.com/analogueorm/analogue/wiki/Basics#value-objects (but one field), I need VO for that property.

RemiCollin commented 7 years ago

Yes I know, but can you explain the use case in the application context ? So we can see if there is a workaround.

Sufir commented 7 years ago

I solve it so

final class User implements Mappable
{
    /**
     * @var UserIdentity
     */
    private $id;
    /**
     * @var int
     */
    private $groupId;
    /**
     * @var string
     */
    private $phone;
    /**
     * @var UserStatus
     */
    private $status;
    /**
     * @var string
     */
    private $hashIdentity;
    /**
     * @var bool
     */
    private $isActive = true;

    public function __construct(UserIdentity $id, string $phone, UserGroup $group)
    {
        Assertion::phone($phone);
        $this->id = $id;
        $this->phone = $phone;
        $this->groupId = $group->id();
        $this->status = new UserStatus(UserStatus::NORMAL);
    }

    /**
     * @internal
     * @access private
     * {@inheritdoc}
     */
    public function setEntityAttributes(array $attributes)
    {
        $this->id = new UserIdentity($attributes['user_id']);
        $this->groupId = $attributes['group_ref_id'];
        $this->phone= $attributes['phone'];
        $this->status = new UserStatus($attributes['status']);
        $this->hashIdentity = $attributes['user_hash_id'];
        $this->isActive = !!$attributes['is_active'];
    }

    /**
     * @internal
     * @access private
     * {@inheritdoc}
     */
    public function getEntityAttributes()
    {
        return [
            'user_hash_id' => $this->hashIdentity,
            'user_id' => $this->id->getId(),
            'phone' => $this->phone,
            'group_ref_id' => $this->groupId,
            'status' => $this->status->getValue(),
            'is_active' => $this->isActive,
        ];
    }
}

But I think is crutch, that is mapper responsibility. Maybe can be taught his?

RemiCollin commented 7 years ago

I think it's not impossible to do, but would add a whole layer of complexity to the mapper.

Might consider a PR on this though, if it turns out to be a nice and simple solution.

RemiCollin commented 7 years ago

Unrelated, but just a word about the use of final keyword.

Entities as Final classes won't be supported in 5.4 as the new lazyloading proxies will be extending them.

Sufir commented 7 years ago

Oh, about extending... how can I use mocks and proxies with repository? For example in the integration testing I'm trying:

    public function myTest(IntegrationTester $I)
    {
        // ...

        $user = Stub::make(User::class, [
            'id' => function () use ($id) {
                return $id;
            },
        ]);
        $result = $this->repository->remove($user);

        // etc...
    }

In result

[Analogue\ORM\Exceptions\EntityMapNotFoundException] No Map registered for Mock_User_84978179

RemiCollin commented 7 years ago

Depends if you are actually hitting the database or not .

Sufir commented 7 years ago

@RemiCollin yes in this test I'm checking DB too.

RemiCollin commented 7 years ago

Maybe try to register the mocked user class for the test, by explicity binding the UserMap to it. Not sure it would work as it could have side effect on relationships to other object.

For testing against DB, I always use the actual class, using analogue/factory to generate quick objects, and mock any service class the object would call to.

Sufir commented 7 years ago

What is analogue/factory, can be an short example?

RemiCollin commented 7 years ago

https://github.com/analogueorm/factory

Sufir commented 7 years ago

This tool depend from Laravel? I dont use Laravel.

[InvalidArgumentException] Unable to locate factory with name [default] [Domain\User].


$analogue = $container->get(Analogue::class);
    $r = new \ReflectionObject($analogue);
    $p = $r->getProperty('manager');
    $p->setAccessible(true);
    $manager = $p->getValue();

    $factory = new \Analogue\Factory\Factory(new \Faker\Generator(), $manager);

    $user = $factory->make(User::class);
RemiCollin commented 7 years ago

It only depends on Illuminate\Database, but it requires some bindings in the container to work properly. I guess it should be able to work if you look at how it's done in FactoryServiceProvider .

Closing it as it's off the current topic, but feel free to open an issue on Analogue/Factory about this if you need.