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
869 stars 436 forks source link

Bug: Fatal error in salesOrderBeforeSave event for deleted payment #280

Closed sreichel closed 1 year ago

sreichel commented 7 years ago

See; https://magento.stackexchange.com/questions/104468/getting-php-fatal-error-call-to-a-member-function-getmethodinstance-on-boolea

Possible fix: https://github.com/OpenMage/magento-lts/blob/1.9.3.x/app/code/core/Mage/Payment/Model/Observer.php#L46

Change:

if ($order->getPayment()->getMethodInstance()->getCode() != 'free') {

To:

if ($order->getPayment() && $order->getPayment()->getMethodInstance()->getCode() != 'free') {
Flyingmana commented 7 years ago

in which cases is a deleted payment to be expected?

sreichel commented 7 years ago

Not tested yet, but one (for me) reproducable case could be .... disable M2E Pro Extension and modify an order where payment method is m2e_payment. I'm pretty sure i've alread read about this case (realted to M2E).

Edit:

Just looking to the code, this is indeed a bug ...

Flyingmana commented 7 years ago

in this case you dont delete the payment methods with this code, you configure a pseudo method with the same type to avoid all the errors.(I know there are some errors in the admin area, if the payment method to the code is not there anymore)

I think it is a bad pattern to delete payments. Once there may be a lot of modules, which expect a payment to exist for an order. And you need to reserve it for quite some time, to being able to handle returns correctly.

So I continue asking, is there another case where a payment is expected to be deleted?

colinmollenhour commented 7 years ago

I think @sreichel's suggested case is valid since stores can be installed for many years and naturally things change.. New payment methods are adopted and old ones deprecated. Perhaps the original author quit supporting it or it was replaced with a different method due to PCI compliance changes, etc..

In my opinion if getPayment() method can return something other than a MSMO_Payment then every time it is used the return value should be checked before assuming that it is an object (unless for some reason that is guaranteed to be a safe assumption such as it was just set in the same request). This applies as a general rule for all methods that can return both objects and non-objects and in this case I think the suggested fix is appropriate.

rafaelpatro commented 4 years ago

Hi guys I was facing this issue frequently. I have applied a Try Catch and it works too. Thanks

damien-biasotto commented 4 years ago

Should it sill be open since PR #730 was merged?

sreichel commented 4 years ago

@damien-biasotto #730 is not related.

addison74 commented 2 years ago

This report is almost 5 years old. If there is still a problem in OpenMage and having a solution a PR should be created.

sreichel commented 1 year ago

This report is almost 5 years old.

... and "we" complained about Magentos issue tracker ... :sunglasses: