Feature/kustom 78 Skip missing shipping info when not provided#3
Feature/kustom 78 Skip missing shipping info when not provided#3vaimohe wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
This may not be necessary to add: if you use $this->objectManager->get() early enough in the test, you can call setPostValue to allowing setting the POST contents and have the tested class still grab the request contents correctly.
|
Looks alright, let's avoid updating the CHANGELOG.md just yet though: once we agree on doing a release then we'll merge all necessary changes forward together and bump up the versions accordingly. |
…cit constant types, as this is supported only from PHP 8.3 and above
…ble and with default null value, as sometimes the properties instantiate before setUp() is called in Magento Testing Framework
|
Took the liberty of making couple of changes in the tests: the constant types are not supported in PHP 8.2 and earlier so removed these, also it seemed like the CaptureTest properties needed to be set as nullable since they ended up somehow getting instantiated before setUp is called. |
| { | ||
| // Use getCommentsCollection to load from database | ||
| $comments = $invoice->getCommentsCollection(true); // true = reload | ||
| $comments = $invoice->getCommentsCollection(reload: true); |
There was a problem hiding this comment.
Do we need the reload: part? Not familiar with this syntax in PHP, does it work with older PHP versions too?
There was a problem hiding this comment.
It was introduced in 8.0: https://php.watch/versions/8.0/named-parameters
Did we have a doc how old we need to support?
MGO 2.4.4 requires 8.1 already: https://experienceleague.adobe.com/en/docs/commerce-operations/installation-guide/system-requirements
There was a problem hiding this comment.
Alright good, if it's been a thing for that long no need to worry about it. I suppose technically the PHP constraints in the packages do include 7.4 but I don't think that's the case in reality considering the syntax we're using in some places + supported Magento versions. We need to update those constraints eventually to be more accurate.
One nitpick I do have is that whether or not including it adds anything. I can see the benefit if there's a lot of arguments on a method (which to be fair you want to avoid in the first place) but here we have only one. I guess for readability it's a bit better since you can quickly see what the flag is about, although you might anyway need to check the actual method contents to see what's happening with it. Still, it doesn't matter here, but just some thoughts. The changes look good to me.
There was a problem hiding this comment.
To me it's what you pondered upon, without it the plain true is ambiguous and having it improves high level readability.
2 participants
Previously empty shipping info would be send when it was entirely missing resulting in an error from the API.
Adds integration test for the changed feature and a unit test to verify formatting as it was mentioned on the ticket.