slince / shopify-api-php

:rocket: Shopify API Client for PHP
MIT License
128 stars 48 forks source link

LogicException when trying to serialize multiple models #40

Closed wanze closed 5 years ago

wanze commented 5 years ago

Description

I am trying to use the built in hydrator service to serialize multiple products. It works fine for a single product, but fails when trying to serialize multiple objects. The following exception is thrown:

LogicException in Context.php line 59: This context was already initialized, and cannot be re-used

To reproduce the error, one can fetch multiple products and then try to serialize them:

$product1 = $client->getProductManager()->find(1);
$product2 = $client->getProductManager()->find(2);

$serialized1 = $client->getProductManager()->toArray($product1); // Works fine!
// or $serialized1 = $client->getHydrator()->extract($product1);
$serialized2 = $client->getProductManager()->toArray($product2); // Throws the above exception

How to fix

The Hydrator::extract method must create a new SerializationContext for each call, instead of using the context initialized in its constructor:

    public function extract($object)
    {
        // Create a new context here, do not use $this->serializationContext.
        $serializationContext = SerializationContext::create()->setSerializeNull(true);

        return $this->serializer->toArray($object, $serializationContext);
    }

@slince What do you think about the proposed fix? I can send you a pull request containing the fix with a test case. Other than that I found that the AbstractManager::fromArray() and AbstractManager::toArray() are not part of the ManagerInterface. Is there a reason to not add this methods to the contract so that IDEs are able to autocomplete these methods when dealing with interfaces?

Thanks for creating this library, it is really easy to use! 👍

Cheers

slince commented 5 years ago

Hello

What do you think about the proposed fix? I can send you a pull request containing the fix with a test case

That sounds great. PR is welcome!

Other than that I found that the AbstractManager::fromArray() and AbstractManager::toArray() are not part of the ManagerInterface

These two methods were for internal use, a syntactic sugar; In fact, outside the manager, we recommend $client->getHydrator()->hydrate and $client->getHydrator()->extract

But now, it looks like adding them to the ManagerInterface is fine

slince commented 5 years ago

fixed