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...

Files: 
CommentFileSizeAuthor
#20 redirect_commerce_compatibility.patch734 bytesrszrama
#19 1518452-19.drupal_theme_initialize.patch933 bytesrszrama
PASSED: [[SimpleTest]]: [MySQL] 3,589 pass(es).
[ View ]
#17 1518452.drupal_theme_initialize.patch895 bytesrszrama
FAILED: [[SimpleTest]]: [MySQL] 3,589 pass(es), 0 fail(s), and 47 exception(s).
[ View ]
#12 commerce_paypal-icons_cause_theme_malfunction-1518452-11.patch2.09 KBandyg5000
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch commerce_paypal-icons_cause_theme_malfunction-1518452-11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#10 commerce_paypal-icons_cause_theme_malfunction-1518452-10.patch1.8 KBandyg5000
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch commerce_paypal-icons_cause_theme_malfunction-1518452-10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#9 commerce_paypal-icons_cause_theme_malfunction-1518452-9.patch4.68 KBandyg5000
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch commerce_paypal-icons_cause_theme_malfunction-1518452-9.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#8 commerce_paypal-premature_theme_initialization-1518452-5.patch836 bytesdarktygur
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch commerce_paypal-premature_theme_initialization-1518452-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

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

Priority:Normal» Major

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]

Category:feature» bug
Priority:Major» Normal

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)

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.

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.

Status:Active» Needs review
StatusFileSize
new836 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch commerce_paypal-premature_theme_initialization-1518452-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

StatusFileSize
new4.68 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch commerce_paypal-icons_cause_theme_malfunction-1518452-9.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

StatusFileSize
new1.8 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch commerce_paypal-icons_cause_theme_malfunction-1518452-10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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.

StatusFileSize
new2.09 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch commerce_paypal-icons_cause_theme_malfunction-1518452-11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Good catch! update attached.

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.

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).

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)

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.

Title:Paypal WPS and theme changePayment method display titles with theme functions initialize the wrong theme with the Redirect module
Project:Commerce PayPal» Drupal Commerce
Component:PayPal WPS» Payment
Priority:Major» Normal
Status:Postponed» Needs review
StatusFileSize
new895 bytes
FAILED: [[SimpleTest]]: [MySQL] 3,589 pass(es), 0 fail(s), and 47 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new933 bytes
PASSED: [[SimpleTest]]: [MySQL] 3,589 pass(es).
[ View ]

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.

Status:Needs review» Needs work
StatusFileSize
new734 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.

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

I believe it's already been dealt with there.