Automattic / woocommerce-subscriptions-core

Subscriptions core package for WooCommerce
Other
87 stars 33 forks source link

wcs_get_subscriptions() ordered by start_date fails to return the correct order. #454

Open prettyboymp opened 1 year ago

prettyboymp commented 1 year ago

Describe the bug

wcs_get_subscriptions() is not properly ordering subscriptions when array( 'orderby' => 'start_date' ) is passed in as the order. ID/create order seems to be returned instead.

To Reproduce

The WCS_Functions_Test::test_5_wcs_get_subscriptions() currently fails to identify this because the assertEquals() comparator does not care about key order or type.

Apply the following diff to update the WCS_Functions_Test::test_5_wcs_get_subscriptions()

index 011d940b..057db60a 100644
--- a/tests/unit/test-wcs-functions.php
+++ b/tests/unit/test-wcs-functions.php
@@ -1144,6 +1144,12 @@ class WCS_Functions_Test extends WP_UnitTestCase {
                        $subscription_2->get_id() => $subscription_2,
                        $subscription_1->get_id() => $subscription_1,
                );
+
+               // We can't trust that array_keys will always be in the same order with numeric keys.
+               $fn_map_to_id = function($subscription) {
+                       return $subscription->get_id();
+               };
+               $this->assertSame( array_map( $fn_map_to_id, $correct_order ), array_map( $fn_map_to_id, $subscriptions ) );
                $this->assertEquals( $subscriptions, $correct_order );

Run the unit test:

./vendor/bin/phpunit --filter=test_5_wcs_get_subscriptions
prettyboymp commented 1 year ago

I don't know that sorting by start_date ever worked properly.

The test_5_wcs_get_subscriptions() test was initially added in https://github.com/woocommerce/woocommerce-subscriptions/pull/1639/files#diff-8dbcd27789dcfb33793110d5fc7dd15567288d956b3ac612f5f1a9ade6fc1fbbR1131.

The array comparator for assertEqual that was likely used by the PHPUnit in use for the project at the time (assumed 5.x) was https://github.com/sebastianbergmann/comparator/blob/2b7424b55f5047b47ac6e5ccb20b2aea4011d9be/src/ArrayComparator.php#L42. Like the one still in use, it wouldn't have compared the order of the elements in the array since assigned keys were used.

wcs_get_subscriptions() would convert the start_date orderby value to just data. WP_Query, would then essentially convert this to when building the clause post_date.

Because start_date is the default orderby value, fixing this may cause unexpected changes for integrations.