Does this module use some king of special theme ? Cause when I turn it on it change my admin theme to my custom theme...

Honestly I don't know wtf is going on :) After disabling this module everything works fine, when module is active and I save something in administration setting my admin theme (I'm using domain theme - DA) is replaced with my custom theme...

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kerios83’s picture

I can confirm that, there is also others modules that don't work with paypal WPS - dhtml_menu for example.

kerios83’s picture

Priority: Normal » Major
dhalbert’s picture

I can confirm this. Most of the time my admin pages display in the admin (Seven) theme, but if I do an operation on an admin page that does an HTTP POST (e.g., Save Configuration on the Modules list page), then the resulting page is displayed with my regular site theme. Refreshing the page (which is not a POST) reverts to the admin theme.

If I disable Paypal WPS, then this problem goes away.

EDIT: Narrowing this down, if I comment out the call to commerce_paypal_icons(); in function commerce_paypal_wps_commerce_payment_method_info() , the problem goes away. I got a hint to try this from #1611570: White screen on every cache flush when Views and Commerce Paypal WPS are enabled at the same time, which describes a different problem.

MORE:
Here is an example stack trace when function commerce_paypal_wps_commerce_payment_method_info() is called. I have added the two items at the top. I believe the call to theme() may be causing the problem, since it is called when we are on an admin page. This stack trace was produced when doing "Save Configuration" to enable PayPal WPS.

     theme()
      commerce_paypal_icons()
#0  commerce_paypal_wps_commerce_payment_method_info()
#1  call_user_func_array(commerce_paypal_wps_commerce_payment_method_info, Array ()) called at [/home/mysite/public_html/includes/module.inc:799]
#2  module_invoke(commerce_paypal_wps, commerce_payment_method_info) called at [/home/mysite/public_html/sites/all/modules/commerce/modules/payment/commerce_payment.module:493]
#3  commerce_payment_methods() called at [/home/mysite/public_html/sites/all/modules/commerce/modules/payment/commerce_payment.module:94]
#4  commerce_payment_entity_info()
#5  call_user_func_array(commerce_payment_entity_info, Array ()) called at [/home/mysite/public_html/includes/module.inc:823]
#6  module_invoke_all(entity_info) called at [/home/mysite/public_html/includes/common.inc:7455]
#7  entity_get_info() called at [/home/mysite/public_html/sites/all/modules/redirect/redirect.module:1033]
#8  redirect_load_entity_from_path(admin/modules) called at [/home/mysite/public_html/sites/all/modules/redirect/redirect.module:217]
#9  redirect_url_inbound_alter(admin/modules, admin/modules, ) called at [/home/mysite/public_html/includes/path.inc:272]
#10 drupal_get_normal_path(admin/modules) called at [/home/mysite/public_html/includes/path.inc:21]
#11 drupal_path_initialize() called at [/home/mysite/public_html/includes/common.inc:5031]
#12 _drupal_bootstrap_full() called at [/home/mysite/public_html/includes/bootstrap.inc:2186]
#13 drupal_bootstrap(7) called at [/home/mysite/public_html/index.php:20]
dhalbert’s picture

Category: feature » bug
Priority: Major » Normal
donquixote’s picture

This is indeed a premature theme initialization. Apparently in combination with redirect module.

19: _drupal_theme_initialize() (Array, 2 elements)
18: drupal_theme_initialize() (Array, 2 elements)
17: theme_get_registry() (Array, 2 elements)
16: theme() (Array, 2 elements)
15: commerce_paypal_icons() (Array, 2 elements)
14: commerce_paypal_wps_commerce_payment_method_info() (Array, 2 elements)
13: call_user_func_array() (Array, 1 element)
12: module_invoke() (Array, 2 elements)
11: commerce_payment_methods() (Array, 2 elements)
10: commerce_payment_entity_info() (Array, 2 elements)
 9: call_user_func_array() (Array, 1 element)
 8: module_invoke_all() (Array, 2 elements)
 7: entity_get_info() (Array, 2 elements)
 6: redirect_load_entity_from_path() (Array, 2 elements)
 5: redirect_url_inbound_alter() (Array, 2 elements)
 4: drupal_get_normal_path() (Array, 2 elements)
 3: drupal_path_initialize() (Array, 2 elements)
 2: _drupal_bootstrap_full() (Array, 2 elements)
 1: drupal_bootstrap() (Array, 2 elements)
 0: main() (Array, 2 elements)
