Skip to content

Conversation

@msaggiorato
Copy link
Contributor

@msaggiorato msaggiorato commented Jul 29, 2024

All Submissions:

Changes proposed in this Pull Request:

Fixes a fatal error when trying to generate orders. Some countries may have an empty "default_locale" property. In which case, this would throw a fatal (at least in PHP 8).

There's actually, only one case of this here, so I guess I was "lucky" to hit this. This issue should be fixed separately in Woo itself, but checking here to avoid a fatal wouldn't be bad either IMO.

How to test the changes in this Pull Request:

  1. Check out trunk
  2. Run wp eval "var_dump( WC\SmoothGenerator\Generator\CustomerInfo::generate_person('MV') );"
  3. See fatal
  4. Check out this branch
  5. Run wp eval "var_dump( WC\SmoothGenerator\Generator\CustomerInfo::generate_person('MV') );"
  6. See success

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?

Changelog entry

Fix fatal when generating a large amount of orders, which increases the chances of hitting the empty locale issue.

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

Copy link
Contributor

@coreymckrill coreymckrill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@msaggiorato thanks for the PR! Wow, you were indeed lucky to encounter this error. Tested and confirmed that this fixes the issue.

@coreymckrill coreymckrill merged commit 4431276 into woocommerce:trunk Aug 2, 2024
@coreymckrill
Copy link
Contributor

Opened a separate issue to fix the default_locale value for MV:
woocommerce/woocommerce#50322

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants