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

Do not attempt to cache orders during order creation #37569

Merged
merged 3 commits into from Apr 5, 2023

Conversation

jorgeatorres
Copy link
Member

@jorgeatorres jorgeatorres commented Apr 4, 2023

Submission Review Guidelines:

Changes proposed in this Pull Request:

As described in #37568, during an order's save various hooks are fired, and after most are executed, the orders cache attempts to cache the order.
Thing is, if during the execution of any of those hooks, wc_get_order() gets called on that same order ID, we end up in a situation where the cache first obtained and saved a specific order class (during the wc_get_order() call) and then WC_Abstract_Order::save() overrides that with the generic WC_Order instance that initiated the save.

This PR attempts to fix this by preventing the cache logic in WC_Abstract_Order::save() from intervening when we're creating an order. During edits/updates, things should be safer, as we should already be working on an object of the correct type.

Closes #37568.

How to test the changes in this Pull Request:

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

  1. Create a test file with the following contents.
    <?php
    add_action(
        'woocommerce_new_order',
        function( $order_id ) {
     	   // this makes the cache store a specific order class instance, but it's quickly replaced by a generic one
     	   // as we're in the middle of a save and this gets executed before the logic in WC_Abstract_Order.
     	   $order = wc_get_order( $order_id );
        }
    );
    
    $order = new \WC_Order();
    $order->save();
    
    $order = wc_get_order( $order->get_id() );
    var_dump( get_class( $order ) );
  2. Confirm that, on trunk, it prints WC_Order.
  3. Confirm that, on this branch, it prints Automattic\WooCommerce\Admin\Overrides\Order, which is the correct order type.

@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Apr 4, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2023

Hi @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

@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2023

Test Results Summary

Commit SHA: 817458a

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 52s
E2E Tests1860010019614m 35s

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.

This allows all the hooks when getting an order to be executed and we cache the correct object.
Copy link
Contributor

@vedanshujain vedanshujain left a comment

Choose a reason for hiding this comment

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

LGTM!

@codecov
Copy link

codecov bot commented Apr 5, 2023

Codecov Report

Merging #37569 (817458a) into trunk (2cd5cdc) will decrease coverage by 0.0%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             trunk   #37569     +/-   ##
==========================================
- Coverage     51.7%    51.7%   -0.0%     
  Complexity   17257    17257             
==========================================
  Files          429      429             
  Lines        79765    79765             
==========================================
- Hits         41202    41200      -2     
- Misses       38563    38565      +2     
Impacted Files Coverage Δ
...ocommerce/includes/abstracts/abstract-wc-order.php 76.0% <100.0%> (ø)

... and 1 file with indirect coverage changes

@vedanshujain vedanshujain merged commit 9e9060e into trunk Apr 5, 2023
19 of 20 checks passed
@vedanshujain vedanshujain deleted the fix/order-cache-order-type branch April 5, 2023 15:19
@github-actions github-actions bot added this to the 7.7.0 milestone Apr 5, 2023
@vedanshujain vedanshujain modified the milestones: 7.7.0, 7.6.0 Apr 5, 2023
lsinger pushed a commit that referenced this pull request Apr 5, 2023
* Do not attempt to cache orders during order creation (#37569)

* Prep for cherry pick 37569

---------

Co-authored-by: Vedanshu Jain <vedanshu.jain.2012@gmail.com>
Co-authored-by: WooCommerce Bot <no-reply@woocommerce.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin: woocommerce Issues related to the WooCommerce Core plugin.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Orders cache overrides specific instance of order class with generic WC_Order under certain circumstances
2 participants