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.

Sign up for GitHub

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
Open

Add helper method to get WooCommerce object meta value #723

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
18f7def
Add helper method to get WooCommerce object meta value based on its d…
nmolham-godaddy Oct 10, 2024
3a6fc7a
Unit test SV_WC_Helper::getWooCommerceObjectMetaValue method
nmolham-godaddy Oct 10, 2024
5249fea
Add preserveGlobalState annotation
nmolham-godaddy Oct 10, 2024
9f45812
Drop the dependency on OrderUtil, add a compat method with tests inst…
ajaynes-godaddy Oct 22, 2024
287c7be
Rename test methods to remove OrderUtil references.
ajaynes-godaddy Oct 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions tests/Mocks/OrderUtil.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<?php

namespace Automattic\WooCommerce\Utilities;

class OrderUtil
{
public static function get_post_or_object_meta(?\WP_Post $post, ?\WC_Data $data, string $key, bool $single)
{
return '';
}
}
90 changes: 89 additions & 1 deletion tests/unit/HelperTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
<?php

namespace SkyVerge\WooCommerce\PluginFramework\v5_14_0\Tests\Unit;
namespace SkyVerge\WooCommerce\PluginFramework\v5_15_1\Tests\Unit;

use Automattic\WooCommerce\Utilities\OrderUtil;
use Mockery;
use ReflectionException;
use SkyVerge\WooCommerce\PluginFramework\v5_15_1\SV_WC_Helper;
use SkyVerge\WooCommerce\PluginFramework\v5_15_1\SV_WC_Plugin_Compatibility;
use SkyVerge\WooCommerce\PluginFramework\v5_15_1\Tests\TestCase;
use WP_Mock;

class HelperTest extends TestCase
{
Expand Down Expand Up @@ -51,4 +54,89 @@ public function testAlwaysDetermineNavigationFeaturedDisabled() : void

$this->assertFalse(SV_WC_Helper::is_wc_navigation_enabled());
}

/**
* @covers \SkyVerge\WooCommerce\PluginFramework\v5_15_1\SV_WC_Helper::getWooCommerceObjectMetaValue()
*
* @runInSeparateProcess
* @preserveGlobalState
*
* @throws ReflectionException
*/
public function testCanGetWooCommerceDataObjectMetaValueUsingOrderUtil() : void
{
require_once PLUGIN_ROOT_DIR.'/tests/Mocks/OrderUtil.php';

$object = Mockery::mock('WC_Data');

$object->expects('get_meta')->never();

$this->mockStaticMethod(OrderUtil::class, 'get_post_or_object_meta')
->once()
->with(null, $object, $field = 'TEST_FIELD', true)
->andReturn($value = 'WC_DATA_META_VALUE');

$this->assertSame($value, SV_WC_Helper::getWooCommerceObjectMetaValue($object, $field));
}

/**
* @covers \SkyVerge\WooCommerce\PluginFramework\v5_15_1\SV_WC_Helper::getWooCommerceObjectMetaValue()
*
* @runInSeparateProcess
* @preserveGlobalState
*/
public function testCanGetWooCommerceDataObjectMetaValueWithoutUsingOrderUtil() : void
{
$object = Mockery::mock('WC_Data');

$object->expects('get_meta')
->once()
->with($field = 'TEST_FIELD', true)
->andReturn($value = 'WC_DATA_META_VALUE');

$this->assertSame($value, SV_WC_Helper::getWooCommerceObjectMetaValue($object, $field));
}

/**
* @covers \SkyVerge\WooCommerce\PluginFramework\v5_15_1\SV_WC_Helper::getWooCommerceObjectMetaValue()
*
* @runInSeparateProcess
* @preserveGlobalState
*
* @throws ReflectionException
*/
public function testCanGetWordPressPostMetaValueUsingOrderUtil() : void
{
require_once PLUGIN_ROOT_DIR.'/tests/Mocks/OrderUtil.php';

$object = Mockery::mock('WP_Post');

WP_Mock::userFunction('get_post_meta')->never();

$this->mockStaticMethod(OrderUtil::class, 'get_post_or_object_meta')
->once()
->with($object, null, $field = 'TEST_FIELD', true)
->andReturn($value = 'WP_POST_META_VALUE');

$this->assertSame($value, SV_WC_Helper::getWooCommerceObjectMetaValue($object, $field));
}

/**
* @covers \SkyVerge\WooCommerce\PluginFramework\v5_15_1\SV_WC_Helper::getWooCommerceObjectMetaValue()
*
* @runInSeparateProcess
* @preserveGlobalState
*/
public function testCanGetWordPressPostMetaValueWithoutUsingOrderUtil() : void
{
$object = Mockery::mock('WP_Post');
$object->ID = 123;

WP_Mock::userFunction('get_post_meta')
->once()
->with($object->ID, $field = 'TEST_FIELD', true)
->andReturn($value = 'WP_POST_META_VALUE');

$this->assertSame($value, SV_WC_Helper::getWooCommerceObjectMetaValue($object, $field));
}
}
31 changes: 31 additions & 0 deletions woocommerce/class-sv-wc-helper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,10 @@

namespace SkyVerge\WooCommerce\PluginFramework\v5_15_1;

use Automattic\WooCommerce\Utilities\OrderUtil;
use SkyVerge\WooCommerce\PluginFramework\v5_15_1\Helpers\NumberHelper;
use WC_Data;
use WP_Post;

defined( 'ABSPATH' ) or exit;

Expand Down Expand Up @@ -1197,6 +1200,34 @@ public static function get_escaped_id_list( array $ids ) {
}


/**
* Gets value of a meta key from WooCommerce object based on its data type.
*
* @param WP_Post|WC_Data $object
* @param string $field
* @param bool $single
*
* @return array|mixed|string|null
*/
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 •
edited
Loading

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?


if ($object instanceof WP_Post) {
return $orderUtilExists ?
OrderUtil::get_post_or_object_meta($object, null, $field, $single) :
get_post_meta($object->ID, $field, $single);
}

if ($object instanceof WC_Data) {
return $orderUtilExists ?
OrderUtil::get_post_or_object_meta(null, $object, $field, $single) :
$object->get_meta($field, $single);
}

return null;
}

}


Expand Down
Loading