Add HydratorInterface::hydrateDocument(): stdClass

[holtkamp]

Would it be an idea to add

public function hydrateObject(Document $document);

to the HydratorInterface ?

Or even (not sure about this), one with a return type

public function hydrateObject(Document $document) : \stdClass;

This way IDE autocompletion also allows hydrateObject() when the HydratorInterface is used fetch the actual implementation from a DIC.

[kocsismate]

Yes, you are right, this method should be added to the interface. I would prefer this version though:

public function hydrateSingleResource(Document $document);

Unfortunately there is only one problem with it: it is a breaking change. :/ So my idea to deal with the situation would be to create a new interface something like DocumentHydratorInterface (other names are welcome :) ). In the same time I could deprecate the original interface.

What do you think about my ideas?

[holtkamp]

Unfortunately there is only one problem with it: it is a breaking change.

Yeah, that also crossed my mind.

So my idea to deal with the situation would be to create a new interface something like DocumentHydratorInterface

That is an option indeed... Something like:

interface DocumentHydratorInterface extends HydratorInterface {
   public function hydrateObject(Document $document);
}

I think the return type should not be forced to stdClass. Users that have their own hydrators would probably not want to be forced to return a (specialized) stdClass.

As of PHP 7.2 object is allowed as return type so this can then become:

interface DocumentHydratorInterface extends HydratorInterface {
   public function hydrateObject(Document $document) : object;
}

[kocsismate]

Implemented in eba24d5 and c4cbab8

I changed some implementation details in ClassDocumentHydrator: if the primary data is a single resource then an empty array is returned when ClassDocumentHydrator::hydrateCollection() is called, and ClassDocumentHydrator::hydrate() will always return a collection of resources.

Please test this new implementation, and if everything is OK I can release v2.1.0.

[holtkamp]

nice! Will try to have a look this week 👍

[holtkamp]

@kocsismate I had a look and it works nice!

And while we are at it: one point of consideration: would it be an idea to drop the Interface suffix in the name of an Interface?

So

interface Hydrator {}
interface DocumentHydrator extends Hydrator {}

instead of

interface HydratorInterface {}
interface DocumentHydratorInterface extends HydratorInterface

Also see http://verraes.net/2013/09/sensible-interfaces/

[kocsismate]

I really admire Mathias' work and blog posts, but I am not bought in this case. Somehow I always felt that having the Interface suffix helps me better reason about the intent of a class. That's why I stayed away from removing these prefixes/suffixes so far.

[kocsismate]

However, I am still not exactly sure how to proceed with the ClassDocumentHydrator.

Currently I'd favour the following scenario in case of hydrateSingleResource():

• throw an exception when the document's primary data is a collection
• return an empty stdClass when the document doesn't have any primary data
• otherwise the document can be hydrated to an stdClass

While in case of hydrateCollection():

• throw an exception when the document's primary data is a single resource
• return an empty array when the document doesn't have any primary data
• otherwise the document can be hydrated to a collection of stdClasses

The reason why I'd slightly prefer an empty stdClass instead of returning null when the document doesn't have any primary data is because null can mess up static analysis more easily (that's why I got rid of nulls from almost everywhere I could in Yang v2.0).

Also, I think these two solutions do not differ much from each other from the end-user perspective. So instead of checking null:

$dog = $hydrator->hydrateSingleResource($document);
if ($dog !== null) {
    // ...
}

The following should be checked:

$dog = $hydrator->hydrateSingleResource($document);
if ($document->hasAnyPrimaryResources()) {
    // ...
}

The problem with the static analysis happens when the check for null is omitted (which is usually the case), so in this case PHPStan et al. will report an issue that the $dog variable can also be null so nullability must be checked.

It won't happen when stdClass is always returned (although it will still be a run-time issue when a referenced attribute is missing).

To also solve this issue, an exception could also be thrown when the document is empty so hydration should be done this way:

if ($document->hasAnyPrimaryResources() === false) {
    return;
}

$dog = $hydrator->hydrateSingleResource($document);

Maybe it is a bit too strict 🤔 but the more I write about this topic, the more I prefer this solution as it eliminates the two possible issues mentioned (static analysis, missing attributes) while costing the same amount of ifs as the other solutions.

I am really curious about your opinion if my reasoning makes sense to you or you think otherwise.

[holtkamp]

Indeed, for people "not in to" static analysis: when using a high level of strictness, this:

if($dog = $hydrator->hydrateSingleResource($document)){
    echo $dog->name ' says woof!';
}

will generate an error:

Only booleans are allowed in an if condition, stdClass|null given.  

For me, null is fundamentally different than an empty stdClass: returning "nothing" / null is different than returning an object (instantiated stdClass) with no properties at all.

I think that expecting a user to check for attributes to exist on an empty stdClass complicates the implementation in "user land" code.

Example: abstracting JSON-API from user land code
For example, I got an API client X which abstracts the whole JSON-API functionality from the part of software that uses API client X:

    public function getHydratedObjectUsingFilter(Resource $resource, array $filter, array $includes = []) : ?stdClass
    {
        $request  = $this->getFetchRequest($resource, $filter, [], $includes);
        $response = $this->sendRequest($request);

        return $this->getHydratedObjectFromJsonApiResponse($response);
    }

    private function getHydratedObjectFromJsonApiResponse(JsonApiResponse $response) : ?stdClass
    {
        return $response->hasDocument()
            ? $this->hydrateDocumentObject($response->document())
            : null;
    }

    private function hydrateDocumentObject(Document $document) : ?stdClass
    {
        if ($document->isSingleResourceDocument()) {
            return $this->getDocumentHydrator()->hydrateSingleResource($document);
        }

        //A single object is requested, but a collection is returned: the collection should contain at most one entry, for example when a filter is performed using a uniquely identifying attribute like user.email
        $objects = $this->getDocumentHydrator()->hydrateCollection($document);
        if (is_countable($objects)) {
            if (count($objects) === 0) {
                return null;
            }
            if (count($objects) === 1) {
                return current($objects); 
            }

            throw new LogicException(sprintf('At most one Resource expected in document, encountered: %d', count($objects)));
        }

        throw new LogicException(sprintf('Received non-countable hydration result of type %s', gettype($objects)));
    }

And eventually some abstraction levels up, the software that uses the API does not have to bother about the JSON-API:

$user = $userRepository->findOneByEmail('user@domain.ext');
if($user === null){
    $user = $this->createUser(/*.... */);
}
//continue dealing with $user

In this case the responsibility for checking for null is back at the place where it belongs...

Not sure this example makes sense though 😄

Bottomline: I think that reducing static analysis error messages should no influence such design decisions (too much)...

Just my 2 cents!

As you can see, I can abstract away the empty stdClass in the mentioned API-Client as well, no problem.

[kocsismate]

@holtkamp It's clear for me now that returning an empty stdClass is the worst alternative from the 3. But how about null and throwing an exception? Which one would you prefer?

For now, I'd still prefer the latter. Probably you are right that static analysis shouldn't be priority no. 1 but I think allowing users to omit null checks paves the road to legacy apps - these are the kind of apps that are pretty impossible to analyse statically.

[holtkamp]

But how about null and throwing an exception? Which one would you prefer?

Currently I (stil) mostly use the null approach opposed to throwing exceptions. But I kind of know this is bad practice and I should use more exceptions and less conditionals (to check whether something is null).

So I would say: keep this project neat, tidy and as strict as possible and go for the exceptions! 😄

[kocsismate]

👍

I implemented the change in my last commit. I'll release v2.1 very soon with it. :)

Thank you very much for bringing up this problem and for the great discussion! Feel free to reopen the ticket if you find any problems with the new hydrator.