Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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...
Comments
Comment #1
kerios83 CreditAttribution: kerios83 commentedI can confirm that, there is also others modules that don't work with paypal WPS - dhtml_menu for example.
Comment #2
kerios83 CreditAttribution: kerios83 commentedComment #3
dhalbert CreditAttribution: dhalbert commentedI 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();
infunction 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 totheme()
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.Comment #4
dhalbert CreditAttribution: dhalbert commentedComment #5
donquixote CreditAttribution: donquixote commentedThis is indeed a premature theme initialization. Apparently in combination with redirect module.
Comment #6
donquixote CreditAttribution: donquixote commentedI think this is indeed major, because it has a huge effect on parts of the site which are not even related to commerce.
Comment #7
donquixote CreditAttribution: donquixote commentedI 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.
Comment #8
darktygur-1 CreditAttribution: darktygur-1 commentedIf 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.
Comment #9
andyg5000I 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.
Comment #10
andyg5000Patch in #9 contained auto-format garbage. Here's is cleaner patch.
Comment #11
dhalbert CreditAttribution: dhalbert commentedUm, I think your -10 patch is incomplete. the -9 patch removes the call to
commerce_paypal_icons()
incommerce_paypal_wps_commerce_payment_method_info()
, and the$display_title
stuff:From the -9 patch.
But the -10 patch is missing this deletion.
Comment #12
andyg5000Good catch! update attached.
Comment #13
dhalbert CreditAttribution: dhalbert commentedSuccessfully 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.
Comment #14
rszrama CreditAttribution: rszrama commentedHmm, 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).
Comment #15
donquixote CreditAttribution: donquixote commentedYup.
entity_get_info() should get theme-agnostic, I would say.
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)
Comment #16
rszrama CreditAttribution: rszrama commentedI 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.
Comment #17
rszrama CreditAttribution: rszrama commentedI 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.
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.
Comment #19
rszrama CreditAttribution: rszrama commentedWell, 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.
Comment #20
rszrama CreditAttribution: rszrama commentedUnfortunately, 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.
Comment #21
rbrownellShould this be cross-posted to the Redirect Issue Queue?
Comment #22
rszrama CreditAttribution: rszrama commentedI believe it's already been dealt with there.
Comment #23
james.williams CreditAttribution: james.williams at ComputerMinds commentedJust 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'sMarkupInterface
? So then thetheme()
call could be avoided until it's actually needed, but it would make it safer to callentity_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 likeredirect_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.
Comment #24
james.williams CreditAttribution: james.williams at ComputerMinds commentedCatchier titles welcome ;-)