[M10N-144] Fix PHP notices produced when the developer is not set.

[Jaesin]

Fixes apigee/apigee-m10n-drupal#144

[codecov-io]

Codecov Report

Merging #61 into 2.x will not change coverage.
The diff coverage is 100%.

@@            Coverage Diff            @@
##                2.x      #61   +/-   ##
=========================================
  Coverage     91.21%   91.21%           
  Complexity     1725     1725           
=========================================
  Files           267      267           
  Lines          3357     3357           
=========================================
  Hits           3062     3062           
  Misses          295      295

Impacted Files	Coverage Δ	Complexity Δ
...ion/Denormalizer/DeveloperRatePlanDenormalizer.php	100% <100%> (ø)	6 <0> (ø)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3729622...d3b758d. Read the comment docs.

[mxr576]

I'll check this next week because IIRC the fixed logic is also used elsewhere and that property should be available always on existing developer specific rate plans based on testing that I did when I developed this code. Maybe the rate plans is malformed? Do you have other developer rate plan that you can reproduce this bug?

[mxr576]

Yes, I remember now, I created the problematic rate plan and try to push the limits of the API (again ;P) so I really would like to see a step by step guide how this problem could be reproduced in normal circumstances and understand the real problem behind without just blindly fixing the problem you see in m10n.

[Jaesin]

Since developer is a nullable property in the DeveloperPropertyAwareTrait trait and could be not set, it doesn't make since to assume it's there.

[mxr576]

Syntactically you are right, but semantically you are not. As I can see I forgot to explain in a comment on the trait as I did in other places that: The property can be only null when you construct an entity object that contains this trait in PHP but ** it cannot be null after the entity has been created on the API**. Even the API response for the POST requests contains the developer object set and the PHP API client automatically sets the developer property's value on the sent object so subsequent calls can access to the object.

It is a unique design of the Apigee APIs that they expect some required properties as part of the URL parameters (ex.: developer/api product/package ids, etc) and some as part of the request payload.

So even if your reasoning would be correct, you should rather check RatePlanInterface::TYPE_DEVELOPER == $data->type && isset($data->developer) instead of what is in your PR at this moment.

BUT, as I explained this issue should not occur because the RatePlanInterface::TYPE_DEVELOPER == $data->type ensures that this denormalizer only gets called on developer rate plans and if rate plan is a developer rate plan then it must contain the nested developer object in $data->developer and on that object the company property must be either false or true. (Unless you are serializing an "incomplete" developer rate plan constructed in the PHP side that you should not.)

This is the reason why I am asking whether you can show me another rate plan that this problem occurs - preferable not from this test org that I used to hack the MINT API while I developed the integration - or you can provide a step by step guide how to create a malformed developer rate plan object like the one that I (accidentally) created as multiple_products-c4xp_rev_share_and_rate_card.

[cnovak]

This fixed an issue found in an org, but may be an edge case so not something we need to get in right away, but is a better check that what was previously there.

[mxr576]

This fixed an issue found in an org, but may be an edge case so not something we need to get in right away, but is a better check that what was previously there.

This is exactly the opposite that I tried to suggest in my previous comment. We should not merge a bug fix until

1. we are not sure what is the actual bug, IOW we are convinced that it is an actual bug in this library
2. we are unable to reproduce it and cover it with a test case

I do not think that we were able to fulfill either 1 or 2 this time, we just merged something that "fixes" something that we experienced in a playground environment.

The changed logic is also used elsewhere in this library, so now 1% of this library behaves differently than the other 99% where this logic is being used. 🙄

With this approach, sooner or later this library becomes an unmaintainable monolith...