Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check if data that is going to be deserialized is also empty #815

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nelutihon
Copy link

No description provided.

@jlevers
Copy link
Owner

jlevers commented Nov 12, 2024

Thanks for the contribution @nelutihon!

What problem are you trying to solve here? Using empty makes me a little nervous, because according to the PHP manual, here's how it works:

A variable is considered empty if it does not exist or if its value equals false.

That means that a boolean false, an empty string, etc would all be deserialized to null, which we don't want.

@nelutihon
Copy link
Author

@jlevers I have this error because Amazon returns an empty array as a prep detail
$data is looking like this:
array(0) {
}

This is the error:
In PrepDetails.php line 23:

Too few arguments to function SellingPartnerApi\Seller\FBAInboundV0\Dto\PrepDetails::__construct(), 0 passed in /vendor/jlevers/selling-partner-api/src/Traits/Deserializes.php on line 71 and exactly 2 expected

@iajrz
Copy link
Collaborator

iajrz commented Nov 14, 2024

@nelutihon I understand the issue you're trying to correct: the semantics for what it means to deserialize is not complete, and you're covering a straightforward case of an empty array. You're going with the interpretation that an empty array means a null object.

As jlevers pointed out, empty does more than what you want to do: it also interprets false as a null object. I believe an exception is an appropriate response to trying to deserialize null into an instance of any class, or at least more appropriate than equating it to a null instance.

We've a fork in the road here. We could path is to rewrite the fix on our end instead of having your contribution (lame!).

Now for the cooler paths:

  • you might think through the semantics of deserialization and conclude that an empty array means an empty structure, not necessarily a null one, and handle the empty array not as "null" but as "instance built with the default constructor";
  • you might rewrite the patch to narrowly add the functionality of returning null when trying to deserialize an empty array -- a polish of your current line of thought where the semantics of deseralizing an empty array means the instance is null.

In either of those cooler paths you might even add a few tests for different scenarios to cement how deserialization is interpreted in this project, making you not only a bugfinder and bugfixer, but also a contributor to the maturation of a small but important corner of the project.

Are you interested in moving forward with one of the cooler paths?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants