search

OpenMage / magento-lts

Official OpenMage LTS codebase | Migrate easily from Magento Community Edition in minutes! Download the source code for free or contribute to OpenMage LTS | Security vulnerability patches, bug fixes, performance improvements and more.
https://www.openmage.org
Open Software License 3.0
863 stars 438 forks source link

UPS Rest API: handling cases where a single service is returned by UPS #4044

Closed fballiano closed 1 week ago

fballiano commented 2 weeks ago

Everything is explained in https://github.com/OpenMage/magento-lts/issues/4043

Fixes https://github.com/OpenMage/magento-lts/issues/4043

kiatng commented 1 week ago

Ref issue #4043, the property "RatedShipment" is usually an array, but if there is only one item in it, then it is an object.

isset($arr['Service']) looks for the key "Service" in $arr (JSON object) and return true if it's truthy. If the key doesn't exist, it returns false.

@F1Red5 proposal is_string(array_key_first($arr)) returns true on "RatedShipment" originally is a JSON object, it doesn't check that the object has the property "Service".

Based on this, I think testing "RatedShipment" for an object is may be better as it doesn't assume what keys $arr has. But may be it's necessary to check for the key "Service", so I don't know. I defer the decision to the PR author @fballiano.

fballiano commented 1 week ago

the code object of this PR parses a maximum of 10 records, I would never trade readability and understandability of code for such a minor improvement in this context. if we were parsing millions then we should be having this talk, but having this talk now for this specific thing, in my opinion, doesn't make any sense. we should have this lever of participation with the other million things we should be doing instead of wasting energy for this.

I've personally tested all of the UPS implementation, @iamniels tested that this PR works for him (because he submitted it actually) so I really cannot understand all of this chat.