Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

multi-currency broke (533) #481

Closed
Dinamiko opened this issue Feb 9, 2022 · 5 comments · Fixed by #482
Closed

multi-currency broke (533) #481

Dinamiko opened this issue Feb 9, 2022 · 5 comments · Fixed by #482
Labels
bug Something isn't working
Milestone

Comments

@Dinamiko
Copy link
Contributor

Dinamiko commented Feb 9, 2022

Describe the Bug

The change in #427 resolved an error while manually creating an order, but it caused multi-currency functionality to stop working entirely it seems.

To Reproduce

  1. install PayPal Payments 1.6.5
  2. install Aelia currency switcher
  3. activate multiple currencies for PayPal
  4. set up currency switcher widget/block
  5. visit product and change the currency to a non-store one
  6. click PayPal button
  7. currency in popup window is still the default store currency, not the one set by the currency switcher.

Environment

  • WordPress Version: 5.9
  • WooCommerce Version: 6.2
  • Plugin Version: 1.6.5
@Dinamiko Dinamiko added the bug Something isn't working label Feb 9, 2022
@Dinamiko Dinamiko added this to the Next version milestone Feb 9, 2022
@Dinamiko Dinamiko changed the title multi-currency broke multi-currency broke (533) Feb 9, 2022
@Dinamiko Dinamiko removed this from the Release 1.7.0 milestone Feb 10, 2022
@tivnet
Copy link

tivnet commented Feb 10, 2022

@Dinamiko Hi Emili

  1. Please go ahead and update the plugin. 3 of my clients are having problems with it and it took me yesterday a few hours to debug your code - but then I saw this change!!!
  2. Why do you need that "fallback"? It's already in the function:
    function get_woocommerce_currency() {
    return apply_filters( 'woocommerce_currency', get_option( 'woocommerce_currency' ) );
    }
  3. You should take the currency from the order. Not the currently active currency. Let's say it's a recurrent payment for a subscription? Or it's a manual payment for a renew via My Account?

@AlexP11223
Copy link
Contributor

  1. Why do you need that "fallback"? It's already in the function:

The function uses it as an initial value, not as a fallback. So it is possible that the filter returns for example null like here: https://wordpress.org/support/topic/manual-add-order-error/#post-15190717

@Dinamiko Dinamiko added this to the Release 1.7.0 milestone Feb 11, 2022
@Dinamiko
Copy link
Contributor Author

Dinamiko commented Feb 11, 2022

First of all, thanks @tivnet for the great feedback, here is the plan:

  1. We are reverting to use get_woocommerce_currency as it was in 1.6.4, removing it and use only get_option in 1.6.5 wroke functionality in multi-currency plugins. That´s the most important change included in this issue and is going to be included in the incoming release (1.7.0)
  2. When for some reason get_woocommerce_currency returns null then we add the fallback for that case as @AlexP11223 mentioned in the previous comment.
  3. We have to check the cases mentioned by @tivnet (subscription renewals, renew manually...) to use the currency from the order. We are going to create dedicated issues for that to work on after the upcoming release.

Just to mention that we are already getting the currency from the order when is available in some parts of our code (https://github.com/woocommerce/woocommerce-paypal-payments/search?q=get_currency%28%29), but still is important that we check the mentioned cases to see if they currently works or not.

@tivnet
Copy link

tivnet commented Feb 11, 2022

@AlexP11223 , @Dinamiko
OK, let's assume that some "strange" filter returns null (or empty string) instead of the currency.
In that case you are going to fallback to the base store currency.
Is that a correct behavior? I am not sure. I'd prefer your return 'NO_CURRENCY'; and an error message.
Otherwise, you go to PayPal with a currency that might not be the one expected by the buyer.

And "just for fun", a "strange" filter can also break the get_option:

$pre = apply_filters( "pre_option_{$option}", false, $option, $default );

or

return apply_filters( "option_{$option}", maybe_unserialize( $value ), $option );

:)

@AlexP11223
Copy link
Contributor

Is that a correct behavior?

For the shop currency which was intended for things like checking if the shop uses supported currency - yes, it should be fine.

And yeah, for proper support of multi-currency we need to use the order currency instead of this when handling orders.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants