Closed holtkamp closed 5 years ago
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?
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;
}
Implemented in https://github.com/woohoolabs/yang/commit/eba24d5e35e8ce9ca80b4526d15f3ad497d130d5 and https://github.com/woohoolabs/yang/commit/c4cbab88ad36e8483e6cde3f7b28d4a4cee86e10
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.
nice! Will try to have a look this week 👍
@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
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.
However, I am still not exactly sure how to proceed with the ClassDocumentHydrator
.
Currently I'd favour the following scenario in case of hydrateSingleResource()
:
stdClass
when the document doesn't have any primary datastdClass
While in case of hydrateCollection()
:
stdClass
esThe 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 null
s 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 if
s as the other solutions.
I am really curious about your opinion if my reasoning makes sense to you or you think otherwise.
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.
@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.
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! 😄
👍
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.
Would it be an idea to add
to the
HydratorInterface
?Or even (not sure about this), one with a return type
This way IDE autocompletion also allows
hydrateObject()
when the HydratorInterface is used fetch the actual implementation from a DIC.