Automattic / woocommerce-payments

Accept payments via credit card. Manage transactions within WordPress.
https://wordpress.org/plugins/woocommerce-payments/
Other
173 stars 69 forks source link

[GlobalStep] PHP deprecated error "strtolower(): Passing null to parameter #1 ($string) of type string" triggered on "Order received" page. #8232

Closed gglobalstep closed 7 months ago

gglobalstep commented 8 months ago

Bug Description:

PHP deprecated error "strtolower(): Passing null to parameter ($string) of type string" triggered on "Order received" page.

Environment:

Woocommerce Version: WooCommerce 8.6.0 WooCommerce Payments: 7.3.0-test-1 WordPress version: v6.4.3

PC: Windows 10, Chrome(Version 121.0.6167.185) Firefox (Version 122.0.1)

Steps To Reproduce:

  1. Create any test site.
  2. Install and activate all the required plugins (i.e Query monitor).
  3. Install the WooCommerce Payments.
  4. Complete WooCommerce Payments KYC flow.
  5. Go to the store page and add a product to your cart.
  6. Go to Checkout page and click place order.
  7. User should be redirected on "Order received" page.
  8. Observe that, PHP deprecated error "strtolower(): Passing null to parameter ($string) of type string" triggered on "Order received" page.

Stack Trace:

strtolower() Passing null to parameter #1 ($string) of type string is deprecated.txt

Actual Result:

PHP deprecated error "strtolower(): Passing null to parameter ($string) of type string" triggered on "Order received" page.

Expected Result:

Order received page should be displayed without any error.

Screenshot:

#8232

Isolating the problem (mark completed items with an [x]):

` ### WordPress Environment ### WC Version: 8.6.0 REST API Version: ✔ 8.6.0 WC Blocks Version: ✔ 11.8.0-dev Action Scheduler Version: ✔ 3.7.1 Log Directory Writable: ✔ WP Version: 6.4.3 WP Multisite: – WP Memory Limit: 512 MB WP Debug Mode: – WP Cron: ✔ Language: en_US External object cache: ✔ ### Server Environment ### Server Info: nginx PHP Version: 8.1.27 PHP Post Max Size: 2 GB PHP Time Limit: 1200 PHP Max Input Vars: 6144 cURL Version: 8.4.0 OpenSSL/1.1.1w SUHOSIN Installed: – MySQL Version: 10.6.15-MariaDB-log Max Upload Size: 2 GB Default Timezone is UTC: ✔ fsockopen/cURL: ✔ SoapClient: ✔ DOMDocument: ✔ GZip: ✔ Multibyte String: ✔ Remote Post: ✔ Remote Get: ✔ ### Database ### WC Database Version: 8.6.0 WC Database Prefix: wp_ Total Database Size: 3.57MB Database Data Size: 1.69MB Database Index Size: 1.88MB wp_woocommerce_sessions: Data: 0.02MB + Index: 0.02MB + Engine InnoDB wp_woocommerce_api_keys: Data: 0.02MB + Index: 0.03MB + Engine InnoDB wp_woocommerce_attribute_taxonomies: Data: 0.02MB + Index: 0.02MB + Engine InnoDB wp_woocommerce_downloadable_product_permissions: Data: 0.02MB + Index: 0.06MB + Engine InnoDB wp_woocommerce_order_items: Data: 0.02MB + Index: 0.02MB + Engine InnoDB wp_woocommerce_order_itemmeta: Data: 0.02MB + Index: 0.03MB + Engine InnoDB wp_woocommerce_tax_rates: Data: 0.02MB + Index: 0.06MB + Engine InnoDB wp_woocommerce_tax_rate_locations: Data: 0.02MB + Index: 0.03MB + Engine InnoDB wp_woocommerce_shipping_zones: Data: 0.02MB + Index: 0.00MB + Engine InnoDB wp_woocommerce_shipping_zone_locations: Data: 0.02MB + Index: 0.03MB + Engine InnoDB wp_woocommerce_shipping_zone_methods: Data: 0.02MB + Index: 0.00MB + Engine InnoDB wp_woocommerce_payment_tokens: Data: 0.02MB + Index: 0.02MB + Engine InnoDB wp_woocommerce_payment_tokenmeta: Data: 0.02MB + Index: 0.03MB + Engine InnoDB wp_woocommerce_log: Data: 0.02MB + Index: 0.02MB + Engine InnoDB wp_actionscheduler_actions: Data: 0.06MB + Index: 0.11MB + Engine InnoDB wp_actionscheduler_claims: Data: 0.02MB + Index: 0.02MB + Engine InnoDB wp_actionscheduler_groups: Data: 0.02MB + Index: 0.02MB + Engine InnoDB wp_actionscheduler_logs: Data: 0.05MB + Index: 0.03MB + Engine InnoDB wp_commentmeta: Data: 0.02MB + Index: 0.03MB + Engine InnoDB wp_comments: Data: 0.05MB + Index: 0.09MB + Engine InnoDB wp_jetpack_sync_queue: Data: 0.02MB + Index: 0.06MB + Engine InnoDB wp_links: Data: 0.02MB + Index: 0.02MB + Engine InnoDB wp_options: Data: 0.30MB + Index: 0.06MB + Engine InnoDB wp_postmeta: Data: 0.11MB + Index: 0.06MB + Engine InnoDB wp_posts: Data: 0.08MB + Index: 0.06MB + Engine InnoDB wp_snippets: Data: 0.02MB + Index: 0.03MB + Engine InnoDB wp_termmeta: Data: 0.02MB + Index: 0.03MB + Engine InnoDB wp_terms: Data: 0.02MB + Index: 0.03MB + Engine InnoDB wp_term_relationships: Data: 0.02MB + Index: 0.02MB + Engine InnoDB wp_term_taxonomy: Data: 0.02MB + Index: 0.03MB + Engine InnoDB wp_usermeta: Data: 0.02MB + Index: 0.03MB + Engine InnoDB wp_users: Data: 0.02MB + Index: 0.05MB + Engine InnoDB wp_wc_admin_notes: Data: 0.06MB + Index: 0.00MB + Engine InnoDB wp_wc_admin_note_actions: Data: 0.06MB + Index: 0.02MB + Engine InnoDB wp_wc_category_lookup: Data: 0.02MB + Index: 0.00MB + Engine InnoDB wp_wc_customer_lookup: Data: 0.02MB + Index: 0.03MB + Engine InnoDB wp_wc_download_log: Data: 0.02MB + Index: 0.03MB + Engine InnoDB wp_wc_orders: Data: 0.02MB + Index: 0.11MB + Engine InnoDB wp_wc_orders_meta: Data: 0.06MB + Index: 0.09MB + Engine InnoDB wp_wc_order_addresses: Data: 0.02MB + Index: 0.06MB + Engine InnoDB wp_wc_order_coupon_lookup: Data: 0.02MB + Index: 0.03MB + Engine InnoDB wp_wc_order_operational_data: Data: 0.02MB + Index: 0.03MB + Engine InnoDB wp_wc_order_product_lookup: Data: 0.02MB + Index: 0.06MB + Engine InnoDB wp_wc_order_stats: Data: 0.02MB + Index: 0.05MB + Engine InnoDB wp_wc_order_tax_lookup: Data: 0.02MB + Index: 0.03MB + Engine InnoDB wp_wc_product_attributes_lookup: Data: 0.02MB + Index: 0.02MB + Engine InnoDB wp_wc_product_download_directories: Data: 0.02MB + Index: 0.02MB + Engine InnoDB wp_wc_product_meta_lookup: Data: 0.02MB + Index: 0.09MB + Engine InnoDB wp_wc_rate_limits: Data: 0.02MB + Index: 0.02MB + Engine InnoDB wp_wc_reserved_stock: Data: 0.02MB + Index: 0.00MB + Engine InnoDB wp_wc_tax_rate_classes: Data: 0.02MB + Index: 0.02MB + Engine InnoDB wp_wc_webhooks: Data: 0.02MB + Index: 0.02MB + Engine InnoDB ### Post Type Counts ### attachment: 24 page: 7 post: 1 product: 18 product_variation: 7 shop_order_placehold: 15 wp_navigation: 1 wp_template: 2 ### Security ### Secure connection (HTTPS): ✔ Hide errors from visitors: ✔ ### Active Plugins (5) ### Query Monitor: by John Blackbourn – 3.15.0 Code Snippets: by Code Snippets Pro – 3.6.2 Companion Plugin: by Osk – 1.30 WooPayments: by Automattic – 7.3.0-test-1 WooCommerce: by Automattic – 8.6.0 ### Inactive Plugins (1) ### Akismet Anti-spam: Spam Protection: by Automattic - Anti-spam Team – 5.3.1 ### Dropin Plugins (3) ### advanced-cache.php: advanced-cache.php db.php: Query Monitor Database Class (Drop-in) object-cache.php: Memcached ### Settings ### API Enabled: – Force SSL: – Currency: USD ($) Currency Position: left Thousand Separator: , Decimal Separator: . Number of Decimals: 2 Taxonomies: Product Types: external (external) grouped (grouped) simple (simple) variable (variable) Taxonomies: Product Visibility: exclude-from-catalog (exclude-from-catalog) exclude-from-search (exclude-from-search) featured (featured) outofstock (outofstock) rated-1 (rated-1) rated-2 (rated-2) rated-3 (rated-3) rated-4 (rated-4) rated-5 (rated-5) Connected to Woo.com: – Enforce Approved Product Download Directories: ✔ HPOS feature screen enabled: ✔ HPOS feature enabled: ✔ Order datastore: Automattic\WooCommerce\Internal\DataStores\Orders\OrdersTableDataStore HPOS data sync enabled: – ### WC Pages ### Shop base: #5 - /shop/ Cart: #6 - /cart/ Checkout: #7 - /checkout/ My account: #8 - /my-account/ Terms and conditions: ❌ Page not set ### Theme ### Name: Storefront Version: 4.5.4 Author URL: https://woo.com/ Child Theme: ❌ – If you are modifying WooCommerce on a parent theme that you did not build personally we recommend using a child theme. See: How to create a child theme WooCommerce Support: ✔ ### Templates ### Overrides: – ### WooPayments ### Version: 7.3.0-test-1 Connected to WPCOM: Yes WPCOM Blog ID: 229644254 Account ID: acct_1OlnP1CAVGMSj8Mh Payment Gateway: Enabled Test Mode: Enabled Enabled APMs: card WooPay: Disabled WooPay Incompatible Extensions: No Apple Pay / Google Pay: Enabled (product,cart,checkout) Fraud Protection Level: basic Multi-currency: Enabled Public Key Encryption: Disabled Auth and Capture: Enabled Documents: Disabled Logging: Enabled ### Admin ### Enabled Features: activity-panels analytics product-block-editor coupons core-profiler customer-effort-score-tracks import-products-task experimental-fashion-sample-products shipping-smart-defaults shipping-setting-tour homescreen marketing mobile-app-banner navigation onboarding onboarding-tasks product-variation-management product-virtual-downloadable product-external-affiliate product-grouped product-linked remote-inbox-notifications remote-free-extensions payment-gateway-suggestions shipping-label-banner subscriptions store-alerts transient-notices woo-mobile-welcome wc-pay-promotion wc-pay-welcome-page Disabled Features: customize-store minified-js new-product-management-experience product-pre-publish-modal settings async-product-editor-category-field Daily Cron: ✔ Next scheduled: 2024-02-21 07:01:03 +00:00 Options: ✔ Notes: 68 Onboarding: skipped ### Action Scheduler ### Canceled: 8 Oldest: 2024-02-20 07:16:20 +0000 Newest: 2024-02-20 07:43:06 +0000 Complete: 116 Oldest: 2024-02-20 07:02:23 +0000 Newest: 2024-02-20 07:51:01 +0000 Failed: 2 Oldest: 2024-02-20 07:03:28 +0000 Newest: 2024-02-20 07:13:03 +0000 Pending: 1 Oldest: 2024-02-21 07:02:23 +0000 Newest: 2024-02-21 07:02:23 +0000 ### Status report information ### Generated at: 2024-02-20 07:58:38 +00:00 `
htdat commented 8 months ago

I can replicate this issue with Multi-Currency feature. Stacktrace also confirms that:

    wp-content/plugins/woocommerce-payments/includes/class-wc-payments-localization-service.php:73
    strtolower()
    wp-content/plugins/woocommerce-payments/includes/class-wc-payments-localization-service.php:73
    WC_Payments_Localization_Service->get_currency_format()
    wp-content/plugins/woocommerce-payments/includes/multi-currency/FrontendCurrencies.php:200
    WCPay\MultiCurrency\FrontendCurrencies->get_price_decimal_separator()

This is just an issue with PHP 8.1 or above. https://3v4l.org/jF91B I think this issue has still been there but it is only uncovered when testing with a site running on PHP 8.1.

htdat commented 8 months ago

This issue impacts Multi-Currency, so assigning to team @Automattic/fractal cc @bborman22 Assigning as part of Gamma Triage process PcreKM-yM-p2.

shaunkuschel commented 7 months ago

Received another report of this on 7817483-zen

bborman22 commented 7 months ago

Hey team! Please add your planning poker estimate with Zenhub @cesarcosta99 @lovo-h @rafaelzaleski @ricardo

Jinksi commented 7 months ago

I also see this error in the logs when renewing a subscription using Woo Subscriptions 6.0.0. It doesn't seem to prevent a successful payment or sub renewal.

PHP v8.1.27

reykjalin commented 7 months ago

This is happening because $currency_code is null here:

https://github.com/Automattic/woocommerce-payments/blob/0acd132eb2aedc8b3efd607af2a746df7f30a0fa/includes/class-wc-payments-localization-service.php#L73

This seems to happen only when the wc_get_price_decimal_separator hook is being run. We pass null to the WC_Payments_Localization_Service::get_currency_format function from the FrontendCurrencies::get_price_decimal_separtor function.

The deprecation warning appears now because PHP 8.1 deprecated support for passing null to non-nullable function parameters:

Passing null to non-nullable internal function parameters is deprecated

PHP 8.1 Release Announcement (see bottom of page)

The simple fix here is to just check for null values and set them to an empty string ('') when appropriate, but this might be a bug we never caught.

The reason $currency_code is null is because the FrontendCurrencies class has the order currency set to null (or maybe not initialized?) when we try to get the decimal separator which then calls the currency_format function that causes the warning:

https://github.com/Automattic/woocommerce-payments/blob/0acd132eb2aedc8b3efd607af2a746df7f30a0fa/includes/multi-currency/FrontendCurrencies.php#L190-L201

This happens when we decide we should be using the order currency here:

https://github.com/Automattic/woocommerce-payments/blob/0acd132eb2aedc8b3efd607af2a746df7f30a0fa/includes/multi-currency/FrontendCurrencies.php#L364-L367

I went back to ad935b02ddb234d48441817b80477648b4c36020 (October last year) and this deprecation message is still present then.

I even tried to go all the way back to 1518a12 (~2 years ago when this part of the FrontendCurrencies class was last changed by @jessepearson) and after running composer install and reloading the page the same deprecation message shows up.

So it looks like we may have been passing null here for a long time and it might be fine?

@jessepearson this is a longshot, but do you remember if setting the $currency_code to null is ok here? My gut feeling is "no" since that may cause incorrect formatting. It's also going to cause us to call a nonsense wcpay__format hook. If you still remember if there's a good default value we can retrieve during the wc_get_price_decimal_separator hook that could be a good starting point for trying to address this.

Otherwise we might have to try to fetch the order number during the hook to get the right currency 🤔

jessepearson commented 7 months ago

@jessepearson this is a longshot, but do you remember if setting the $currency_code to null is ok here? My gut feeling is "no" since that may cause incorrect formatting. It's also going to cause us to call a nonsense wcpay__format hook. If you still remember if there's a good default value we can retrieve during the wc_get_price_decimal_separator hook that could be a good starting point for trying to address this.

It looks like get_currency_format returns USD settings if null is passed, so I feel like changing null to '' isn't going to change the behavior that's been there for 2+ years.

IIRC this code is looped through about 5 billion times when a page is loaded, so it's likely something that was missed due to our environments have to run php 7.4. It may be that the code is called before the order is actually loaded and we aren't able to get the currency from it.

As mentioned above, the default USD formatting is used in the instance of no currency formatting being found. There's three paths I can think about taking:

  1. If null is passed to get_currency_format as the $currency_code, convert it to '' (or the default store currency code) as mentioned. From there do some tests with different currencies to make sure formatting is displaying correctly. It's likely it will or else we would have had other issues pop up.
  2. Update the argument definition for get_currency_format so that $currency_code is defined to only be a string and then backtrace from there to update the currency code to the default store currency if null is ever found.
  3. Try to figure out what hook/filter is being used to call these methods in the first place and see if they need to be called and if they can not be called so soon.

I know this is more than what you asked for, but I wanted to be as helpful as possible since I am the main one that used to fix these issues.

reykjalin commented 7 months ago

Thank you Jesse! I'll dig some more into the wc_get_price_decimal_separator hook (where this error is happening) and see if I can figure out a good way to get the right order number (or just use a relatively sane default like the store currency).

I might reach out in Slack if I need some more help and then keep this thread up-to-date 👍

IIRC this code is looped through about 5 billion times when a page is loaded

You would be entirely correct here, but I was able to use a debugger to find the affected codepaths 😄

Very thankful xdebug_break() exists 🙏

reykjalin commented 7 months ago

I think we need the actual order currency.

Say a merchant has their store set to a 0-decimal currency, but has multi-currency enabled (say USD, a 2-decimal currency). A customer makes an order in USD.

If that customer opens the order received page, and we hit the conditions we're running into in this issue, the amounts will be formatted according to the store currency with 0 decimal points. That'll mean important information will be missing from the order received page:

image

So I think we have to do everything we can, even reading the URL, to make sure we get the actual order currency.

reykjalin commented 7 months ago

I've discovered a weird thing. The call stack is quite interesting, it looks something like this (formatted like a backtrace, i.e. later function calls towards the top):

  1. FrontendCurrencies::get_price_decimal_separator
  2. apply_filters on the wc_get_price_decimal_separator filter.
  3. FrontendCurrencies::init_order_currency
  4. apply_filters on the woocommerce_thankyou_order_id filter.

We call wc_get_order in init_order_currency. That triggers the wc_get_price_decimal_separator filter which in turn ends up calling get_price_decimal_separator _before the order has been initialized in the FrontendCurrencies class 😥

In essence; there's a cyclical dependency here. We need an initialized order to get the correct decimal separator but to initialize the order we first need the correct decimal separator.

I think the most "correct" solution here would be to remove wc_get_order from init_order_currency to remove the cyclical dependency and instead try to use the DB directly to fetch the currency directly from the DB. At least that's what I'm going to look into next.

reykjalin commented 7 months ago

Well, that's actually just one of the weird cases. Unfortunately this also happens in other kinds of calls stacks where the only thing being requested is the decimal point format without any of the other functions in FrontendCurrencies being called first.