The current version of commerce_paypal_wps (indirectly) calls a theme function when hook_entity_info()
is invoked.
This breaks theme switching (e.g. switching to the admin theme on admin pages,
or any custom theme switching logic).
Since the result of hook_entity_info is sometimes cached this
leads to strange bugs like the admin theme being replaced by the default theme
right after clearing the cache but not on any subsequent requests.
What's more this behaviour also depends on the presence of other modules
that influence the order of calls so this bug does not appear under all circumstances,
making it hard to reproduce.
For example, entity_translation needs to be active, too, for the bug to appear in my setup.
I found one cause of the problem:
It's hook_commerce_payment_method_info() in commerce_paypal_wps.
Here's the sequence of calls on my setup:
entity_get_info()
->
commerce_payment_entity_info()
->
commerce_payment_methods()
->
commerce_paypal_wps_commerce_payment_method_info()
->
commerce_paypal_icons()
commerce_paypal_icons tries to theme the icons.
But this is wrong in the above case:
1. The result is cached as part of the result of hook_entity_info() which "freezes" the theme that happens to be active at that time. Chances are you always get the default theme.
2. The fact that a simple call to entity_get_info() indirectly initializes the theme engine breaks core's admin theme funtionality as well as custom theme switching engines like themekey.
See also issue #1858946: Document that it's not allowed to trigger theme functions in hook_entity_info_alter().
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | commerce_paypal-theme-engine-no-init-1862878-4.patch | 1.22 KB | cspitzlay |
| #1 | commerce_paypal-theme-engine-no-init-1862878-1.patch | 1.09 KB | cspitzlay |
Comments
Comment #1
cspitzlayA simple albeit radical fix would be to remove the icons altogether.
See attached patch.
Comment #2
rszrama commentedThanks for posting. It looks like we came to the same conclusion, but there's already a previous issue that I pushed upstream to Commerce itself: #1518452: Payment method display titles with theme functions initialize the wrong theme with modules that use entity_get_info() early in bootstrap
Also, fwiw, I was able to get an upstream fix into the Redirect module, which was the primary incompatibility with this code as is.
Comment #3
cspitzlayThanks for pointing out the duplicate. I agree that it may be better to solve it in commerce itself.
But unfortunately the redirect module is not the main issue for me.
The paypal module breaks the regular theme switching on my site
on each cache clear both my custom rules as well as
the switching to the admin theme attempted by core's system module.
So for the time being I'll have to go with locally patching the payment modules
I need (novalnet has similar problems).
Comment #4
cspitzlayFWIW here is an updated version of the patch that removes the icons.
The first one was not clean and threw a notice.
I kept the !logo placeholder as to not break existing translations.
Comment #5
rszrama commentedMy ultimate solution would be for us to simply use a callback instead of a static string. For PayPal, you should want the logos, so in your case I'd use your patch but also alter back in the logo version to the checkout form.