WC_Data: Throw an exception when attempting to parse an invalid date format #58700
Conversation
Testing GuidelinesHi @naman03malhotra , Apart from reviewing the code changes, please make sure to review the testing instructions (Guide) and verify that relevant tests (E2E, Unit, Integration, etc.) have been added or updated as needed. Reminder: PR reviewers are required to document testing performed. This includes:
|
Test using WordPress PlaygroundThe changes in this pull request can be previewed and tested using a WordPress Playground instance. Test this pull request with WordPress Playground. Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit. |
There was a problem hiding this comment.
Testing the actual failure is difficult because it requires having a Woo store with older data.
👋 Hey Rooco, thanks for taking this forward.
Would it be possible to simulate the behavior that might be causing this issue? I believe it’s important to replicate the exact scenario before proposing a fix that will help us to remove any uncertainty. One approach could be to manually adjust the relevant database values (or use a temporary data stub) to mimic older store data and observe the outcome.
|
@naman03malhotra challenge accepted. |
WalkthroughThis change refines error handling for date properties in WooCommerce's data abstraction, ensuring that invalid date inputs—such as malformed coupon dates—trigger explicit errors instead of causing a TypeError. The update directly addresses a REST API crash when orders contain coupons with incorrectly formatted dates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant REST API (Orders)
participant WC_Data (Order/Coupon)
participant Error Handler
Client->>REST API (Orders): GET /wc/v3/orders
REST API (Orders)->>WC_Data (Order/Coupon): set_date_prop(date_created, value)
alt value is WC_DateTime, numeric, or string
WC_Data (Order/Coupon)-->>REST API (Orders): Set date property
else value is invalid (e.g., stdClass)
WC_Data (Order/Coupon)->>Error Handler: error('invalid_date', ...)
Error Handler-->>REST API (Orders): Return error response
end
REST API (Orders)-->>Client: Return orders or error message
Assessment against linked issues
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
plugins/woocommerce/includes/abstracts/abstract-wc-data.php (3)
22-24: Replace placeholderx.x.xwith a concrete version before releaseDoc-block placeholders are useful during development, but should be resolved to an actual version tag (e.g.
@since 8.9.0) before the code lands intrunk, otherwise the generated developer docs become ambiguous.Also applies to: 30-30
908-910: Follow WP coding-standards: useelseif, notelse ifThe surrounding codebase consistently uses
elseif. Switching to the canonical form avoids scrutiny from PHPCS and keeps the file stylistically uniform.- } else if( is_string( $value ) ) { + } elseif ( is_string( $value ) ) {
917-920: Consider logging the swallowedinvalid_dateexception
$this->error()throws aWC_Data_Exception, but thecatchimmediately swallows all exceptions. While this is an intentional “fail-soft” pattern, completely ignoring the error makes debugging harder.A lightweight compromise:
- // If we get here, the value is not a valid date. - $this->error( 'invalid_date', __( 'Invalid date provided.', 'woocommerce' ) ); + // If we get here, the value is not a valid date. + $this->error( 'invalid_date', __( 'Invalid date provided.', 'woocommerce' ) );- } catch ( Exception $e ) {} // @codingStandardsIgnoreLine. + } catch ( Exception $e ) { + // Surface in debug logs while still failing softly for end-users. + wc_get_logger()->debug( + sprintf( 'set_date_prop(%s) failed: %s', $prop, $e->getMessage() ), + array( 'source' => 'wc-data' ) + ); + }This keeps the public behaviour unchanged yet leaves a breadcrumb trail for site owners.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/woocommerce/changelog/58700-wooplug-4457-rest-api-orders-get-with-coupons-has-typeerror-preg_match(1 hunks)plugins/woocommerce/includes/abstracts/abstract-wc-data.php(3 hunks)plugins/woocommerce/includes/rest-api/Controllers/Version2/class-wc-rest-orders-v2-controller.php(0 hunks)
💤 Files with no reviewable changes (1)
- plugins/woocommerce/includes/rest-api/Controllers/Version2/class-wc-rest-orders-v2-controller.php
⏰ Context from checks skipped due to timeout of 90000ms (24)
- GitHub Check: Blocks e2e tests 7/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 9/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 10/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 6/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core API tests - @woocommerce/plugin-woocommerce [api]
- GitHub Check: Blocks e2e tests 8/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 5/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 4/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 3/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 5/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 1/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 4/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Metrics - @woocommerce/plugin-woocommerce [performance]
- GitHub Check: Blocks e2e tests 3/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 1/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 2/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: PHP: 8.4 WP: latest [WP latest] 1/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: Core e2e tests 6/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 2/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: PHP: 8.4 WP: latest [WP latest] 2/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: PHP: 7.4 WP: latest - 1 [WP 6.7.2] 2/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: PHP: 7.4 WP: latest - 1 [WP 6.7.2] 1/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: Lint - @woocommerce/plugin-woocommerce
- GitHub Check: build
🔇 Additional comments (1)
plugins/woocommerce/changelog/58700-wooplug-4457-rest-api-orders-get-with-coupons-has-typeerror-preg_match (1)
1-4: Changelog entry looks goodClear, concise, and formatted per the existing convention.
|
@roccotripaldi make sure to check the failing tests. For example, linting failed on this PR Coderabbit has also made some suggestions, check them out if they are valid :) Thanks to both of you @naman03malhotra and @roccotripaldi for driving this forward |
There was a problem hiding this comment.
Hey Rocco, thanks for taking it forward. Please do take a look at the failing linter CI and tests, as Panos mentioned. Additionally, it is important that we test this change to ensure it remains future-proof against any unintended alterations.
I'll take a proper look at it next week.
|
Thanks for the review Naman. I spent 2 hours today trying to write a unit test for this, but I couldn't figure out how. There are higher priorities now, so I don't want to spend too much time with this. I'll close this PR, but maybe someone with more Woo experience can take a try at a later date. |
|
Hey @roccotripaldi - before closing the PR, let's discuss what you tested, if you were able to replicate or not the issue , so the next one is aware. Thank you |
Thanks for that Panos. I didn't believe there was any value in this PR, but maybe their is! I've updated the PR description to better summarize my fix, and what I've tried. |
There was a problem hiding this comment.
Hey @roccotripaldi your fix looks good, I've tested it with mutiple invalid date formats and I am able to reproduce it with the instructions you mentioned.
I've created a followup PR to add test for this to make sure we don't regress, I've assigned it for the review: #60022
There was a problem hiding this comment.
Once we merge the next PR with tests: #60022, the next step would be to fix the lint issues and make sure everything is green before we merge.
* added tests for invalid date * lint issues
| @@ -19,7 +19,7 @@ | |||
| * | |||
| * Implemented by classes using the same CRUD(s) pattern. | |||
| * | |||
| * @version 2.6.0 | |||
| * @version x.x.x | |||
There was a problem hiding this comment.
So in core we don't rename it to x.x.x, we add the exact version, i.e the version this PR will be released with.
Also, I think we add @SInCE to preserve the original version.
There was a problem hiding this comment.
What @naman03malhotra said :)
@since 2.6.0
@version 10.2.0
There was a problem hiding this comment.
Hey @roccotripaldi, this should be good to go, I've merged the PR for tests #60022, once the CI passed and some minor feedback to be addressed in the PR.
|
CI is passing now after the fresh pull from trunk. |
Naman gave his thumbs up and all feedback was addressed.
There was a problem hiding this comment.
Approving as CI is passing! testing well and the test PR #60022 is also merged!
|
@roccotripaldi this is ready to be merged, but I'll let you do the honors. |
Submission Review Guidelines:
Changes proposed in this Pull Request:
The issue:
In
WC_Data->set_date_propassumes that the value of the date, if it is not an int or a WC_DateTime object, will be a string. It will attempt to runpreg_matchon the value, without fully ensuring the value is of type string. This will result in fatals in rare cases when the value is something other than an int, a WC_DateTime object, or a string.My fix:
Make sure the value is actually a string before running
preg_match. If it's not a string, throw an exception.Closes #58401
How to test the changes in this Pull Request:
~Testing the actually failure is difficult because it requires having a Woo store with older data. ~
Here is a patch to emulate the error from the bug report:
debug.patch
We can emulate the error like this:
/wp-json/wc/v3/ordersTest the fix in this PR:
You can remove the patch, and finally test:
wp-json/wc/v3/orderscontinue to work.Testing that has already taken place:
I've run through the testing instructions above a few times, and made sure the issue goes away.
I did make some attempts to unit test here, but I wasn't able to create a test that worked, or made any sense!
See my latest commit:
9682157
Changelog entry
Changelog Entry Details
Significance
Type
Message
Changelog Entry Comment
Comment
Fixes a bug where listing Woo orders on the REST API will crash if the order has a coupon with a malformed date.
Summary by CodeRabbit
Bug Fixes
Style