HPOS: fix handling order meta containing nonexistent class when sync is on

[lsinger]

Submission Review Guidelines:

• I have followed the WooCommerce Contributing Guidelines and the WordPress Coding Standards.
• I have checked to ensure there aren't other open Pull Requests for the same update/change.
• I have reviewed my code for security best practices.
• Following the above guidelines will result in quick merges and clear and detailed feedback when appropriate.

Changes proposed in this Pull Request:

Assume a plugin adds metadata to an order, and that metadata is a serialized object that uses a class that the plugin defines. Now assume the plugin is subsequently deactivated and the class definition necessary for deserializing the object in the order's metadata isn't available anymore.

This is not an issue as long as the metadata isn't touched, but with HPOS and sync turned on, there are situations where the metadata is deserialized, changed (by WordPress Core’s map_deep), and the serialized again. Under PHP less than 8 this was not an issue, but under PHP 8 throws an error.

There is an open ticket in WordPress Core for this issue but it’s not clear when, how, and if the issue will be addressed. So to ship a fix for WooCommerce users experiencing this issue, this PR catches the error and tries to imitate the metadata actions using $wpdb directly.

Detecting this specific error can, as far as I can tell, only be done by matching the kind of error message, which I find a bit sub-optimal. I'd love to hear ideas for alternative ways of detection.

Closes #38304.

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

1. ensure CI passes
2. check out the branch locally
3. run the new unit test under PHP 8.0 like so: nvm use && wp-env stop && WP_ENV_PHP_VERSION=8.0 wp-env start && pnpm test:php:env --filter=test_unserialize_meta_with_nonexistent_class -- the test should pass
4. undo the actual fix by walking back a few commits like so: git checkout 19b2ddf
5. run the new unit test again: pnpm test:php:env --filter=test_unserialize_meta_with_nonexistent_class -- the test should now fail

Instructions for Manual Testing

1. Ensure to test this with WooCommerce running on PHP 8.0 or later.
2. Under WooCommerce / Settings / Advanced / Features, ensure both HPOS is turned on and sync is also turned on.
3. Install the Code Snippets plugin.
4. Create an order.
5. Find the order in wp-admin and note down it's ID.
6. Add the following snippet, replacing __ID__ with the ID of the order you just created. Make sure to select the "execute only once" option.

$incomplete_meta_object_test = function() {
	$order = wc_get_order( __ID__ );
	global $wpdb;
	$order_id = $order->get_id();
	$meta_key = 'test_unserialize_meta_with_nonexistent_class';
	$meta_value = 'O:11:"geoiprecord":14:{s:12:"country_code";s:2:"BE";s:13:"country_code3";s:3:"BEL";s:12:"country_name";s:7:"Belgium";s:6:"region";s:3:"BRU";s:4:"city";s:8:"Brussels";s:11:"postal_code";s:4:"1000";s:8:"latitude";d:50.833300000000001;s:9:"longitude";d:4.3333000000000004;s:9:"area_code";N;s:8:"dma_code";N;s:10:"metro_code";N;s:14:"continent_code";s:2:"EU";s:11:"region_name";s:16:"Brussels Capital";s:8:"timezone";s:15:"Europe/Brussels";}}';
	$order_meta_table = Automattic\WooCommerce\Internal\DataStores\Orders\OrdersTableDataStore::get_meta_table_name();
	$wpdb->query( "INSERT INTO {$order_meta_table} (order_id, meta_key, meta_value) VALUES ({$order_id}, '{$meta_key}', '{$meta_value}')" );
	$order->set_date_modified( time() );
	$order->save();
	$order_results = wc_get_orders(array('limit' => 300,'offset' => 0,'order' => 'DESC','order_by' => 'ID','return' => 'objects','meta_key' => $meta_key,'meta_value' => '','meta_compare' => '!='));
	$meta = $order_results[0]->get_meta( $meta_key );
	$logger = wc_get_logger();
	$logger->error( 'results: ' . var_export( $meta, true ) );
};
add_action( 'woocommerce_after_register_post_type', $incomplete_meta_object_test );