donquixote’s picture

Priority: Normal » Major

I think this is indeed major, because it has a huge effect on parts of the site which are not even related to commerce.

donquixote’s picture

I should add, it only happens after I submit a form.
E.g. on admin/commerce/products/types/product/display

The subsequent request (after the redirect) will have its theme switched.
Probably the redirect module does not fire otherwise.

darktygur-1’s picture

Status: Active » Needs review
FileSize
836 bytes

If the issue is a premature theme initialization caused by the theme() call in commerce_paypal_icons(), then this patch should fix it. Please test.

I noticed that the commit fixing issue #1345822: PayPal Icons broken using multilingual site introduced the theme() call, so I took it back out, taking care to make sure the url is still generated the same way that actually fixed that issue.

andyg5000’s picture

I was unable to replicate the issue, but here is a patch that uses hook_form_alter() to add the icons instead of theming the $display_title. This should prevent any problems described in this issue.

andyg5000’s picture

Patch in #9 contained auto-format garbage. Here's is cleaner patch.

dhalbert’s picture

Um, I think your -10 patch is incomplete. the -9 patch removes the call to commerce_paypal_icons() in commerce_paypal_wps_commerce_payment_method_info(), and the $display_title stuff:

From the -9 patch.

 function commerce_paypal_wps_commerce_payment_method_info() {
   $payment_methods = array();
 
-  $icons = commerce_paypal_icons();
-  $display_title = t('!logo PayPal - pay securely without sharing your financial information', array('!logo' => $icons['paypal']));
-  $display_title .= '<div class="commerce-paypal-icons"><span class="label">' . t('Includes:') . '</span>' . implode(' ', $icons) . '</div>';
-

But the -10 patch is missing this deletion.

andyg5000’s picture

Good catch! update attached.

dhalbert’s picture

Status: Needs review » Reviewed & tested by the community

Successfully tested patch in #12. I believe you have diagnosed and fixed the problem, even if you couldn't duplicate it yourself (I saw the problem when using the Omega theme.)

Thanks for taking the time to work on the module! The various bugs in Commerce Paypal have been showstoppers for me, and I'm now looking forward to using Commerce.

rszrama’s picture

Status: Reviewed & tested by the community » Postponed

Hmm, my problem with this approach is that the display_title property is supposed to be there specifically to provide the HTML used on the checkout form for the payment method's radio button text. I've also never encountered any problems with theme switching, even on my site that uses PayPal WPS with an Omega subtheme. I also don't think we want to remove the use of theme() here, so I wonder where else we might resolve this issue.

In fact, talking this over w/ Damien Tournoud just now and examining your backtrace, we determined the problem lies in the Redirect module. Note that both backtraces have function calls coming from that module, which is attempting to load entities at what appears to be a premature point in the Drupal bootstrap process. If you actually look at the functions in question, you'll even see that this code is completely purposeless, as the result of these functions is ignored. : P

Anyways, it's that code that results in the entity_get_info() calls that cause theme functions to be executed. That said, we also shouldn't need to call theme functions in our entity info definition. The patches above would be fine to use for now, but ultimately we'll want to turn the display title preparation into a callback function. Unfortunately, such an API change cannot happen until Commerce 2.x.

I'm not entirely sure what to do with this present issue, but it seems clear to me the issue doesn't lie in the PayPal module but in Redirect and / or Commerce (and potentially other modules that attempt to handle entities prior to the full bootstrap, which seems like the deeper problem).

donquixote’s picture

That said, we also shouldn't need to call theme functions in our entity info definition.

Yup.
entity_get_info() should get theme-agnostic, I would say.

other modules that attempt to handle entities prior to the full bootstrap, which seems like the deeper problem

depends what we understand by "full bootstrap".
Redirect is indeed quite bold, and trying to fire quite early in the request, in the drupal_path_initialize() bootstrap phase. This is for performance, obviously.

On the other hand, entity_get_info() should be agnostic to the current path, shouldn't it? Entities should not change depending on the page we are on.. (or else it would be a dirty hack)

rszrama’s picture

I think the idea is that the Redirect module could work based on a look-up table instead of trying to actually load entities that may or may not exist in the URL looking for default entity paths.

Also, what's going on in the payment method definition isn't any theme-specific code. It's just that display titles may call the theme() function so aspects of the display title may be re-themed at will (such as this module's CC images). Normally this wouldn't happen until after the full bootstrap has occurred, during which the theme would be properly initialized. Because of Redirect (and potentially other modules), that order is being subverted.

What Damien was saying is that modules shouldn't be using full Drupal APIs like the entity API prior to a full bootstrap. Connecting to the database and reading a lookup table would be fine, though.

rszrama’s picture

Title: Paypal WPS and theme change » Payment method display titles with theme functions initialize the wrong theme with the Redirect module
Project: Commerce PayPal » Commerce Core
Component: PayPal WPS » Payment
Priority: Major » Normal
Status: Postponed » Needs review
FileSize
895 bytes

I had a quick insight this evening while going through the queue and realized we could simply prevent payment methods from being loaded prior to theme initialization. In a quick test using the following hook function, checking to see if the global $theme variable isset() before loading payment methods worked just fine.

function hook_url_inbound_alter(&$path, $original_path, $path_language) {
  $payment_methods = commerce_payment_methods();
}

Before the attached patch, my theme was always switched to the default theme on admin pages instead of using the admin theme. After the patch, I got the correct theme. This may have unintended side effects if some other module caches incorrect information even if the Payment module doesn't use the static cache until it's actually populated, but I don't see any way around that, and in practice I'm not having any issues accessing payment related items in the admin interface.

Attaching a patch for testbot review.

Status: Needs review » Needs work

The last submitted patch, 1518452.drupal_theme_initialize.patch, failed testing.

rszrama’s picture

Status: Needs work » Needs review
FileSize
933 bytes

Well, that won't work. : )

I can mix it up here in commerce_payment_methods(), but I really do worry about poorly cached entity info even more now.

rszrama’s picture

Status: Needs review » Needs work
FileSize
734 bytes

Unfortunately, I've encountered another problem with this approach while testing the patch in conjunction with the PayPal module. That module implements a URL that functions as an IPN listener that uses a dynamic menu argument to load a payment method instance. The problem is that in _drupal_bootstrap_full() in common.inc, menu_set_custom_theme() is called before initializing a theme, and it's that function that is also responsible for autoloading menu arguments.

What happens is that the payment method instance gets loaded as a stub containing only the instance specific data and not the normal payment method data as well, because with this path, the global $theme variable has not been set, causing the function commerce_payment_methods() to return an empty array. I don't have time to tease this out for now, so I'll just have to mark this "needs work" and come back to it later.

In the meantime, folks who want to use the Redirect module can just use the patch attached to this comment to use it with Commerce with no ill effect, since the offending code in the Redirect module isn't even used. :-/

The patch was rolled against Redirect 7.x-1.0-beta4.

One thought to solve this would be to change the patch in #19 to allow pass-through even if $theme isn't set based on a hook or something, so a module like PayPal could explicitly allow payment methods to be loaded on IPN URLs where we know for a fact that whether or not the wrong theme gets initialized is inconsequential.

rbrownell’s picture

Should this be cross-posted to the Redirect Issue Queue?

rszrama’s picture

I believe it's already been dealt with there.

james.williams’s picture

Issue summary: View changes

Just an idea: what if the display_title property was mapped to a classed object that has a __toString() magic method, a bit like Drupal 8's MarkupInterface? So then the theme() call could be avoided until it's actually needed, but it would make it safer to call entity_get_info() during this early part of bootstrap.

Some context: this problem can arise regardless of the redirect module being around or patched. Entity_translation module is the 'culprit' for me, because it calls entity_get_info() early in the bootstrap, just like redirect_load_entity_from_path() did in the reported problem here. It's only a problem immediately after clearing caches.

For reference, the wider issue was resolved in D8 core in #1954892: Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system.

james.williams’s picture

Title: Payment method display titles with theme functions initialize the wrong theme with the Redirect module » Payment method display titles with theme functions initialize the wrong theme with modules that use entity_get_info() early in bootstrap

Catchier titles welcome ;-)