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

Prevent possible warning in COTMigrationUtil::get_post_or_object_meta() #37026

Merged
merged 2 commits into from Mar 2, 2023

Conversation

jorgeatorres
Copy link
Member

@jorgeatorres jorgeatorres commented Mar 1, 2023

All Submissions:

Changes proposed in this Pull Request:

This PR makes sure that $post->ID is set before attempting to use it inside calls to COTMigrationUtil::get_post_or_object_meta().

This function mostly exists for HPOS/posts compat reasons and serves functions woocommerce_wp_select(), woocommerce_wp_text_input(), woocommerce_wp_hidden_input(), etc. in wc-meta-box-functions.php.

Closes #35543.

How to test the changes in this Pull Request:

Per #35543, you should "write a snippet to call woocommerce_wp_select in a WooCommerce > Settings hook […]".

The issue can also be replicated by just calling those functions in almost any context where the $post global is not set, but to replicate the exact conditions of #35543, the following can be used:

  1. Create a test plugin/snippet with the following code (or add it to the theme's functions.php)
    add_action(
        'woocommerce_settings_general',
        function() {
     	   woocommerce_wp_select(
     		   array(
     			   'id'          => 'my-favorite-topping',
     			   'value'       => 'pepperoni',
     			   'label'       => 'Pizza Topping',
     			   'options'     => [
     				   'pineapple' => 'Pineapple',
     				   'salami'    => 'Salami',
     				   'pepperoni' => 'Pepperoni',
     				   'onion'     => 'Onion',
     			   ],
     			   'desc_tip'    => true,
     			   'description' => 'Toppings are a serious matter. Choose wisely.',
     		   )
     	   );
        },
        999
    );
  2. Go to WC > Settings and scroll to the bottom of the page.
  3. You should see a dropdown named "Pizza Topping" (no need to make a selection, though the best choice has been pre-selected for you).
  4. Now, to the "errors" part:
    • On trunk, you should see a warning in the log such as PHP Notice: Trying to get property 'ID' of non-object in […].
    • On this branch, no warning should be logged.

@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Mar 1, 2023
@@ -96,7 +96,7 @@ public function get_post_or_object_meta( ?WP_Post $post, ?\WC_Data $data, string
}
return $data->get_meta( $key, $single );
} else {
return get_post_meta( $post->ID, $key, $single );
return isset( $post->ID ) ? get_post_meta( $post->ID, $key, $single ) : false;
Copy link
Member Author

Choose a reason for hiding this comment

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

Just noting that I've decided to use false as "default" value when $post->ID is not set, just to make get_post_or_object_meta () mimic what get_post_meta() does: return false for invalid $post_id (see codex). This, however, makes probably little difference in real use.

@jorgeatorres jorgeatorres requested review from a team and rrennick and removed request for a team March 1, 2023 23:01
@codecov
Copy link

codecov bot commented Mar 1, 2023

Codecov Report

Merging #37026 (7c76118) into trunk (ac47d18) will decrease coverage by 0.0%.
The diff coverage is 24.3%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             trunk   #37026     +/-   ##
==========================================
- Coverage     46.7%    46.7%   -0.0%     
- Complexity   17183    17188      +5     
==========================================
  Files          429      429             
  Lines        64799    64820     +21     
==========================================
+ Hits         30251    30253      +2     
- Misses       34548    34567     +19     
Impacted Files Coverage Δ
...ocommerce/includes/admin/class-wc-admin-assets.php 0.0% <0.0%> (ø)
...mmerce/includes/admin/class-wc-admin-importers.php 0.0% <0.0%> (ø)
plugins/woocommerce/includes/class-wc-ajax.php 4.3% <0.0%> (-0.1%) ⬇️
...s/Version1/class-wc-rest-coupons-v1-controller.php 42.9% <0.0%> (ø)
...rs/Version1/class-wc-rest-orders-v1-controller.php 46.2% <0.0%> (ø)
...rters/class-wc-product-csv-importer-controller.php 36.1% <28.6%> (ø)
.../includes/import/class-wc-product-csv-importer.php 74.9% <44.4%> (ø)
...s/Version2/class-wc-rest-coupons-v2-controller.php 94.7% <75.0%> (ø)
plugins/woocommerce/includes/class-wc-coupon.php 73.9% <100.0%> (ø)
...lugins/woocommerce/includes/class-wc-discounts.php 84.5% <100.0%> (ø)
... and 19 more

@github-actions
Copy link
Contributor

github-actions bot commented Mar 1, 2023

Test Results Summary

Commit SHA: 7c76118

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 53s
E2E Tests189006019517m 11s

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.

@PanosSynetos
Copy link
Contributor

Hey @jorgeatorres , thanks for working on this! - Once this is ready to merge, could you please share a zip so we can test it on our environments? cc @jimjasson

@rrennick
Copy link
Contributor

rrennick commented Mar 2, 2023

@jorgeatorres There were issues in the PHP 7.4 unit tests.

@jorgeatorres
Copy link
Member Author

@jorgeatorres There were issues in the PHP 7.4 unit tests.

Thanks @rrennick. Don't know why those failed but after re-running the job, they are now passing.

Copy link
Contributor

@rrennick rrennick left a comment

Choose a reason for hiding this comment

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

LGTM. Tested great.

@rrennick rrennick merged commit 3cee721 into trunk Mar 2, 2023
@rrennick rrennick deleted the fix/35543 branch March 2, 2023 18:05
@github-actions github-actions bot added this to the 7.6.0 milestone Mar 2, 2023
@rrennick
Copy link
Contributor

rrennick commented Mar 2, 2023

Once this is ready to merge, could you please share a zip so we can test it on our environments?

@PanosSynetos I went ahead and merged this as it was a single line change. If you want to test this you can use pnpm run build:zip in the plugins/woocommerce folder to create a zip.

@PanosSynetos
Copy link
Contributor

Thanks, we'll test it out! (sorry I missed the ping)

@PanosSynetos
Copy link
Contributor

Tested, we're good 👌 and the warning is gone

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.

Calling woocommerce_wp_select in WooCommerce > Settings context throws a notice
3 participants