7. Save the snippet, and click "Execute Once".
8. Reload the page. You should not get an error.
9. Possibly optional (?) -- to ensure the above instructions are correct, retry the same using the latest official WooCommerce. An error should be thrown in the last step after reloading: "Fatal error: Uncaught Error: The script tried to modify a property on an incomplete object. [...]" Another reload should make the error go away. When you look at the error logs (http://localhost:8888/wp-admin/admin.php?page=wc-status&tab=logs), the error should be in there as well.

Changelog entry

• Automatically create a changelog entry from the details below.

Significance

• Patch
• Minor
• Major

Type

• Fix - Fixes an existing bug
• Add - Adds functionality
• Update - Update existing functionality
• Dev - Development related task
• Tweak - A minor adjustment to the codebase
• Performance - Address performance issues
• Enhancement - Improvement to existing functionality

Message

Comment

[github-actions]

Test Results Summary

Commit SHA: b9b4a4e

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	259	0	0	2	0	261	0m 37s
E2E Tests	292	0	0	7	0	299	6m 40s

---

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

[github-actions]

Hi @jorgeatorres, @Konamiman, @vedanshujain,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

[Konamiman]

I get an error when trying to run all the tests of the file (so without the --filter):

OrdersTableDataStoreTests::test_order_updated_webhook_delivered_once
Error: Class "WC_Tests_Webhook_Functions" not found

[lsinger]

I get an error when trying to run all the tests of the file (so without the --filter):

OrdersTableDataStoreTests::test_order_updated_webhook_delivered_once
Error: Class "WC_Tests_Webhook_Functions" not found

@Konamiman Unfortunately I cannot reproduce this locally. Checking out this branch, running pnpm install, and then pnpm test:php:env gives me (after 411s):

OK, but incomplete, skipped, or risky tests!
Tests: 3971, Assertions: 14354, Skipped: 9.
✔ Ran `vendor/bin/phpunit -c phpunit.xml --verbose` in 'tests-cli'. (in 411s 82ms)

Did you run pnpm install and possibly pnpm build after checking out the branch? I've had issues about files not being found before and that fixed it. My errors looked very different though.

Note that all the CI checks pass and the class that is not being found in your setup doesn't seem to have anything to do with the changes in this PR -- seems like this could be a local issue?

[vedanshujain]

Looks good to me, but we need to modify testing instructions so that folks are able to test it without unit test setup. Perhaps we can use a small dummy plugin that adds a class and we can then enable/disable that plugin during testing.

[Konamiman]

@lsinger I'm running the test as follows: vendor/bin/phpunit tests/php/src/Internal/DataStores/Orders/OrdersTableDataStoreTests.php (I always run the unit test locally like that). Shouldn't this be equivalent to what you posted in the testing instructions?

[lsinger]

Looks good to me, but we need to modify testing instructions so that folks are able to test it without unit test setup. Perhaps we can use a small dummy plugin that adds a class and we can then enable/disable that plugin during testing.

Good point @vedanshujain. I've add testing instructions for situations where git and unit tests might not be available. Please take another look.

[lsinger]

@lsinger I'm running the test as follows: vendor/bin/phpunit tests/php/src/Internal/DataStores/Orders/OrdersTableDataStoreTests.php (I always run the unit test locally like that). Shouldn't this be equivalent to what you posted in the testing instructions?

@Konamiman Should -- maybe. ☺️ Unfortunately our unit tests aren't well isolated from each other, so there's definitely a chance that just running the single test file is an issue. We can't rule out anything being off in your local setup, though -- that's what I like about the ephemerality of wp-env. FWIW, the pnpm script runs this command in the container: vendor/bin/phpunit -c phpunit.xml --verbose

Maybe adding the configuration file argument explicitly makes a difference?

[vedanshujain]

Looks good, can you add one more test which checks for deleting meta as well, just like we are covering adding meta.

[Konamiman]

@lsinger What fixes the test for me is adding the following to the test:

require_once __DIR__ . '/../../../../../legacy/unit-tests/webhooks/functions.php';

I think this makes sense, as we are referencing a file in tests/legacy from withing a file in tests/php - I would expect an explicit include to be required in this case.

[jorgeatorres]

@lsinger: While looking at this it occurred to me that instead of trying to capture the throwable and determine its nature by the error message, we could preemptively detect when $metadata->value is of type __PHP_Incomplete_Class and resort to the direct queries in that case?

I know it's not exactly the same logic but given we only seem to care about that one throwable, the result is pretty much the same (and maybe even bit cleaner).

There might be some limitation I'm ignoring, so please let me know if that's the case 😸.

[lsinger]

@lsinger What fixes the test for me is adding the following to the test:

require_once __DIR__ . '/../../../../../legacy/unit-tests/webhooks/functions.php';

I think this makes sense, as we are referencing a file in tests/legacy from withing a file in tests/php - I would expect an explicit include to be required in this case.

@Konamiman interesting -- this should be a problem on trunk as well then? 🤔 Should we file an issue?

[lsinger]

@lsinger: While looking at this it occurred to me that instead of trying to capture the throwable and determine its nature by the error message, we could preemptively detect when $metadata->value is of type __PHP_Incomplete_Class and resort to the direct queries in that case?

I know it's not exactly the same logic but given we only seem to care about that one throwable, the result is pretty much the same (and maybe even bit cleaner).

There might be some limitation I'm ignoring, so please let me know if that's the case 😸.

Thank you @jorgeatorres, that's a great suggestion -- I implemented it in 1abb8ed. There might of course be other cases where an exception is thrown, but we haven't come across them yet -- and the old solution would have rethrown them anyway. So I agree that we should prefer this simpler way of handling things.

[lsinger]

Addresses all review comments, would love to get another review. ☺️

[jorgeatorres]

@lsinger: Thanks for making the changes. I haven't properly tested this yet, but I noticed a few things in the code, and so I'm leaving some feedback. Luckily, nothing serious. Let me know what you think.

[lsinger]

@lsinger: Thanks for making the changes. I haven't properly tested this yet, but I noticed a few things in the code, and so I'm leaving some feedback. Luckily, nothing serious. Let me know what you think.

Thank you for having my back, @jorgeatorres -- I should have caught these. All great notes / suggestions, I made the changes. ✅

[vedanshujain]

@jorgeatorres @lsinger , re: the __PHP_Incomplete_Class change, I agree that it's way cleaner than matching the error message, but note that it's not functionally the same. Now we will trigger this if condition in earlier PHP versions where this was not an issue, but also, if map_deep ever changes to actually handle this scenario, we will still be hitting the if code block even if it's not necessary anymore.

It's still should be fine, unless you can think of a scenario when it isn't.

[lsinger]

@jorgeatorres @lsinger , re: the __PHP_Incomplete_Class change, I agree that it's way cleaner than matching the error message, but note that it's not functionally the same. Now we will trigger this if condition in earlier PHP versions where this was not an issue, but also, if map_deep ever changes to actually handle this scenario, we will still be hitting the if code block even if it's not necessary anymore.

It's still should be fine, unless you can think of a scenario when it isn't.

I can't think of a scenario where it wouldn't be fine to go the alternative route. Note also that we do log the instances where we go that way, so it wouldn't be able to happen completely invisibly.

[jorgeatorres]

Hey @vedanshujain!

but note that it's not functionally the same.

I agree, that's why I said that it was mostly but not exactly "the same".

Now we will trigger this if condition in earlier PHP versions where this was not an issue but also, if map_deep ever changes to actually handle this scenario.

We could mitigate this bit by adding a PHP version check, but not sure if it's worth it. As for map_deep() eventually handling this, that'd be great but I don't think that will happen very soon and (even in that case) we would need to have a work around in place.
I'm not sure if error messages are "guaranteed" to remain the same either, so relying on that seems (to me) even less future proof.

It's still should be fine, unless you can think of a scenario when it isn't.

I can't think of one right now but "never say never". Considering this is already an edge case, I'd expect this new code not be hit very often so we would at least have some room to improve it if necessary.

---

Note also that we do log the instances where we go that way, so it wouldn't be able to happen completely invisibly.

@lsinger: Could you expand on this? I don't think we're logging anything now that we would handle the exception before it even occurs. Maybe actually logging this unusual instance of meta sync wouldn't be a bad idea.

[lsinger]

@lsinger: Could you expand on this? I don't think we're logging anything now that we would handle the exception before it even occurs. Maybe actually logging this unusual instance of meta sync wouldn't be a bad idea.

🤦 No, we're not -- I was thinking of #43739. So let me rephrase: we should log this. 💯 I'll add that.

[vedanshujain]

Ok sounds good, let's ship then!

[lsinger]

@jorgeatorres @vedanshujain added logging in 0beb5d5.

Ok sounds good, let's ship then!

FWIW, someone needs to actually hit the "approve" button. 😅

[Konamiman]

this should be a problem on trunk as well then? 🤔 Should we file an issue?

I rather think that when running the full test suite (or with pnpm as you suggest) the missing file gets included somehow (via another test?) and the issue only shows when running the tests for OrdersTableDataStoreTest.php only. So I'd add that require_once statement as it won't hurt anyway.

[lsinger]

this should be a problem on trunk as well then? 🤔 Should we file an issue?

I rather think that when running the full test suite (or with pnpm as you suggest) the missing file gets included somehow (via another test?) and the issue only shows when running the tests for OrdersTableDataStoreTest.php only. So I'd add that require_once statement as it won't hurt anyway.

@Konamiman my intuition is that we should create an individual PR for this as it's fixing a different issue. What do you think?