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

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

Closed
1 of 3 tasks
gglobalstep opened this issue Feb 20, 2024 · 11 comments · Fixed by #8387
Assignees
Labels
focus: multi-currency good first issue The issue is a good candidate for the first community contribution/for a newcomer to the team. priority: medium The issue/PR is medium priority—non-critical functionality loss, minimal effect on usability type: bug The issue is a confirmed bug.

Comments

@gglobalstep
Copy link

gglobalstep commented Feb 20, 2024

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]):

  • I have deactivated other plugins and confirmed this bug occurs when only WooCommerce plugin is active.
  • This bug happens with a default WordPress theme active, or Storefront.
  • I can reproduce this bug consistently using the steps above.

`

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
`

@gglobalstep gglobalstep added the type: bug The issue is a confirmed bug. label Feb 20, 2024
@htdat
Copy link
Member

htdat commented Feb 21, 2024

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
Copy link
Member

htdat commented Feb 21, 2024

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

@htdat htdat added the component: customer multi-currency Issues related to customer multi-currency project label Feb 21, 2024
@c-shultz c-shultz added the good first issue The issue is a good candidate for the first community contribution/for a newcomer to the team. label Feb 21, 2024
@pierorocca pierorocca added the priority: medium The issue/PR is medium priority—non-critical functionality loss, minimal effect on usability label Feb 22, 2024
@shaunkuschel
Copy link

Received another report of this on 7817483-zen

@bborman22
Copy link
Contributor

@Jinksi
Copy link
Member

Jinksi commented Mar 7, 2024

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 reykjalin self-assigned this Mar 11, 2024
@pierorocca pierorocca removed the component: customer multi-currency Issues related to customer multi-currency project label Mar 13, 2024
@reykjalin
Copy link
Contributor

This is happening because $currency_code is null here:

return apply_filters( 'wcpay_' . strtolower( $currency_code ) . '_format', $currency_format, $locale );

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:

public function get_price_decimal_separator( $separator ): string {
$currency_code = $this->get_currency_code();
$store_currency_code = $this->get_store_currency()->get_code();
if ( $currency_code === $store_currency_code ) {
$currency_code = $store_currency_code;
$this->price_decimal_separators[ $currency_code ] = $separator;
}
if ( empty( $this->price_decimal_separators[ $currency_code ] ) ) {
$this->price_decimal_separators[ $currency_code ] = $this->localization_service->get_currency_format( $currency_code )['decimal_sep'];
}

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

private function get_currency_code() {
if ( $this->should_use_order_currency() ) {
return $this->order_currency;
}

I went back to ad935b0 (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
Copy link
Contributor

@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
Copy link
Contributor

reykjalin commented Mar 14, 2024

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
Copy link
Contributor

reykjalin commented Mar 14, 2024

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
Copy link
Contributor

reykjalin commented Mar 15, 2024

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
focus: multi-currency good first issue The issue is a good candidate for the first community contribution/for a newcomer to the team. priority: medium The issue/PR is medium priority—non-critical functionality loss, minimal effect on usability type: bug The issue is a confirmed bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants