Fixed erroneous call to getText() method in Mage_Usa_Model_Shipping_Carrier_Ups::getAllowedMethods()

[ragnese]

We ran into an error when getting shipping methods,

[Fri May 24 17:07:36.650694 2024] [:error] [pid 29360] [client 209.215.171.162:7451] PHP Fatal error: Uncaught Error: Call to a member function getText() on string in /opt/data/sites/ www.hubindustrial.com/.modman/hub-src/app/code/local/Mage/Usa/Model/Shipping/Carrier/Ups.php:1344\nStack

The value in question can't ever be anything besides a string from what I see.

[fballiano]

@ragnese do you know how to reproduce the problem? because I'm trying to but it seems that the getAllowedMethods() is never called :-\ at least in the core

[ragnese]

Hmm. Yeah, in our case this code was called by a custom shipping-related module/plugin that we have.

So, I'm not sure if/where it is used by Core. Since it's just a type error (and not a business logic bug), it would be easy to just insert a call to it anywhere you want and watch it crash. Like, you could literally just add,

$allowedMethods = (new Mage_Usa_Model_Shipping_Carrier_Ups())->getAllowedMethods();
var_dump($allowedMethods);

to any code path that you know is going to be called, and you will see the error.

The bug and fix are also very obvious. The for-loop in that method was copy+pasted from the Magento2 code base, but the getCode() method is moved in Magento2 to a helper class (Magento/Ups/Helper/Config) and the getCode() method on that class does not return (arrays of) strings, but instead returns instances of Magento/Framework/Phrase, which has a method getText().

It's clear that the getCode() in OpenMage only returns (arrays of) strings, and therefore will never return anything that has a getText() method.

[fballiano]

@ragnese can you please make the PR editable?

I think the whole method should be this:

    public function getAllowedMethods()
    {
        $allowedMethods = explode(',', (string)$this->getConfigData('allowed_methods'));
        $availableByTypeMethods = $this->getCode('originShipment', $this->getConfigData('origin_shipment'));

        $methods = [];
        foreach ($availableByTypeMethods as $methodCode => $methodData) {
            if (in_array($methodCode, $allowedMethods)) {
                $methods[$methodCode] = $methodData;
            }
        }

        return $methods;
    }

because checkin for isXML or isRest is totally useless

[ragnese]

@fballiano Unfortunately, I'm not sure what it means to make the PR editable... What do I need to do, exactly?

As far as the weird ($isUpsXml || $isUpsRest) check, I agree that it's a little weird. It's how it's done in Magento 2, and I think it's done that way because they still have three options for the 'type' config: "UPS", "UPS_XML", and "UPS_REST". The "UPS" type used the old CGI methods. But we don't have that as an option anymore, so I agree that we can change the logic in this method.

But, I do wonder what's going to happen for users that used to have the type set to "UPS" before upgrading to the new version. I don't think it'll be smooth for them- there will probably be errors as it tries to use the REST API calls with now configured auth keys and stuff. I'm not sure if we should've tried to make the upgrade transition smoother...

[fballiano]

it's this things ;-)

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

[fballiano]

it doesn't really matter if they have UPS in the configuration, UPS is the CGI API which doesn't work since months...

[ragnese]

it doesn't really matter if they have UPS in the configuration, UPS is the CGI API which doesn't work since months...

Yeah, M2 opens a popup in this case, instead of actually using the CGI API:

    protected function _getCgiTracking($trackings)
    {
        //ups no longer support tracking for data streaming version
        //so we can only reply the popup window to ups.
        $result = $this->_trackFactory->create();
        foreach ($trackings as $tracking) {
            $status = $this->_trackStatusFactory->create();
            $status->setCarrier('ups');
            $status->setCarrierTitle($this->getConfigData('title'));
            $status->setTracking($tracking);
            $status->setPopup(1);
            $status->setUrl(
                "http://wwwapps.ups.com/WebTracking/processInputRequest?HTMLVersion=5.0&error_carried=true" .
                "&tracknums_displayed=5&TypeOfInquiryNumber=T&loc=en_US&InquiryNumber1={$tracking}" .
                "&AgreeToTermsAndConditions=yes"
            );
            $result->append($status);
        }

        $this->_result = $result;

        return $result;
    }

But, whatever. That's getting off topic from this specific PR, anyway.

Also, I think there may be a permission mismatch with my Github account and our organization, because I don't see the option to make the PR editable as shown in the link you posted. I'll have to see if I need more permissions for our org. :/