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

Add helper method to get WooCommerce object meta value #723

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

nmolham-godaddy
Copy link
Contributor

Summary

Adds SV_WC_Helper::getWooCommerceObjectMetaValue that gets value of a meta key from WooCommerce object based on its data type.

Story: MWC-17547

QA

  • Code review
  • Checks pass

Before merge

  • I have confirmed these changes in each supported minor WooCommerce version

*/
public static function getWooCommerceObjectMetaValue($object, string $field, bool $single = true)
{
$orderUtilExists = class_exists(OrderUtil::class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Reading through the Woo source code, I understand what you have here will absolutely work for any WC class that extends WC_Data (such as products).

Just wondering if it's at all weird/risky to use the OrderUtil class anyway? That class is described as a utility class for orders, and it does work with products anyway, but do you think there's a risk of that ever changing so that it is orders only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, as far as I can tell, the same OrderUtil class is used in many places in WC codebase to get all sort of meta data for objects despite being orders or not! I know it is weird and sounds risky, but if you look into OrderUtil::get_post_or_object_meta source code, it doesn't case if the passed object is an order or not, it just loos for the WC_Data or WP_Post

I can't find an equivalent to is within WC codebase for products or any other WC_Data/WP_Post based objects

Copy link
Contributor

Choose a reason for hiding this comment

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

Knowing it's considered to be an internal utility class, it might not be the worst idea to abstract the source of get_post_or_object_meta() to our own trait or helper.

	public function get_post_or_object_meta( ?WP_Post $post, ?\WC_Data $data, string $key, bool $single ) {
		if ( isset( $data ) ) {
			if ( method_exists( $data, "get$key" ) ) {
				return $data->{"get$key"}();
			}
			return $data->get_meta( $key, $single );
		} else {
			return isset( $post->ID ) ? get_post_meta( $post->ID, $key, $single ) : false;
		}
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first I thought about doing so, but I remembered how many time WC change stuff internally that causes compatibility issues. So, the idea here is to use their implementation with fallback with our own implementation.

Make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'm going to try just adding it as a second method to the helper, see how annoying the tests are to write.

Copy link
Contributor

@ajaynes-godaddy ajaynes-godaddy Oct 22, 2024

Choose a reason for hiding this comment

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

Added the compat method in 9f45812 and 287c7be

Feel free to revert or adjust as needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That looks good to me, how about you @agibson-godaddy, what do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants