Codecov Report

Attention: Patch coverage is 81.10236% with 24 lines in your changes missing coverage. Please review.

Project coverage is 89.60%. Comparing base (53edaa9) to head (f410f49).
Report is 2 commits behind head on 7.x.

@@             Coverage Diff              @@
##                7.x    #1009      +/-   ##
============================================
- Coverage     90.46%   89.60%   -0.86%     
- Complexity     1451     1477      +26     
============================================
  Files           140      146       +6     
  Lines          4238     4330      +92     
============================================
+ Hits           3834     3880      +46     
- Misses          404      450      +46     

Some minor comments, but overall LGTM! 👍

Just saw that the review was not submitted 🤦

Great feedback, thanks so much! I'll make sure to implement your suggestions once I get back at my desk!

Hi @jeromegamez just wanted to make sure if everything is fine on your side, or you need some help regarding the integration of the mapper? Have a good day!

Everything is fine, thanks a lot for asking! I'm just short on time and contemplating if I should leave it with this to introduce it in the first place or if I should wait until I have migrated all the other value objects first - but mostly short on time 😅

Alright, no worries!

I'd recommend releasing it with just a few DTO in the first place. This way, if anything goes badly for end-users (we never know) it will be easier to fix the issues. But your decision, of course. 😊

YOU'RE JUST HERE FOR THE DOWNLOADS!!! 😂

I'd be lying if I said I'm not 😁! I'm mostly interested in seeing if this works fine with a big user base, though. 😛

But I'd have given the same advice anyway! 😅

@romm What I meant to ask - would something like https://github.com/kreait/firebase-php/blob/f410f49f2209ccfb105d02cc39041d594bf89dbe/src/Firebase/Valinor/Source.php make sense to integrate into Valinor by default for the JSON Source?

@romm What I meant to ask - would something like f410f49/src/Firebase/Valinor/Source.php make sense to integrate into Valinor by default for the JSON Source?

I see the value it can bring, however I'm more of an "explicit coding" type of guy, most of the time, so I'd prefer users to use existing Source::* methods.

Also, JSON values can start with [, if you also want to support JSON lists in your own Source::parse() method. 😊

Oh, good point, I will add this, thank you!

Released with 7.21.0 🤗

🚀

I should have a bit more spare time in the upcoming days, do not hesitate to ping me if any issue related to the mapping/normalization occurs, I could probably help!

Also, it just occurred to me (missed it during the review) that no cache is used: do you plan to use it at some point? Do you already use cache in other parts of the library?

Anyway, thanks for keeping me updated!

That was on purpose - I think we discussed this on Mastodon, but I don't remember (if there was) a conclusion... 😅

I wanted to be sure that this wouldn't break production for projects using read-only filesystems, for example on AWS ECS (I have first-hand experience there) or cloud functions (with Bref).

Concerning cache - the SDK comes with a PSR-16 Memory Cache only, but others can be configured with the factory, including the mapper and normalizer cache - I did forget to update the docs, though, I think 😬

Indeed, I forgot about that, it makes sense. People who need better performance can always inject the cache themselves I guess. Updating the docs could be nice indeed. 😊