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

Address possible metadata duplication in 8.1 (backport) #40148

Merged
merged 8 commits into from Sep 14, 2023

Conversation

jorgeatorres
Copy link
Member

@jorgeatorres jorgeatorres commented Sep 13, 2023

Submission Review Guidelines:

Changes proposed in this Pull Request:

This PR applies a subset of the changes from #40129, which -among other things-, prevent duplication of metadata when using the save_meta_data() order method, which is common with 3rd party code (including popular plugins such as WC Subscriptions).

How to test the changes in this Pull Request:

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

  1. Make sure both HPOS and sync are enabled in WooCommerce > Settings > Advanced > Features.

  2. Run the following PHP snippet via WP-CLI (for example):

    $order = wc_create_order();
    $order->add_meta_data( 'this_gets_duplicated', 'for reals' );
    $order->save_meta_data();
    
    echo $order->get_id();

    Take note of the ID that gets printed to the screen.

  3. Make sure that metadata this_gets_duplicated appears only once in the output of the following WP-CLI commands:

    1. wp db query "SELECT * FROM $(wp db prefix)wc_orders_meta WHERE order_id = ORDER_ID"
    2. wp post meta list ORDER_ID

    Replace ORDER_ID with the ID from step 2.

In 8.1 (without this fix), you'll see that steps 3 shows you the metadata duplication.

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 github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Sep 13, 2023
@jorgeatorres jorgeatorres changed the title Release 8 1 fix dup metadata Address possible metadata duplication in 8.1 (backport) Sep 13, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 13, 2023

Test Results Summary

Commit SHA: 57d08d7

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 52s
E2E Tests1950015021019m 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.

@jonathansadowski jonathansadowski added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Sep 13, 2023

if ( $object instanceof WC_Abstract_Order && $this->should_backfill_post_record() ) {
$delete_meta = $this->data_store_meta->delete_meta( $object, $meta );
$changes_applied = $this->after_meta_change( $object, $meta );
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure it's essential, but should we do anything if this returns false? Atm I don't believe we do anything with $changes_applied.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, this should be included, adding a small commit for this.

Copy link
Member

@barryhughes barryhughes left a comment

Choose a reason for hiding this comment

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

I think we're good, so provisionally approving (did leave one question, though).

@vedanshujain
Copy link
Contributor

Seems like PHP 7.4 tests with nightly are failing because of something in the build process. I'd recommend not fixing them here as this PR targets the release branch.

@vedanshujain vedanshujain marked this pull request as ready for review September 14, 2023 07:24
Copy link
Contributor

@peterfabian peterfabian left a comment

Choose a reason for hiding this comment

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

LGTM, cross-checked with #40129.

@jonathansadowski jonathansadowski merged commit 839da56 into release/8.1 Sep 14, 2023
22 of 25 checks passed
@jonathansadowski jonathansadowski deleted the release-8-1-fix-dup-metadata branch September 14, 2023 20:06
@nigeljamesstevenson nigeljamesstevenson added needs: internal testing Indicates if the PR requires further testing conducted by Solaris status: analysis complete Indicates if a PR has been analysed by Solaris and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels Sep 18, 2023
@alopezari alopezari added this to the 8.1.0 milestone Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: internal testing Indicates if the PR requires further testing conducted by Solaris plugin: woocommerce Issues related to the WooCommerce Core plugin. status: analysis complete Indicates if a PR has been analysed by Solaris
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants