If you try to enable a payment method in a component (instead of in the base rule) you get:

PHP Fatal error:  Call to a member function getPluginName() on a non-object in /home/rfay/workspace/commerce/sites/all/modules/commerce/modules/payment/commerce_payment.rules.inc on line 203

This is a result of attempting to traverse too far up the parentage of the rule. I don't even really understand the use of the rule itself in this action, but the attached patch resolves the problem by preventing the traversal past where there are parents.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rszrama’s picture

Hmm, interesting. With your patch, are you sure it still works properly? As in, will the payment method instance load with all the settings from the action?

rfay’s picture

I just tried it out with Paypal, and it delivered me to paypal with the correct account.

I do think it's odd that it's even using the rule.. Why would it use the rule, when it's the action (that we're executing) that has the information? It looks like it's just using the rule for a hash to create an id or something.

rszrama’s picture

Priority: Critical » Normal

That's what's happening. When we developed it, there was no consistency for Rule IDs, so we had to base it on the name of the Rule and traverse it looking for the action that enables the payment method. I think Rules fixed that bug and we have an open issue to change our ID generation... but honestly I think we'll just wait for 2.x and remove payment gateway configuration from the Rules action altogether (like I did for Commerce Flat Rate for Shipping 2.x).

rfay’s picture

That makes sense. However... One rule can contain more than one action that enables a payment gateway, so it doesn't seem right at all. See the rule in http://www.drupalcommerce.org/node/1247#comment-2847, where one rule (but two components as actions) are involved. However, it would certainly be possible for one rule to have two actions. There's no reason for it to cause a fatal.

I believe that this patch is probably OK then as a short-term approach, but it makes the assumption (as the code already did) that the rules "thing" that contains the action only contains one action.

The reason I marked this critical is that it causes a fatal during expected user interaction.

rszrama’s picture

Ahh, ok. I suppose it just needs better documentation, then, as it's an intentional limitation they're bumping against. The Payment module, because of the (at least past) instability of Rule configuration IDs, always only accepts the first action enabling a payment method as valid. It was a limitation of the system at the time, I just don't know if we've documented it on DC.org.

rfay’s picture

Wonder if we could get this in the current round of commits.

rfay’s picture

tevih’s picture

I had the same issue and the above patch worked.

Thanks!

rfay’s picture

vasike’s picture

Status: Needs review » Reviewed & tested by the community

i can confirm that i was able to reproduce the issue and the patch save the day.

rszrama’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
752 bytes

I've rewritten this a bit to use the instanceof operator instead of depending on the plugin name like we were. I also wasn't sure that we should still allow a payment method to be enabled if for some odd reason we couldn't determine its ultimate ActionContainer. So instead of just ending the while() loop, I'm returning if there is no parent element, which in turn prevents the payment method from being enabled. This will need a good review.

vasike’s picture

Status: Needs review » Reviewed & tested by the community

the last patch with the last dev made the this issue history.

rszrama’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for the review. : )

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.